-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[RNTuple] Implement I/O Performance Metrics: Sparseness, Randomness, and Transactions #20994
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
base: master
Are you sure you want to change the base?
[RNTuple] Implement I/O Performance Metrics: Sparseness, Randomness, and Transactions #20994
Conversation
jblomer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Before going into a detailed review, I see a few larger items that need to be addressed.
- The metrics should not be added directly as atomic integers to the RNTupleMetrics object. We have
RNTuplePerfCounterfor this. - Nice observation on the randomness metric. However, I would prefer doing this internally, e.g. excluding header and footer or having two metrics (with and without).
- I think it makes more sense to do the accounting in the
fReader; should save us to chase all the invocations ofReadBuffer.
|
@jblomer Thanks for the review. I have updated the PR to address your three main points: Counters: I replaced the Accounting: I moved all I/O accounting into Randomness Logic: As suggested, I handled the exclusion internally rather than via I added an internal The reader automatically switches to the "Analysis Phase" at the end of
|
|
While verifying the changes, I noticed a pre-existing issue with This results in 0 metrics during verification tests that snapshot the reader across a move. It likely doesn't affect production (where the reader owns the source), but it breaks the "Snapshot & Subtract" verification logic. I have not added a custom Move Constructor to fix this yet, as it felt like a separate architectural fix outside the scope of this metrics refactor. Let me know if you would prefer I include that fix here. |
|
I pushed a polish commit addressing 3 specific things that i noticed while reviewing:
|
|
I'm afraid this isn't going quite in the right direction. Take a look at |
fc79786 to
c92fc14
Compare
|
hey @jblomer, Thanks for the review, sorry about miss understanding the previous review. I moved the implementation to follow the please let me know if they are in the right directions, or should i look a bit deeper in the library. On a side note RNTuple seems very interesting to me, I'm interested in the low-level systems engineering side of this and learning about columnar data structures in general, could you let me know if there is anyway to deep dive and understand it even better (i did go through the root reference guide i could only find an introduction page. And i also went through the youtube video on the HSF youtube channel but it did not have a lot of details). please have a review and let me know if these changes are as desired. Thanks! |
c92fc14 to
5a8845d
Compare
This Pull request:
improves the existing Performance Metrics in RNTuple.
Changes or fixes:
This PR addresses the issue that had been raised for the improvements of metrics. It implements,
Implementation Details:
These have been implemented through the introduction of transient, thread-safe members in RNTupleMetrics,
std::atomicstd::uint64_t fSumSkip: To track total seek distance.
std::atomicstd::uint64_t fExplicitBytesRead: To track payload bytes.
std::atomicstd::uint64_t fTransactions: To track I/O operations.
I have changed RPageStorageFile to update these counters during ReadBuffer, ReadV, and LoadClusters operations. The use of std::atomic ensures thread safety with negligible performance overhead on the I/O path.
Reset Functionality & Accuracy:
I added a Reset() method to RNTupleMetrics. This is needed for obtaining accurate Randomness metrics for the analysis loop.
Without Reset(): The metric is dominated by the initial file seek (Header → Footer → Header), resulting in a Randomness score > 2.0.
With Reset(): Users can clear the counters after initialization, isolating the steady-state performance of their event loop.
Verification:
Added a new unit test TEST(Metrics, IOMetrics) in tree/ntuple/test/ntuple_metrics.cxx to verify the logic and the Reset() behavior.
Also tested locally using this code
Click to view Manual Verification Code & Output
This PR fixes #20853