Repository navigation
[ntuple] Grow RVec capacity geometrically in ResizeRVec - #23685
ishaaaan17 wants to merge 1 commit into
Conversation
When reading consecutive entries with variable-sized RVec collections, ResizeRVec previously reallocated only enough capacity for nItems. This incurred frequent reallocations across entries with growing element counts. Grow capacity geometrically (doubling, capped at int32_t max) when reallocating an already-allocated RVec, amortizing allocation costs and avoiding repeated malloc/free cycles across entry iterations. Assisted-by: antigravity:gemini
There was a problem hiding this comment.
Seems reasonable to me. Although should we also provide a ShrinkToFit method for specific cases that want to reclaim all possible memory?
@jblomer thoughts?
Test Results 24 files 24 suites 3d 23h 5m 46s ⏱️ For more details on these failures, see this check. Results for commit 3ba182d. |
jblomer
left a comment
There was a problem hiding this comment.
Thank you very much for addressing this TODO!
I took your commit and extended it a it (since we are on the subject) in PR #23718
Specifically, I changed the growth factor to 1.5 for all but small vectors (with the aim to be a bit more memory-friendly). An in the same spirit, there is also automatic shrinking added.
Let's continue the discussion in #23718
|
Thank |
this pull request:
makes
RRVecField::ResizeRVecgrow capacity geometrically (doubling, capped atstd::int32_tmax) when reading variable-sizedROOT::RVec<T>collections.before,
ResizeRVecreallocated just enough fornItems. if the collection size crept up from event to event, that meant a lot ofmalloc/freecycles.this takes care of the todo in
tree/ntuple/src/RFieldSequenceContainer.cxx:// TODO Increment capacity by a factor rather than just enough to fit the elements.changes or fixes:
RVecthat is already allocated needs to grow, its capacity now grows geometrically instead of to exactlynItems.TEST(RNTuple, RVecGeometricGrowth)intree/ntuple/test/rfield_vector.cxx. it checks the growth and that capacity is kept across entries of different sizes..clang-format.checklist:
ai-assisted coding disclosure:
Assisted-by: antigravity:gemini. i used an llm assistant to look at different ways of growing the capacity and to draft the first version of the changes. i then went through the memory semantics, edge cases, alignment constraints and the tests myself.