Skip to content

[ntuple] Grow RVec capacity geometrically in ResizeRVec - #23685

Closed
ishaaaan17 wants to merge 1 commit into
root-project:masterfrom
ishaaaan17:perf/ntuple-rvec-geometric-growth
Closed

ishaaaan17 wants to merge 1 commit into
root-project:masterfrom
ishaaaan17:perf/ntuple-rvec-geometric-growth

Conversation

@ishaaaan17

Copy link
Copy Markdown

this pull request:

makes RRVecField::ResizeRVec grow capacity geometrically (doubling, capped at std::int32_t max) when reading variable-sized ROOT::RVec<T> collections.

before, ResizeRVec reallocated just enough for nItems. if the collection size crept up from event to event, that meant a lot of malloc/free cycles.

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:

  • when an RVec that is already allocated needs to grow, its capacity now grows geometrically instead of to exactly nItems.
  • the first allocation and memory-adopting transfers still use the exact capacity needed, so single-element containers don't get over-allocated.
  • added the unit test TEST(RNTuple, RVecGeometricGrowth) in tree/ntuple/test/rfield_vector.cxx. it checks the growth and that capacity is kept across entries of different sizes.
  • formatted with ROOT's .clang-format.

checklist:

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

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.

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

@silverweed silverweed left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems reasonable to me. Although should we also provide a ShrinkToFit method for specific cases that want to reclaim all possible memory?
@jblomer thoughts?

@github-actions

Copy link
Copy Markdown

Test Results

    24 files      24 suites   3d 23h 5m 46s ⏱️
 3 886 tests  3 885 ✅ 0 💤 1 ❌
83 181 runs  83 180 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 3ba182d.

@jblomer jblomer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@jblomer

jblomer commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Thank

@jblomer jblomer closed this Oct 14, 2026
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.

4 participants