perf: batch sink flushes and cache cluster group records when extending RNTuples - #1700
Draft
ariostas wants to merge 1 commit into
Draft
perf: batch sink flushes and cache cluster group records when extending RNTuples#1700ariostas wants to merge 1 commit into
ariostas wants to merge 1 commit into
Conversation
…ng RNTuples
add_rblob flushed the sink for every blob it wrote, so one extend flushed once
per column, once for the page list envelope, and once for the footer, before
the explicit flush at the end of extend. Flush only at the commit boundary
instead. write() now flushes explicitly, since it relied on add_rblob for that.
Every extension also rewrites the whole footer, which re-serialized every
cluster group record written so far even though a record is never modified
after it is appended. Cache each record's bytes, removing a quadratic term
from repeated extension.
Measured over repeated extends of an int64 RNTuple (local file sink):
5 columns, 1600 extends: 2.00s -> 1.24s, 12803 -> 1602 flushes
50 columns, 100 extends: 0.48s -> 0.46s, 5303 -> 102 flushes
File contents are unchanged. This does not address the footer *bytes* written
growing quadratically, which is inherent to rewriting a valid footer on every
extension and would need a design change to defer it to close.
Assisted-by: claude-code:claude-opus-5[1m]
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Addresses finding 2 of "PR 2" in #1688.
Flush per blob → flush per commit
add_rblobflushed the sink for every blob it wrote, so a singleextendflushed once per column, once for the page list envelope and once for the footer, before the explicitsink.flush()already at the end ofextend. That is especially expensive for remote sinks. Flushing now happens only at the commit boundary;write()flushes explicitly since it relied onadd_rblobfor that.Data is still on disk at the end of every
extend— there is a test for that.Cached cluster group records
Every extension rewrites the whole footer, which re-serialized every cluster group record written so far, even though a record is never modified once appended. At 800 extends that was ~320k calls to
NTuple_ClusterGroupRecord.serializeand roughly a third of the wall time. Caching the bytes removes the quadratic term.Measured
Repeated
extendof an int64 RNTuple, local file sink:The 50-column row shows the flush reduction rather than a wall-clock win, because a local flush is cheap; that is the case remote sinks care about.
File contents are byte-for-byte unchanged.
Not addressed
The bytes written for footers still grow quadratically (the footer is rewritten in full on every extension, and it grows by one cluster group record each time — ~38 kB after 800 extends). That is inherent to keeping a valid footer on disk after every extension; deferring it to
close()would leave an unclosed file unreadable, so it seems like a design decision rather than something to slip into a perf PR.I also left
sink.set_file_lengthper blob rather than hoisting it, since skipping it mid-extend relies on writes past EOF zero-filling, which is guaranteed locally but not obviously so for every fsspec backend.Tests
tests/test_1688_rntuple_extend_flushing.py: flush count perextendpinned at 1 for 1, 8 and 32 columns; a 20-step round trip; the record cache; and readability of the file between extends. 4 of the 6 fail onmain.Full suite passes locally (1029 passed, 90 skipped).