perf(odf): halve what a spreadsheet costs to decode, and stop two repeat bombs - #776
Merged
Conversation
The odf, ooxml, svg and xml engines keep the parsed dom resident as their backing store rather than throwing it away after parsing, so the size of a node is the size of the document. A `xml_node_struct` costs 64 bytes by default and 12 in compact mode, an `xml_attribute_struct` 40 and 8. On a 297 MB `content.xml` (6.3M nodes, 5.3M attributes) that is 594 MB of structure against 116 MB, out of the same buffer and the same parse call, and it parses no slower. Compact is the floor for a tree of pointers into the source: names and values are one byte each, page-relative against a shared base. The define is ABI affecting, and a translation unit that misses it links fine and then reads the tree through the wrong layout - so it rides on the imported target rather than on `odr`, which links pugixml PRIVATE while `odr_test` links it again on its own. `header_only=True` is the other half: with a prebuilt library the define alone would compile and then corrupt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX
A sheet already keeps repeated columns, rows and empty cells as a single entry at the end of the range they repeat over. Cells with content did not get that: the parser expanded them, building a cell, a paragraph and a text element for every position the repeat covered. Both repeat counts are unbounded, and a sheet is 1048576 x 1024 - so a flat document of 409 bytes asked for half a gigabyte, and one naming the whole grid asks for three billion elements. Nothing rejects it earlier; the process is simply killed. Registering the group once removes the expansion, and with it the whole empty-row special case, which existed only to avoid it: the two branches become the same code. A cell's `TablePosition` is now the anchor of its range rather than each position it covers - which only `DocumentPath::Cell` and `element_is_editable` read, and spreadsheets are not editable. The rendered output is unchanged: `sheet.cell(column, row)` resolves every position of the range to the same entry, as it already did for empty ones. The whole ods corpus renders byte for byte as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX
…deque Three changes to what a decoded sheet costs, none of them visible in the output. The `columns`/`rows`/`cells` index was three levels of `std::map`. Parsing appends in document order, so the keys only ever grow and a tree buys nothing over a sorted vector resolved with an upper bound - while a rb-tree node costs 64 bytes to carry 12 of payload, and every row carried a map of its own. The cells now live in one array per sheet, each row recording where its run starts, so a sheet is two allocations rather than one per row. On a million-row sheet: 274 MB of maps against 89 MB of vectors. The elements were a `std::vector`. `create_element` hands back a reference and the parser keeps parsing, a million elements deep, so the container must not reallocate under it; a deque also spares the document the doubling, which at three million elements holds a third more memory than it has elements and reaches its peak holding both halves. `table_rows`/`table_columns` materialised every row of a sheet into a vector, on each of the three walks a render makes - `for_each_table_row` visits them instead. The collecting versions had no callers left outside the tests, which now collect for themselves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX
`<mergeCell ref=…>` marks the cells it covers by visiting every position in the range. A `ref` may name any range the grid allows, and the grid is 17 billion positions - so "A1:XFD1048576" in a workbook of a few hundred bytes walks all of them, and the open never returns. Past the point where the range is wider than the sheet has cells, walk the cells and ask which ones the range contains. The work is then bounded by what was actually read. Both paths mark the same cells; forcing the new one for every merge leaves the corpus - including a workbook with 111 of them - rendering byte for byte as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX
…with `PUGIXML_COMPACT` rides on the imported target and `odr` links pugixml PRIVATE, so the define stops at the library. But `install(DIRECTORY src/ ... PATTERN "*.hpp")` ships the internal headers too, and sixteen of them expose pugixml types - `odf_document.hpp`, `ooxml_util.hpp`, `xml_file.hpp` and the registries among them. A consumer that includes one compiles a 64-byte node against a library built with a 12-byte one: the silent corruption the define is otherwise careful about, one scope out. There is no `install(EXPORT)` here - the consumer's target is generated by conan - so it takes both halves. `target_compile_definitions(odr INTERFACE)` covers anything linking `odr` in tree or by `add_subdirectory`, which `cli/translate` now compiles with and did not before; `cpp_info.defines` covers the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d9ycFE78bvn43TyiH8Tv8
The comments carried their own measurements and the story of how the change was arrived at; git and the pull request have that. Each now keeps the fact a reader needs at the call site and nothing more, and the rationale worth keeping stays in the two AGENTS.md, which is where this repo puts it. `CHANGELOG.md` had nothing under `## Unreleased`, which a release run refuses, and three of these are consumer-visible: the two repeat bombs, and - breaking - `SheetCell::position()` reporting the anchor of the range a repeated ods cell covers rather than the position it was looked up at, with `Sheet::cell()` handing back the same element throughout that range. Also records what `cursor.add_row(rows_repeated)` drops: `add_row` clears the cursor's pending ranges for a repeat greater than one, so a rowspan reaching out of a repeated row is lost. That is contradictory ODF and the old empty-row path did the same, so it is a TODO and not a fix. And `row_cells` does pointer arithmetic over `rows`, so it says which rows it means. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019d9ycFE78bvn43TyiH8Tv8
andiwand
force-pushed
the
perf/odf-sheet-memory
branch
from
August 30, 2026 06:48
0e0e9a2 to
442b360
Compare
This was referenced Aug 30, 2026
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.
🤖 Generated with Claude Code
Closes #768. Takes most of #762, and fixes a bug found on the way.
odr-private/ods/efficiency-big-1.ods(1.4 MB, a 297 MBcontent.xml) went from1667 MB to 873 MB of peak footprint, and from 1.87 s to 1.22 s to decode.
Rendering the first view now adds nothing measurable on top.
Measured with
phys_footprint— the metric jetsam and lmkd actually use — onmacOS arm64, streaming the html to a sink.
content.xml, which the dom points intoThe four commits
perf(xml): pugixml in compact mode. Axml_node_structcosts 12 bytesinstead of 64, an attribute 8 instead of 40. The engines keep the parsed dom
resident as their backing store, so this is the single largest line item, it
helps odf, ooxml, svg and the xml source view alike, and no
src/code changes.The define is ABI affecting and a translation unit that misses it links fine and
then reads the tree through the wrong layout, so it rides on the imported target
and is paired with
header_only=True— there must be no prebuilt library tomismatch against.
xml_util.cppasserts the layout it compiled against.conan.lockneeds no change: it pins recipe revisions, and an option changes thepackage id, not the revision.
fix(odf): a repeated sheet cell is one entry, not one per position. A sheetalready stored repeated columns, rows and empty cells once, at the end of the
range they repeat over. Cells with content were expanded — three elements per
position. Both repeat counts are unbounded and the grid is 1048576 x 1024, so:
A
refnaming the whole grid asks for three billion elements from four hundredbytes. Collapsing the group removes the expansion and with it the entire
empty-row special case, which existed only to avoid it — the two branches become
the same code.
perf(odf): sorted vectors for the sheet index, a deque for the elements.Parsing appends in document order, so the index keys only grow and a tree buys
nothing over an upper bound — while a rb-tree node spends 64 bytes carrying 12,
and every row carried a map of its own. Cells now live in one array per sheet,
each row recording where its run starts: two allocations per sheet rather than
one per row. The elements move to a deque because
create_elementhands back areference and the parser keeps parsing under it, and because a vector reaches its
peak holding both halves of the doubling.
table_rows/table_columnsmaterialised every row of a sheet on each of the three walks a render makes;
for_each_table_rowvisits them instead.fix(ooxml): bound a merged range by the cells the sheet has. Sibling of theodf bomb, found while measuring.
<mergeCell ref="A1:XFD1048576"/>marked coveredcells by visiting all 17 billion positions, so a workbook of a few hundred bytes
never finished opening. Past the point where the range is wider than the sheet has
cells, walk the cells instead.
Verification
odr_testgreen: 1256 pass, 6 pre-existing skips.private — which is the point: every change here is representation, not meaning.
1048576 x 1024repeat is under 16 elements and stillreads correctly at every position it covers; a whole-grid
mergeCellopens in0 ms (it does not return within 8 s with the guard removed).
mergeCellpaths were shown equivalent by forcing the new one forevery merge and re-running the corpus,
ruski.xlsx's 111 merges included.cli,jniandodr_testall build; the debug and release conan profileswere regenerated.
Not in here
Left for #762, with measured numbers in the issue: the element registry still
builds a cell, a paragraph and a text for every non-empty cell before any
HtmlConfiglimit applies, and the 284 MB source buffer goes only with astreaming parse. The remaining ladder — side maps to sorted vectors, a narrower
stored id, an exact reserve — is worth about another 170 MB in the same file.