Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 18, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions tree/ntuple/v7/src/RNTuple.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,12 @@
void ROOT::RNTuple::Streamer(TBuffer &buf)
{
if (buf.IsReading()) {
UInt_t offClassBuf;
UInt_t bcnt;
auto classVersion = buf.ReadVersion(&offClassBuf, &bcnt);

// Strip class version from checksum calculation
UInt_t lenStrip = sizeof(Version_t);

if (bcnt < lenStrip)
throw Experimental::RException(R__FAIL("invalid anchor byte count: " + std::to_string(bcnt)));

auto lenCkData = bcnt - lenStrip;
// Skip byte count and class version
auto offCkData = offClassBuf + sizeof(UInt_t) + sizeof(Version_t);
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

@jblomer jblomer Nov 15, 2024

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

@hahnjo hahnjo Nov 15, 2024

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

@pcanal pcanal Nov 15, 2024

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.


std::uint64_t onDiskChecksum;
buf.ReadClassBuffer(RNTuple::Class(), this, classVersion, offClassBuf, bcnt);
if (static_cast<std::size_t>(buf.BufferSize()) < buf.Length() + sizeof(onDiskChecksum))
throw Experimental::RException(R__FAIL("the buffer containing RNTuple is too small to contain the checksum!"));
buf >> onDiskChecksum;
Expand Down
Loading