Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ntuple] Simplify RNTuple::Streamer() #16956

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Nov 15, 2024

This makes the writing and reading paths symmetric.

This makes the writing and reading paths symmetric.
auto expectedChecksum = XXH3_64bits(buf.Buffer() + offCkData, lenCkData);
auto offCkData = buf.Length() + sizeof(UInt_t) + sizeof(Version_t);
buf.ReadClassBuffer(RNTuple::Class(), this);
std::uint64_t expectedChecksum = XXH3_64bits(buf.Buffer() + offCkData, buf.Length() - offCkData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we are assuming that (essentially) bcnt is the same as buf.Length(). Is this always true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, somewhat: From my understanding, buf.Length() is essentially the current offset in buf. buf.Length() - offCkData with offCkData = buf.Length() + sizeof(UInt_t) + sizeof(Version_t) is therefore the number of processed bytes after the version (which is according to bcnt).

For comparison with the old version, note that lenStrip = sizeof(Version_t), which was the "relative offset" of the class buffer, while offCkData is the absolute offset in buf. See also the writing code path for the symmetry mentioned in the commit message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bcnt is the same as buf.Length(). Is this always true?

With an offset. buf.Length() is usually already non-zero at the start of a ::Streamer function.

bcnt is the number of bytes written for an object, including the version tag.

with offCkData = buf.Length() + sizeof(UInt_t) + sizeof(Version_t)

has 2 caveats. In this expression buf.Length() is captured/recorded at the start of the ::Streamer function.
and the sizeof(Version_t) part is valid only for 'versioned classes`. For unversioned class were record both a marker (4 bytes with the value 0) and the checksum (another 4 bytes).

For RNTuple::Streamer, we know RNTuple is versioned and thus the calculation is correct.

Copy link

Test Results

    18 files      18 suites   4d 7h 28m 30s ⏱️
 2 678 tests  2 678 ✅ 0 💤 0 ❌
46 342 runs  46 342 ✅ 0 💤 0 ❌

Results for commit b8d5e81.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants