[io] Properly abort when buffer size overflows max integer or size > maxBufferSize

[ferdymercury]

This Pull request:

Changes or fixes:

Fixes #14770

And is a first step towards #6734

Checklist:

• tested changes locally
• updated the docs (if necessary)

[pcanal]

Can you make a separate PR with only the change in variable name (bufsize) to reduce the noise of this more challenging PR.

[github-actions]

Test Results

    20 files      20 suites   3d 6h 24m 9s ⏱️
 3 376 tests  3 374 ✅ 0 💤 2 ❌
65 866 runs  65 860 ✅ 0 💤 6 ❌

For more details on these failures, see this check.

Results for commit eb0dc82.

♻️ This comment has been updated with latest results.

[jblomer]

Cool, many thanks!

I think generally we want to use std::size_t for buffer sizes instead of Long64_t.

We should probably also update TBuffer::[Read|Write]Buf, TBuffer::ReadString, TBuffer::MapObject, TBuffer::[Check|Set]ByteCount, TBuffer::SetBufferDisplacement.

Maybe also TBuffer::[Read|Write]Clones.

Sometimes we are checking for kMaxBufferSize, sometimes for kMaxInt. Shouldn't we always check for kMaxBufferSize?

Regarding commit messages, I'd suggest

[NFC] remove unused headers

and

[io] accept and check long buffer size params

with an explanation why we (at this point) allow for long buffer sizes but then abort when they are actually used.

@pcanal: do we have an indication that the optimization of initializing the buffer size to the average buffer size seen so far in the file is actually useful? There are certainly write patterns where it hurts rather than helps. Removing this optimization would get us a fair amount of simplification in a number of read/write APIs.

[jblomer]

Maybe put those in another commit/PR.

[ferdymercury]

I think generally we want to use std::size_t for buffer sizes instead of Long64_t.

Even if that changes the data type from signed to unsigned?

Also, wouldn't it be better to have ULong64_t instead of std::size_t?

Since size_t is unsigned int for 32-bit targets and unsigned long long or unsigned long for 64-bit targets, depending on the C++ implementation and the compilation target.
Where as ULong64_t would always be unsigned long long.

[jblomer]

Even if that changes the data type from signed to unsigned?

Also, wouldn't it be better to have ULong64_t instead of std::size_t?

Since size_t is unsigned int for 32-bit targets and unsigned long long or unsigned long for 64-bit targets, depending on the C++ implementation and the compilation target. Where as ULong64_t would always be unsigned long long.

In my opinion, that's the point. The std::size_t type is the integer type describing a length of something in memory. This is platform-dependent. On a 32bit platform, e.g., there will never be a buffer > 4GB.

[ferdymercury]

In my opinion, that's the point. The std::size_t type is the integer type describing a length of something in memory. This is platform-dependent. On a 32bit platform, e.g., there will never be a buffer > 4GB.

Sounds reasonable. What if a TTree in the future contains a big entry over 4GB ? Does it mean that it won't be read in 32-bits? How do we error out then, or it's silently cropped? Or is it going to be emulated as several buffers one after the other?
Using a data type larger than the actual type (ulong64 vs size_t) leaves room for detecting those kind of situations.
But maybe it's all very corner cases and is not worth?

[jblomer]

In my opinion, that's the point. The std::size_t type is the integer type describing a length of something in memory. This is platform-dependent. On a 32bit platform, e.g., there will never be a buffer > 4GB.

Sounds reasonable. What if a TTree in the future contains a big entry over 4GB ? Does it mean that it won't be read in 32-bits? How do we error out then, or it's silently cropped? Or is it going to be emulated as several buffers one after the other? Using a data type larger than the actual type (ulong64 vs size_t) leaves room for detecting those kind of situations. But maybe it's all very corner cases and is not worth?

I think generally we have to distinguish between the in-memory buffer and what's serialized to disk. If a big atomic object (e.g., a histogram) is serialized to disk, it can't be read back on 32bit platforms. I think that's fine and unavoidable. The 32bit machine is simply not capable enough. On disk, of course, we will need to represent the size of objects in a platform-independent way. I think that the deserialization of the object length will be the proper point to throw errors.

Regarding the concrete on-disk representation, the plan is to chunk large objects in multiple keys to keep the changes to the TFile on-disk format minimal.

[pcanal]

do we have an indication that the optimization of initializing the buffer size to the average buffer size seen so far in the file is actually useful?

This is hard to really measure for sure as it is of course very dependent of the actual workload. When this was introduced, this was in direct reaction to issues related to not only memory fragmentation (increase of process virtual size due to the inability to re-use some memory that just a tad too small) but also thread scaling (by reducing the amount of memory allocation which requires (in most cases) the system to take a global lock).

[pcanal]

On a 32bit platform, e.g., there will never be a buffer > 4GB.

On the other hand we need to also make sure we probably error-out when there is a request for it ...

[pcanal]

What if a TTree in the future contains a big entry over 4GB ? ... Or is it going to be emulated as several buffers one after the other?

That is the current plan.

[pcanal]

That might be necessary but is a serious problem. This function is overload a lot in both our code but also very possibly in user code. Unless those user have upgrade their code to use the override keyword (which is unlikely in my opinion), their code will compile correctly but do the wrong thing (revert to use the default behavior rather than their customization .... )

[ferdymercury]

What if we implement a dummy

Int_t Write(const char *name, Int_t option, Int_t bufsize) const final;

to trigger a compilation error, or at least a warning and avoid that silenced wrong behavior?

[pcanal]

That would indeed provoke a compilation error ....