From dfa6750022aa5a4ea8329883002770559feb58d4 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 29 Aug 2026 20:00:15 +0200 Subject: [PATCH 1/6] perf(xml): build pugixml in compact mode 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) Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX --- AGENTS.md | 11 +++++++++++ CMakeLists.txt | 12 ++++++++++++ conanfile.py | 3 +++ src/odr/internal/util/xml_util.cpp | 8 ++++++++ 4 files changed, 34 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 5e20c133f..5c5eac257 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,6 +106,17 @@ cmake --build cmake-build-relwithdebinfo --target translate # CLI: file → HTM need the data, and several gigabytes should not arrive because someone turned tests on. The `update_test_data` target moves existing checkouts onto the pins. The two private repositories need credentials. +- **pugixml is built in compact mode** (`PUGIXML_COMPACT`, set on the imported + target in `CMakeLists.txt`, paired with `pugixml/*:header_only` in + `conanfile.py`). The odf/ooxml/svg/xml engines keep the parsed DOM resident as + their backing store, so its size *is* the document's: a 12-byte node instead of + 64 took a 297 MB `content.xml` from 594 MB of structure to 116 MB, and parsed + no slower. The define is ABI affecting and mixing it across translation units + is silent corruption, not a link error — which is why it rides on the imported + target (`odr` links pugixml PRIVATE, `odr_test` links it again) and why there + must be no prebuilt library to mismatch against. A new target that includes + `pugixml.hpp` has to get it too; `util/xml_util.cpp` asserts the layout it + compiled against. ## Releasing diff --git a/CMakeLists.txt b/CMakeLists.txt index ab129a0d5..bae33a274 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,6 +54,18 @@ endif () add_compile_options("$<$:/utf-8>") find_package(pugixml REQUIRED) +# A `xml_node_struct` costs 64 bytes by default and 12 in compact mode, which is +# what a document made of markup is mostly made of - the odf/ooxml models keep +# the dom resident and read attributes off it. Measured on a 297 MB content.xml: +# 594 MB of structure becomes 116 MB, and parsing is no slower. +# +# The define is ABI affecting and mixing a compact translation unit with a +# non-compact one is silent corruption, not a link error, so it goes on the +# imported target: `odr` links pugixml PRIVATE and `odr_test` links it again on +# its own, and both compile the headers themselves. `header_only=True` in +# `conanfile.py` is the other half - there is no prebuilt library to mismatch. +set_property(TARGET pugixml::pugixml APPEND PROPERTY + INTERFACE_COMPILE_DEFINITIONS PUGIXML_COMPACT) find_package(md4c REQUIRED) find_package(miniz REQUIRED) find_package(cryptopp REQUIRED) diff --git a/conanfile.py b/conanfile.py index 0b70df147..24b4ae166 100644 --- a/conanfile.py +++ b/conanfile.py @@ -38,6 +38,9 @@ class OpenDocumentCoreConan(ConanFile): "with_apple": False, "with_wasm": False, "bundle_assets": False, + # paired with PUGIXML_COMPACT in CMakeLists.txt: the define changes the + # node layout, so there must be no prebuilt library to mismatch against + "pugixml/*:header_only": True, } exports_sources = ["apple/*", "cli/*", "cmake/*", "jni/*", "python/*", "resources/dist/*", "wasm/*", "src/*", "CMakeLists.txt"] diff --git a/src/odr/internal/util/xml_util.cpp b/src/odr/internal/util/xml_util.cpp index a931e4ede..cf9ed8ca8 100644 --- a/src/odr/internal/util/xml_util.cpp +++ b/src/odr/internal/util/xml_util.cpp @@ -17,6 +17,14 @@ namespace odr::internal::util { +// `PUGIXML_COMPACT` is set on the imported target in CMakeLists.txt and changes +// the size of every node. A translation unit that misses it links fine and then +// reads the tree through the wrong layout, so assert what this one compiled +// against - it will not catch a *new* target that forgets the define, only the +// define going away. +static_assert(sizeof(pugi::xml_node_struct) == 12); +static_assert(sizeof(pugi::xml_attribute_struct) == 8); + pugi::xml_document xml::parse(const std::string &in) { pugi::xml_document result; if (const auto success = result.load_string(in.c_str()); !success) { From 91c281919b45000316ef43a85f0fa7aba9d79886 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 29 Aug 2026 20:00:26 +0200 Subject: [PATCH 2/6] fix(odf): store a repeated sheet cell once, not once per position 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) Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX --- src/odr/internal/odf/odf_parser.cpp | 97 ++++++------------- test/CMakeLists.txt | 2 + .../internal/odf/odf_sheet_repeat_test.cpp | 89 +++++++++++++++++ 3 files changed, 122 insertions(+), 66 deletions(-) create mode 100644 test/src/internal/odf/odf_sheet_repeat_test.cpp diff --git a/src/odr/internal/odf/odf_parser.cpp b/src/odr/internal/odf/odf_parser.cpp index 57f9c7ac7..eb94ef6bf 100644 --- a/src/odr/internal/odf/odf_parser.cpp +++ b/src/odr/internal/odf/odf_parser.cpp @@ -4,7 +4,6 @@ #include #include -#include #include #include @@ -118,7 +117,7 @@ parse_table(ElementRegistry ®istry, const pugi::xml_node node) { // TODO inflate table first? - for (const pugi::xml_node column_node : table_columns(node)) { + for_each_table_column(node, [&](const pugi::xml_node column_node) { const std::uint32_t repeat = column_node.attribute("table:number-columns-repeated").as_uint(1); for (std::uint32_t i = 0; i < repeat; ++i) { @@ -126,13 +125,13 @@ parse_table(ElementRegistry ®istry, const pugi::xml_node node) { auto [column_id, _] = parse_any_element_tree(registry, column_node); registry.append_column(element_id, column_id); } - } + }); - for (const pugi::xml_node row_node : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row_node) { // TODO log warning if repeated auto [row_id, _] = parse_any_element_tree(registry, row_node); registry.append_child(element_id, row_id); - } + }); return {element_id, node.next_sibling()}; } @@ -154,89 +153,55 @@ parse_sheet(ElementRegistry ®istry, const pugi::xml_node node) { TableCursor cursor; - for (const pugi::xml_node column_node : table_columns(node)) { + for_each_table_column(node, [&](const pugi::xml_node column_node) { const std::uint32_t columns_repeated = column_node.attribute("table:number-columns-repeated").as_uint(1); sheet.register_column(cursor.column(), columns_repeated, column_node); cursor.add_column(columns_repeated); - } + }); sheet.dimensions.columns = cursor.column(); cursor = {}; - for (const pugi::xml_node row_node : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row_node) { const std::uint32_t rows_repeated = row_node.attribute("table:number-rows-repeated").as_uint(1); sheet.register_row(cursor.row(), rows_repeated, row_node); // TODO covered cells - const bool row_empty = std::ranges::all_of( - row_node.children("table:table-cell"), is_cell_empty); - - if (row_empty) { - // TODO covered cells - for (const pugi::xml_node cell_node : - row_node.children("table:table-cell")) { - const std::uint32_t columns_repeated = - cell_node.attribute("table:number-columns-repeated").as_uint(1); - const std::uint32_t colspan = - cell_node.attribute("table:number-columns-spanned").as_uint(1); - const std::uint32_t rowspan = - cell_node.attribute("table:number-rows-spanned").as_uint(1); - - sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, - rows_repeated, cell_node, null_element_id); - - cursor.add_cell(colspan, rowspan, columns_repeated); + for (const pugi::xml_node cell_node : + row_node.children("table:table-cell")) { + const std::uint32_t columns_repeated = + cell_node.attribute("table:number-columns-repeated").as_uint(1); + const std::uint32_t colspan = + cell_node.attribute("table:number-columns-spanned").as_uint(1); + const std::uint32_t rowspan = + cell_node.attribute("table:number-rows-spanned").as_uint(1); + const bool is_repeated = columns_repeated > 1 || rows_repeated > 1; + + ElementIdentifier cell_id = null_element_id; + if (!is_cell_empty(cell_node)) { + const auto &[id, unused1, unused2] = registry.create_sheet_cell_element( + cell_node, cursor.position(), is_repeated); + cell_id = id; + registry.append_sheet_cell(element_id, cell_id); } - cursor.add_row(rows_repeated); - continue; - } + sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, + rows_repeated, cell_node, cell_id); - for (std::uint32_t row_repeat = 0; row_repeat < rows_repeated; - ++row_repeat) { - sheet.register_row(cursor.row(), 1, row_node); - - // TODO covered cells - for (const pugi::xml_node cell_node : - row_node.children("table:table-cell")) { - const std::uint32_t columns_repeated = - cell_node.attribute("table:number-columns-repeated").as_uint(1); - const std::uint32_t colspan = - cell_node.attribute("table:number-columns-spanned").as_uint(1); - const std::uint32_t rowspan = - cell_node.attribute("table:number-rows-spanned").as_uint(1); - const bool is_repeated = columns_repeated > 1 || rows_repeated > 1; - - if (is_cell_empty(cell_node)) { - sheet.register_cell(cursor.column(), cursor.row(), columns_repeated, - 1, cell_node, null_element_id); - - cursor.add_cell(colspan, rowspan, columns_repeated); - continue; - } - - for (std::uint32_t column_repeat = 0; column_repeat < columns_repeated; - ++column_repeat) { - const auto &[cell_id, unused1, unused2] = - registry.create_sheet_cell_element(cell_node, cursor.position(), - is_repeated); - registry.append_sheet_cell(element_id, cell_id); - sheet.register_cell(cursor.column(), cursor.row(), 1, 1, cell_node, - cell_id); - parse_any_element_children(registry, cell_id, cell_node); - - cursor.add_cell(colspan, rowspan, 1); - } + if (cell_id != null_element_id) { + parse_any_element_children(registry, cell_id, cell_node); } - cursor.add_row(1); + cursor.add_cell(colspan, rowspan, columns_repeated); } - } + + cursor.add_row(rows_repeated); + }); sheet.dimensions.rows = cursor.row(); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 3a896cf05..00a37bce1 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -72,6 +72,7 @@ add_executable(odr_test "src/internal/rtf/rtf_tokenizer_test.cpp" "src/internal/odf/odf_flat_file_test.cpp" + "src/internal/odf/odf_sheet_repeat_test.cpp" "src/internal/odf/odf_table_test.cpp" "src/internal/oldms/doc_test.cpp" @@ -81,6 +82,7 @@ add_executable(odr_test "src/internal/ooxml/ooxml_crypto_test.cpp" "src/internal/ooxml/ooxml_text_style_test.cpp" + "src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp" "src/internal/ooxml/ooxml_util_test.cpp" "src/internal/ooxml/ooxml_presentation_style_test.cpp" diff --git a/test/src/internal/odf/odf_sheet_repeat_test.cpp b/test/src/internal/odf/odf_sheet_repeat_test.cpp new file mode 100644 index 000000000..90c3f3eb6 --- /dev/null +++ b/test/src/internal/odf/odf_sheet_repeat_test.cpp @@ -0,0 +1,89 @@ +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +std::string flat_sheet(const std::string &body) { + return R"()" + R"()" + R"()" + R"()" + + body + + R"()"; +} + +/// One row repeated @p rows_repeated times holding one cell repeated +/// @p columns_repeated times, all of it non-empty. +std::string repeated_rows(const std::uint32_t rows_repeated, + const std::uint32_t columns_repeated) { + return R"()" + R"(x)"; +} + +std::shared_ptr document_of(const std::string &source) { + const std::unique_ptr file = + open_strategy::open_document_file(std::make_shared(source), + Logger::null()); + return file->document(); +} + +} // namespace + +/// A repeat is a range in the index, not a run of elements: both counts here +/// are legal, and expanding them would ask for three billion elements from a +/// document of four hundred bytes. +TEST(OdfSheetRepeat, a_repeated_cell_is_one_element) { + const std::string source = flat_sheet(repeated_rows(1048576, 1024)); + const std::shared_ptr held = document_of(source); + const auto *document = dynamic_cast(held.get()); + ASSERT_NE(document, nullptr); + + EXPECT_LT(document->element_registry().size(), 16); +} + +TEST(OdfSheetRepeat, a_repeated_cell_reads_at_every_position_it_covers) { + const std::string source = flat_sheet(repeated_rows(4, 3)); + const std::shared_ptr held = document_of(source); + + const odr::Document public_document(held); + const Sheet sheet = + (*public_document.root_element().children().begin()).as_sheet(); + + EXPECT_EQ(sheet.dimensions().rows, 4); + EXPECT_EQ(sheet.dimensions().columns, 3); + + for (std::uint32_t row = 0; row < 4; ++row) { + for (std::uint32_t column = 0; column < 3; ++column) { + const SheetCell cell = sheet.cell(column, row); + ASSERT_TRUE(cell) << column << "," << row; + const Element paragraph = *cell.children().begin(); + EXPECT_EQ((*paragraph.children().begin()).as_text().content(), "x"); + } + } +} From 9c9e108d9a79126764d364ad70fc9ac7c6cdd0b4 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 29 Aug 2026 20:00:43 +0200 Subject: [PATCH 3/6] perf(odf): index a sheet with sorted vectors, and hold elements in a 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) Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX --- src/odr/internal/odf/AGENTS.md | 34 ++++++-- src/odr/internal/odf/odf_document.cpp | 12 +-- src/odr/internal/odf/odf_element_registry.cpp | 78 +++++++++++++------ src/odr/internal/odf/odf_element_registry.hpp | 29 ++++++- src/odr/internal/odf/odf_table.cpp | 24 +++--- src/odr/internal/odf/odf_table.hpp | 16 ++-- test/src/internal/odf/odf_table_test.cpp | 24 ++++-- 7 files changed, 150 insertions(+), 67 deletions(-) diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 5092e886c..77554bdd9 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -19,13 +19,15 @@ Unlike the binary engines (`oldms/`, `pdf/`), which parse bytes into their own structures, ODF keeps the parsed **`content.xml` / `styles.xml` DOMs resident** (`Document` owns `m_content_xml`, `m_styles_xml`). The `ElementRegistry` is a thin *index over* that DOM: every `ElementRegistry::Element` stores a live -`pugi::xml_node` alongside its tree ids. Style/content/attribute access always +`pugi::xml_node` alongside its tree ids. The DOM being the backing store, and +not a parse artifact, is why pugixml is built in compact mode — see the top +level [`AGENTS.md`](../../../../AGENTS.md). Style/content/attribute access always goes back to the node. **This is why ODF alone can edit and save**: a text edit is a local DOM splice, and `save` re-serialises the mutated tree. The other engines throw away the source, so their models are read-only. -Everything else follows the shared registry/adapter pattern: flat -`std::vector`, id = index + 1, `null_element_id == 0`, parent/child/ +Everything else follows the shared registry/adapter pattern: a flat element +store, id = index + 1, `null_element_id == 0`, parent/child/ sibling ids, per-subtype side maps (`m_texts`, `m_tables`, `m_sheets`, `m_sheet_cells`). One mega `ElementAdapter` multiply-inherits every abstract per-type adapter and dispatches by returning `this`/`nullptr` on `element_type`. @@ -46,11 +48,27 @@ element and breaks the run. Reading expands `text:s`→N spaces (via `text:c`), `text:tab`→`\t`. **Sheets are modelled sparsely, off-tree.** A `Sheet` side-struct holds -position-keyed `columns`/`rows`/`cells` maps rather than a child chain. Repeated -columns/rows/cells are stored **once** at a range key and resolved with -`lookup_greater_than`, so a 5000-row `number-columns-repeated` doesn't inflate. -Only non-empty cells get a real `sheet_cell` Element; empty ones are recorded as -repeated ranges. Cells carry a `TablePosition` + `is_repeated` flag. +`columns`/`rows`/`cells` keyed by position rather than a child chain. Repeated +columns/rows/cells are stored **once**, at the *end* of the range they repeat +over, and resolved with an upper bound — so a 5000-row +`number-columns-repeated` costs one entry, whether or not the cell has content. +That last part is load-bearing: expanding a repeat per position let a +400-byte document ask for a `1048576 × 1024` grid of elements, both counts being +legal repeats. Only non-empty cells get a real `sheet_cell` Element; empty ones +are recorded as ranges alone. Cells carry a `TablePosition` (the anchor of the +range, not each position it covers) + `is_repeated` flag. + +The three containers are **sorted vectors, not maps**: parsing appends in +document order, so the keys only grow, and a rb-tree node costs more than the 12 +bytes of payload it carries — on a million-row sheet the index alone was 275 MB +of maps against 89 MB of vectors. The cells of every row live in one array per +sheet, each row recording where its own run starts, so a sheet is two +allocations rather than one per row. `register_cell` therefore has to follow the +`register_row` of the row it belongs to, and says so. + +The elements themselves are a `std::deque`: `create_element` hands back a +reference and the parser keeps parsing, and a vector both reallocates under that +reference and peaks holding two copies. **Styles resolve to a flattened `ResolvedStyle`, eagerly.** `StyleRegistry` first builds name→node indices from *both* files (automatic and named styles land diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index cb05d94eb..d425856b8 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -469,7 +469,7 @@ class ElementAdapter final : public abstract::ElementAdapter, TableDimensions result; TableCursor cursor; - for (const pugi::xml_node row : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row) { const auto rows_repeated = row.attribute("table:number-rows-repeated").as_uint(1); cursor.add_row(rows_repeated); @@ -492,7 +492,7 @@ class ElementAdapter final : public abstract::ElementAdapter, result.columns = new_cols; } } - } + }); return result; } @@ -731,20 +731,20 @@ class ElementAdapter final : public abstract::ElementAdapter, TableDimensions result; TableCursor cursor; - for (const pugi::xml_node column : table_columns(node)) { + for_each_table_column(node, [&](const pugi::xml_node column) { const auto columns_repeated = column.attribute("table:number-columns-repeated").as_uint(1); cursor.add_column(columns_repeated); - } + }); result.columns = cursor.column(); cursor = {}; - for (const pugi::xml_node row : table_rows(node)) { + for_each_table_row(node, [&](const pugi::xml_node row) { const auto rows_repeated = row.attribute("table:number-rows-repeated").as_uint(1); cursor.add_row(rows_repeated); - } + }); result.rows = cursor.row(); diff --git a/src/odr/internal/odf/odf_element_registry.cpp b/src/odr/internal/odf/odf_element_registry.cpp index 1ea0dbcf2..2bc72be2e 100644 --- a/src/odr/internal/odf/odf_element_registry.cpp +++ b/src/odr/internal/odf/odf_element_registry.cpp @@ -1,7 +1,6 @@ #include -#include - +#include #include namespace odr::internal::odf { @@ -246,13 +245,26 @@ void ElementRegistry::check_sheet_cell_id(const ElementIdentifier id) const { void ElementRegistry::Sheet::register_column(const std::uint32_t column, const std::uint32_t repeated, const pugi::xml_node element) { - columns[column + repeated] = {.node = element}; + const std::uint32_t end = column + repeated; + if (!columns.empty() && columns.back().end >= end) { + columns.back() = {.end = end, .node = element}; + return; + } + columns.push_back({.end = end, .node = element}); } void ElementRegistry::Sheet::register_row(const std::uint32_t row, const std::uint32_t repeated, const pugi::xml_node element) { - rows[row + repeated].node = element; + const std::uint32_t end = row + repeated; + if (!rows.empty() && rows.back().end >= end) { + rows.back().end = end; + rows.back().node = element; + return; + } + rows.push_back({.end = end, + .first_cell = static_cast(cells.size()), + .node = element}); } void ElementRegistry::Sheet::register_cell(const std::uint32_t column, @@ -261,40 +273,56 @@ void ElementRegistry::Sheet::register_cell(const std::uint32_t column, const std::uint32_t rows_repeated, const pugi::xml_node element, const ElementIdentifier element_id) { - Cell &cell = rows[row + rows_repeated].cells[column + columns_repeated]; - cell.node = element; - cell.element_id = element_id; + const std::uint32_t row_end = row + rows_repeated; + if (rows.empty() || rows.back().end != row_end) { + throw std::invalid_argument( + "ElementRegistry::Sheet::register_cell: no row to hold the cell"); + } + + const std::uint32_t end = column + columns_repeated; + if (cells.size() > rows.back().first_cell && cells.back().end >= end) { + cells.back() = {.end = end, .node = element, .element_id = element_id}; + return; + } + cells.push_back({.end = end, .node = element, .element_id = element_id}); } +namespace { + +/// The entry whose range covers @p at, i.e. the first one ending past it. +template +const Entry *lookup(const std::span entries, + const std::uint32_t at) { + const auto it = std::ranges::upper_bound(entries, at, {}, &Entry::end); + return it != std::end(entries) ? &*it : nullptr; +} + +} // namespace + const ElementRegistry::Sheet::Column * ElementRegistry::Sheet::column(const std::uint32_t column) const { - if (const auto it = util::map::lookup_greater_than(columns, column); - it != std::end(columns)) { - return &it->second; - } - return nullptr; + return lookup(columns, column); } const ElementRegistry::Sheet::Row * ElementRegistry::Sheet::row(const std::uint32_t row) const { - if (const auto it = util::map::lookup_greater_than(rows, row); - it != std::end(rows)) { - return &it->second; - } - return nullptr; + return lookup(rows, row); } const ElementRegistry::Sheet::Cell * ElementRegistry::Sheet::cell(const std::uint32_t column, const std::uint32_t row) const { - if (const Row *row_entry = this->row(row); row_entry != nullptr) { - const auto &cells = row_entry->cells; - if (const auto cell_it = util::map::lookup_greater_than(cells, column); - cell_it != std::end(cells)) { - return &cell_it->second; - } - } - return nullptr; + const Row *row_entry = this->row(row); + return row_entry != nullptr ? lookup(row_cells(*row_entry), column) + : nullptr; +} + +std::span +ElementRegistry::Sheet::row_cells(const Row &row) const { + const auto next = &row + 1; + const std::size_t end = + next != rows.data() + rows.size() ? next->first_cell : cells.size(); + return {cells.data() + row.first_cell, end - row.first_cell}; } [[nodiscard]] pugi::xml_node diff --git a/src/odr/internal/odf/odf_element_registry.hpp b/src/odr/internal/odf/odf_element_registry.hpp index 0c7515828..352d07ace 100644 --- a/src/odr/internal/odf/odf_element_registry.hpp +++ b/src/odr/internal/odf/odf_element_registry.hpp @@ -7,7 +7,9 @@ #include #include +#include #include +#include #include #include @@ -36,25 +38,35 @@ class ElementRegistry final { pugi::xml_node last; }; + /// Columns, rows and cells at the *end* of the range they repeat over, + /// sorted and resolved with an upper bound - a run of 5000 identical rows is + /// one entry, not 5000. Sorted vectors rather than maps: parsing appends in + /// document order, so the keys only ever grow, and a rb-tree node costs more + /// than the 12 bytes of payload it carries. The cells of every row live in + /// one array per sheet, each row holding where its own run starts. struct Sheet final { struct Column final { + std::uint32_t end{0}; pugi::xml_node node; }; struct Cell final { + std::uint32_t end{0}; pugi::xml_node node; ElementIdentifier element_id{null_element_id}; }; struct Row final { + std::uint32_t end{0}; + std::uint32_t first_cell{0}; pugi::xml_node node; - std::map cells; }; TableDimensions dimensions; - std::map columns; - std::map rows; + std::vector columns; + std::vector rows; + std::vector cells; ElementIdentifier first_shape_id{null_element_id}; ElementIdentifier last_shape_id{null_element_id}; @@ -63,6 +75,7 @@ class ElementRegistry final { pugi::xml_node element); void register_row(std::uint32_t row, std::uint32_t repeated, pugi::xml_node element); + /// Has to follow the @ref register_row of the row it belongs to. void register_cell(std::uint32_t column, std::uint32_t row, std::uint32_t columns_repeated, std::uint32_t rows_repeated, pugi::xml_node element, @@ -73,6 +86,9 @@ class ElementRegistry final { [[nodiscard]] const Cell *cell(std::uint32_t column, std::uint32_t row) const; + /// The cells of @p row, in column order. + [[nodiscard]] std::span row_cells(const Row &row) const; + [[nodiscard]] pugi::xml_node column_node(std::uint32_t column) const; [[nodiscard]] pugi::xml_node row_node(std::uint32_t row) const; [[nodiscard]] pugi::xml_node cell_node(std::uint32_t column, @@ -126,7 +142,12 @@ class ElementRegistry final { void append_sheet_cell(ElementIdentifier sheet_id, ElementIdentifier cell_id); private: - std::vector m_elements; + /// A deque, not a vector: `create_element` hands back a reference and the + /// parser keeps parsing, a million more elements deep. It also spares the + /// document the doubling - at three million elements a vector holds a third + /// more memory than it has elements, and reaches the peak holding both + /// halves. + std::deque m_elements; std::unordered_map m_texts; std::unordered_map m_tables; std::unordered_map m_sheets; diff --git a/src/odr/internal/odf/odf_table.cpp b/src/odr/internal/odf/odf_table.cpp index f08d12721..1efc62b3c 100644 --- a/src/odr/internal/odf/odf_table.cpp +++ b/src/odr/internal/odf/odf_table.cpp @@ -29,17 +29,17 @@ bool is_displayed(const pugi::xml_node group) { return !display || display.as_bool(true); } -void collect(const pugi::xml_node parent, const std::string_view name, - const std::span group_names, - std::vector &out) { +void walk(const pugi::xml_node parent, const std::string_view name, + const std::span group_names, + const TableNodeVisitor &visit) { for (const pugi::xml_node child : parent.children()) { const std::string_view child_name = child.name(); if (child_name == name) { - out.push_back(child); + visit(child); } else if (std::ranges::find(group_names, child_name) != std::end(group_names) && is_displayed(child)) { - collect(child, name, group_names, out); + walk(child, name, group_names, visit); } } } @@ -50,16 +50,14 @@ void collect(const pugi::xml_node parent, const std::string_view name, namespace odr::internal { -std::vector odf::table_rows(const pugi::xml_node table) { - std::vector result; - collect(table, "table:table-row", row_group_names, result); - return result; +void odf::for_each_table_row(const pugi::xml_node table, + const TableNodeVisitor &visit) { + walk(table, "table:table-row", row_group_names, visit); } -std::vector odf::table_columns(const pugi::xml_node table) { - std::vector result; - collect(table, "table:table-column", column_group_names, result); - return result; +void odf::for_each_table_column(const pugi::xml_node table, + const TableNodeVisitor &visit) { + walk(table, "table:table-column", column_group_names, visit); } } // namespace odr::internal diff --git a/src/odr/internal/odf/odf_table.hpp b/src/odr/internal/odf/odf_table.hpp index 18c10da2f..c35b7518f 100644 --- a/src/odr/internal/odf/odf_table.hpp +++ b/src/odr/internal/odf/odf_table.hpp @@ -1,6 +1,6 @@ #pragma once -#include +#include namespace pugi { class xml_node; @@ -8,11 +8,15 @@ class xml_node; namespace odr::internal::odf { -/// A table's rows in document order, including those a grouping element holds -/// - the `table:table-*-rows` family, which nests ([ODF 1.2] 9.1.7). -[[nodiscard]] std::vector table_rows(pugi::xml_node table); +using TableNodeVisitor = std::function; -/// The column counterpart of `table_rows()` ([ODF 1.2] 9.1.6). -[[nodiscard]] std::vector table_columns(pugi::xml_node table); +/// Calls @p visit for a table's rows in document order, including those a +/// grouping element holds - the `table:table-*-rows` family, which nests +/// ([ODF 1.2] 9.1.7). A visitor rather than a container: a sheet of a million +/// rows is walked more than once, and each walk would materialise them all. +void for_each_table_row(pugi::xml_node table, const TableNodeVisitor &visit); + +/// The column counterpart of @ref for_each_table_row ([ODF 1.2] 9.1.6). +void for_each_table_column(pugi::xml_node table, const TableNodeVisitor &visit); } // namespace odr::internal::odf diff --git a/test/src/internal/odf/odf_table_test.cpp b/test/src/internal/odf/odf_table_test.cpp index cf3869b55..a800be7e5 100644 --- a/test/src/internal/odf/odf_table_test.cpp +++ b/test/src/internal/odf/odf_table_test.cpp @@ -26,6 +26,20 @@ using namespace odr::internal::odf; namespace { +std::vector rows_of(const pugi::xml_node table) { + std::vector result; + for_each_table_row(table, + [&](const pugi::xml_node row) { result.push_back(row); }); + return result; +} + +std::vector columns_of(const pugi::xml_node table) { + std::vector result; + for_each_table_column( + table, [&](const pugi::xml_node column) { result.push_back(column); }); + return result; +} + std::vector names_of(const std::vector &nodes) { std::vector result; for (const pugi::xml_node node : nodes) { @@ -109,7 +123,7 @@ TEST(OdfTable, rows_directly_under_the_table) { )"); - EXPECT_EQ(names_of(table_rows(table)), (std::vector{"a", "b"})); + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"a", "b"})); } TEST(OdfTable, rows_inside_a_grouping_element) { @@ -124,7 +138,7 @@ TEST(OdfTable, rows_inside_a_grouping_element) { )"); - EXPECT_EQ(names_of(table_rows(table)), + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"header", "body"})); } @@ -143,7 +157,7 @@ TEST(OdfTable, row_groups_nest) { )"); - EXPECT_EQ(names_of(table_rows(table)), + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"a", "b", "c"})); } @@ -162,7 +176,7 @@ TEST(OdfTable, a_collapsed_group_is_not_shown) { )"); - EXPECT_EQ(names_of(table_rows(table)), (std::vector{"shown"})); + EXPECT_EQ(names_of(rows_of(table)), (std::vector{"shown"})); } TEST(OdfTable, columns_inside_a_grouping_element) { @@ -178,7 +192,7 @@ TEST(OdfTable, columns_inside_a_grouping_element) { )"); - EXPECT_EQ(names_of(table_columns(table)), + EXPECT_EQ(names_of(columns_of(table)), (std::vector{"a", "b", "c"})); } From aeaddd64fae0be2363709ccd90c8d452390740ce Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 29 Aug 2026 20:00:43 +0200 Subject: [PATCH 4/6] fix(ooxml): bound a merged range by the cells the sheet has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `` 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) Claude-Session: https://claude.ai/code/session_015NMKFPpDVw1xgS7BC2aeAX --- .../spreadsheet/ooxml_spreadsheet_parser.cpp | 17 +++ .../ooxml/ooxml_spreadsheet_merge_test.cpp | 108 ++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp index becf7edd1..6a615c4d6 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp @@ -149,6 +149,23 @@ parse_sheet_element(ElementRegistry ®istry, const ParseContext &context, TableDimensions(range.to().row - range.from().row + 1, range.to().column - range.from().column + 1); + // A `ref` may name any range the grid allows, and "A1:XFD1048576" is 17 + // billion positions in a file of a few hundred bytes - so once the range + // is wider than the sheet has cells, walk the cells instead, which bounds + // the work by what was actually read. + const std::uint64_t area = + static_cast(range.to().row - range.from().row + 1) * + (range.to().column - range.from().column + 1); + + if (area > sheet.cells.size()) { + for (const auto &[position, cell] : sheet.cells) { + if (position != range.from() && range.contains(position)) { + registry.sheet_cell_element_at(cell.element_id).is_covered = true; + } + } + continue; + } + for (std::uint32_t row = range.from().row; row <= range.to().row; ++row) { for (std::uint32_t column = range.from().column; column <= range.to().column; ++column) { diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp new file mode 100644 index 000000000..7dfc10da4 --- /dev/null +++ b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp @@ -0,0 +1,108 @@ +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +void insert(zip::ZipArchive &zip, const std::string &path, + const std::string &content) { + zip.insert_file(std::end(zip), RelPath(path), + std::make_shared(content)); +} + +/// The smallest workbook that opens: one sheet, whose `` and +/// `` are @p sheet_data and @p merge_cells. +std::shared_ptr workbook(const std::string &sheet_data, + const std::string &merge_cells) { + zip::ZipArchive zip; + insert( + zip, "[Content_Types].xml", + R"()" + R"()" + R"()" + R"()"); + insert( + zip, "_rels/.rels", + R"()" + R"()" + R"()"); + insert( + zip, "xl/workbook.xml", + R"()" + R"()"); + insert( + zip, "xl/_rels/workbook.xml.rels", + R"()" + R"()" + R"()"); + insert( + zip, "xl/styles.xml", + R"()"); + insert( + zip, "xl/worksheets/sheet1.xml", + R"()" + R"()" + + sheet_data + R"()" + merge_cells + R"()"); + + std::stringstream out; + zip.save(out); + return std::make_shared(out.str()); +} + +Sheet first_sheet(const Document &document) { + return (*document.root_element().children().begin()).as_sheet(); +} + +Document decode(const std::shared_ptr &file) { + return Document( + open_strategy::open_document_file(file, Logger::null())->document()); +} + +constexpr const char *two_cells = + R"(a)" + R"(b)"; + +} // namespace + +TEST(OoxmlSpreadsheetMerge, a_merge_covers_the_cells_it_spans) { + const Document document = decode(workbook( + two_cells, R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_EQ(sheet.cell(0, 0).span().columns, 2); + EXPECT_FALSE(sheet.cell(0, 0).is_covered()); + EXPECT_TRUE(sheet.cell(1, 0).is_covered()); +} + +/// A `ref` may name any range the grid allows, and the whole grid is 17 billion +/// positions - so the covered cells have to be found by walking what was read, +/// not by visiting every position the range names. +TEST(OoxmlSpreadsheetMerge, + a_merge_over_the_whole_grid_is_bounded_by_the_file) { + const Document document = decode( + workbook(two_cells, + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_FALSE(sheet.cell(0, 0).is_covered()); + EXPECT_TRUE(sheet.cell(1, 0).is_covered()); +} From d7fe6ed5a8ba1c957d63adf22b39005e37c6f037 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 08:39:48 +0200 Subject: [PATCH 5/6] fix(build): give a consumer the pugixml layout the library was built 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) Claude-Session: https://claude.ai/code/session_019d9ycFE78bvn43TyiH8Tv8 --- AGENTS.md | 16 ++++++++-------- CMakeLists.txt | 18 ++++++++---------- conanfile.py | 7 +++++-- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5c5eac257..d1c24923e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,14 +109,14 @@ cmake --build cmake-build-relwithdebinfo --target translate # CLI: file → HTM - **pugixml is built in compact mode** (`PUGIXML_COMPACT`, set on the imported target in `CMakeLists.txt`, paired with `pugixml/*:header_only` in `conanfile.py`). The odf/ooxml/svg/xml engines keep the parsed DOM resident as - their backing store, so its size *is* the document's: a 12-byte node instead of - 64 took a 297 MB `content.xml` from 594 MB of structure to 116 MB, and parsed - no slower. The define is ABI affecting and mixing it across translation units - is silent corruption, not a link error — which is why it rides on the imported - target (`odr` links pugixml PRIVATE, `odr_test` links it again) and why there - must be no prebuilt library to mismatch against. A new target that includes - `pugixml.hpp` has to get it too; `util/xml_util.cpp` asserts the layout it - compiled against. + their backing store, so its size *is* the document's: a 12-byte node instead + of 64 took a 297 MB `content.xml` from 594 MB to 116 MB. The define is ABI + affecting and mixing it across translation units is silent corruption, not a + link error — hence the imported target, and no prebuilt library to mismatch + against. A new target that includes `pugixml.hpp` has to get it too, and the + installed internal headers expose pugixml types, so `odr` carries the define + INTERFACE and `package_info` declares it. `util/xml_util.cpp` asserts the + layout it compiled against. ## Releasing diff --git a/CMakeLists.txt b/CMakeLists.txt index bae33a274..c822cfe34 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,16 +54,10 @@ endif () add_compile_options("$<$:/utf-8>") find_package(pugixml REQUIRED) -# A `xml_node_struct` costs 64 bytes by default and 12 in compact mode, which is -# what a document made of markup is mostly made of - the odf/ooxml models keep -# the dom resident and read attributes off it. Measured on a 297 MB content.xml: -# 594 MB of structure becomes 116 MB, and parsing is no slower. -# -# The define is ABI affecting and mixing a compact translation unit with a -# non-compact one is silent corruption, not a link error, so it goes on the -# imported target: `odr` links pugixml PRIVATE and `odr_test` links it again on -# its own, and both compile the headers themselves. `header_only=True` in -# `conanfile.py` is the other half - there is no prebuilt library to mismatch. +# A compact `xml_node_struct` is 12 bytes rather than 64, and the engines keep +# the dom resident as their backing store. The define is ABI affecting and a +# mismatch is silent, so it rides on the imported target and is paired with +# `header_only` in conanfile.py. See AGENTS.md. set_property(TARGET pugixml::pugixml APPEND PROPERTY INTERFACE_COMPILE_DEFINITIONS PUGIXML_COMPACT) find_package(md4c REQUIRED) @@ -323,6 +317,10 @@ target_link_libraries(odr uchardet::uchardet utf8::cpp ) +# pugixml is linked PRIVATE, but the installed internal headers expose its +# types - so a consumer of those needs the same layout. `cpp_info.defines` in +# conanfile.py is the same thing for the conan package. +target_compile_definitions(odr INTERFACE PUGIXML_COMPACT) if (ODR_WITH_HTTP_SERVER) find_package(httplib REQUIRED) diff --git a/conanfile.py b/conanfile.py index 24b4ae166..04d005b63 100644 --- a/conanfile.py +++ b/conanfile.py @@ -38,8 +38,8 @@ class OpenDocumentCoreConan(ConanFile): "with_apple": False, "with_wasm": False, "bundle_assets": False, - # paired with PUGIXML_COMPACT in CMakeLists.txt: the define changes the - # node layout, so there must be no prebuilt library to mismatch against + # paired with PUGIXML_COMPACT in CMakeLists.txt: no prebuilt library + # to mismatch against the node layout the define changes "pugixml/*:header_only": True, } @@ -106,3 +106,6 @@ def package(self): def package_info(self): self.cpp_info.libs = ["odr"] + # the installed internal headers expose pugixml types, whose layout + # PUGIXML_COMPACT changes + self.cpp_info.defines = ["PUGIXML_COMPACT"] From 442b3606a2bbdabc8799fe7d7a85551ef76b55be Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 08:40:01 +0200 Subject: [PATCH 6/6] docs(odf): cut the comments back, and write the changelog 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) Claude-Session: https://claude.ai/code/session_019d9ycFE78bvn43TyiH8Tv8 --- CHANGELOG.md | 14 +++++++++++ src/odr/internal/odf/AGENTS.md | 24 +++++++++---------- src/odr/internal/odf/odf_element_registry.hpp | 20 +++++++--------- src/odr/internal/odf/odf_parser.cpp | 2 ++ src/odr/internal/odf/odf_table.hpp | 4 ++-- .../spreadsheet/ooxml_spreadsheet_parser.cpp | 6 ++--- src/odr/internal/util/xml_util.cpp | 7 ++---- .../internal/odf/odf_sheet_repeat_test.cpp | 5 ++-- .../ooxml/ooxml_spreadsheet_merge_test.cpp | 5 ++-- 9 files changed, 45 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76061a0ae..e72af7e7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,20 @@ The release run heads these entries with the version and opens a fresh `text/html` and `application/xhtml+xml`. Classification only: no `open`, no `translate_html`, and never detected from its bytes. +- **Breaking** An ods sheet stores a repeated cell once, at the range it + covers, rather than once per position: `SheetCell::position()` reports the + anchor of that range instead of the position the cell was looked up at, and + `Sheet::cell()` hands back the same element everywhere in it. +- An ods `table:number-columns-repeated` beside a `table:number-rows-repeated` + no longer inflates — the grid's own `1048576 × 1024` asked for three billion + elements out of four hundred bytes. +- An xlsx `` whose `ref` names more positions than the sheet has + cells opens again; `ref="A1:XFD1048576"` used to visit all 17 billion. +- Decoding a spreadsheet costs about half the memory it did — pugixml is built + in compact mode and the ods sheet index is sorted vectors rather than maps. + A consumer that includes an internal header now needs `PUGIXML_COMPACT`, and + the conan package declares it. + ## v6.11.0 - 2026-08-29 - No view declares `` any more. A link back into what diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 77554bdd9..bfd23b336 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -50,25 +50,23 @@ element and breaks the run. Reading expands `text:s`→N spaces (via `text:c`), **Sheets are modelled sparsely, off-tree.** A `Sheet` side-struct holds `columns`/`rows`/`cells` keyed by position rather than a child chain. Repeated columns/rows/cells are stored **once**, at the *end* of the range they repeat -over, and resolved with an upper bound — so a 5000-row -`number-columns-repeated` costs one entry, whether or not the cell has content. -That last part is load-bearing: expanding a repeat per position let a -400-byte document ask for a `1048576 × 1024` grid of elements, both counts being -legal repeats. Only non-empty cells get a real `sheet_cell` Element; empty ones -are recorded as ranges alone. Cells carry a `TablePosition` (the anchor of the +over, and resolved with an upper bound — whether or not the cell has content. +That last part is load-bearing: expanding a repeat per position let a 400-byte +document ask for a `1048576 × 1024` grid of elements, both counts being legal +repeats. Only non-empty cells get a real `sheet_cell` Element; empty ones are +recorded as ranges alone. Cells carry a `TablePosition` (the anchor of the range, not each position it covers) + `is_repeated` flag. The three containers are **sorted vectors, not maps**: parsing appends in document order, so the keys only grow, and a rb-tree node costs more than the 12 -bytes of payload it carries — on a million-row sheet the index alone was 275 MB -of maps against 89 MB of vectors. The cells of every row live in one array per -sheet, each row recording where its own run starts, so a sheet is two -allocations rather than one per row. `register_cell` therefore has to follow the -`register_row` of the row it belongs to, and says so. +bytes it carries — on a million-row sheet, 275 MB of maps against 89 MB of +vectors. The cells of every row live in one array per sheet, each row recording +where its own run starts, so a sheet is two allocations rather than one per row. +`register_cell` therefore has to follow its row's `register_row`. The elements themselves are a `std::deque`: `create_element` hands back a -reference and the parser keeps parsing, and a vector both reallocates under that -reference and peaks holding two copies. +reference the parser holds on to, and a vector both invalidates it and peaks +holding two copies. **Styles resolve to a flattened `ResolvedStyle`, eagerly.** `StyleRegistry` first builds name→node indices from *both* files (automatic and named styles land diff --git a/src/odr/internal/odf/odf_element_registry.hpp b/src/odr/internal/odf/odf_element_registry.hpp index 352d07ace..d8358891f 100644 --- a/src/odr/internal/odf/odf_element_registry.hpp +++ b/src/odr/internal/odf/odf_element_registry.hpp @@ -38,12 +38,10 @@ class ElementRegistry final { pugi::xml_node last; }; - /// Columns, rows and cells at the *end* of the range they repeat over, - /// sorted and resolved with an upper bound - a run of 5000 identical rows is - /// one entry, not 5000. Sorted vectors rather than maps: parsing appends in - /// document order, so the keys only ever grow, and a rb-tree node costs more - /// than the 12 bytes of payload it carries. The cells of every row live in - /// one array per sheet, each row holding where its own run starts. + /// Columns, rows and cells keyed by the *end* of the range they repeat over + /// and resolved with an upper bound, so a run of 5000 is one entry. Sorted + /// vectors, not maps: parsing appends in document order. The cells of every + /// row live in one array per sheet, each row holding where its run starts. struct Sheet final { struct Column final { std::uint32_t end{0}; @@ -86,7 +84,7 @@ class ElementRegistry final { [[nodiscard]] const Cell *cell(std::uint32_t column, std::uint32_t row) const; - /// The cells of @p row, in column order. + /// The cells of @p row - one of this sheet's `rows` - in column order. [[nodiscard]] std::span row_cells(const Row &row) const; [[nodiscard]] pugi::xml_node column_node(std::uint32_t column) const; @@ -142,11 +140,9 @@ class ElementRegistry final { void append_sheet_cell(ElementIdentifier sheet_id, ElementIdentifier cell_id); private: - /// A deque, not a vector: `create_element` hands back a reference and the - /// parser keeps parsing, a million more elements deep. It also spares the - /// document the doubling - at three million elements a vector holds a third - /// more memory than it has elements, and reaches the peak holding both - /// halves. + /// A deque, not a vector: `create_element` hands back a reference the parser + /// holds on to, and a vector both invalidates it and peaks holding two + /// copies. std::deque m_elements; std::unordered_map m_texts; std::unordered_map m_tables; diff --git a/src/odr/internal/odf/odf_parser.cpp b/src/odr/internal/odf/odf_parser.cpp index eb94ef6bf..250d5eca2 100644 --- a/src/odr/internal/odf/odf_parser.cpp +++ b/src/odr/internal/odf/odf_parser.cpp @@ -200,6 +200,8 @@ parse_sheet(ElementRegistry ®istry, const pugi::xml_node node) { cursor.add_cell(colspan, rowspan, columns_repeated); } + // TODO a rowspan out of a repeated row is dropped - `add_row` clears the + // cursor's pending ranges for a repeat > 1 cursor.add_row(rows_repeated); }); diff --git a/src/odr/internal/odf/odf_table.hpp b/src/odr/internal/odf/odf_table.hpp index c35b7518f..20d3f470c 100644 --- a/src/odr/internal/odf/odf_table.hpp +++ b/src/odr/internal/odf/odf_table.hpp @@ -12,8 +12,8 @@ using TableNodeVisitor = std::function; /// Calls @p visit for a table's rows in document order, including those a /// grouping element holds - the `table:table-*-rows` family, which nests -/// ([ODF 1.2] 9.1.7). A visitor rather than a container: a sheet of a million -/// rows is walked more than once, and each walk would materialise them all. +/// ([ODF 1.2] 9.1.7). A visitor, not a container: a sheet is walked repeatedly +/// and each walk would materialise every row. void for_each_table_row(pugi::xml_node table, const TableNodeVisitor &visit); /// The column counterpart of @ref for_each_table_row ([ODF 1.2] 9.1.6). diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp index 6a615c4d6..c30d7fb6e 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp @@ -149,10 +149,8 @@ parse_sheet_element(ElementRegistry ®istry, const ParseContext &context, TableDimensions(range.to().row - range.from().row + 1, range.to().column - range.from().column + 1); - // A `ref` may name any range the grid allows, and "A1:XFD1048576" is 17 - // billion positions in a file of a few hundred bytes - so once the range - // is wider than the sheet has cells, walk the cells instead, which bounds - // the work by what was actually read. + // A `ref` may name the whole grid, 17 billion positions - so past the + // point where the range is bigger than the sheet has cells, walk the cells. const std::uint64_t area = static_cast(range.to().row - range.from().row + 1) * (range.to().column - range.from().column + 1); diff --git a/src/odr/internal/util/xml_util.cpp b/src/odr/internal/util/xml_util.cpp index cf9ed8ca8..074636260 100644 --- a/src/odr/internal/util/xml_util.cpp +++ b/src/odr/internal/util/xml_util.cpp @@ -17,11 +17,8 @@ namespace odr::internal::util { -// `PUGIXML_COMPACT` is set on the imported target in CMakeLists.txt and changes -// the size of every node. A translation unit that misses it links fine and then -// reads the tree through the wrong layout, so assert what this one compiled -// against - it will not catch a *new* target that forgets the define, only the -// define going away. +// `PUGIXML_COMPACT` (CMakeLists.txt) changes the size of every node, and a +// translation unit that misses it links fine and then reads the wrong layout. static_assert(sizeof(pugi::xml_node_struct) == 12); static_assert(sizeof(pugi::xml_attribute_struct) == 8); diff --git a/test/src/internal/odf/odf_sheet_repeat_test.cpp b/test/src/internal/odf/odf_sheet_repeat_test.cpp index 90c3f3eb6..d3cb872fb 100644 --- a/test/src/internal/odf/odf_sheet_repeat_test.cpp +++ b/test/src/internal/odf/odf_sheet_repeat_test.cpp @@ -55,9 +55,8 @@ std::shared_ptr document_of(const std::string &source) { } // namespace -/// A repeat is a range in the index, not a run of elements: both counts here -/// are legal, and expanding them would ask for three billion elements from a -/// document of four hundred bytes. +/// Both repeats are legal, and expanding them would ask for three billion +/// elements from four hundred bytes. TEST(OdfSheetRepeat, a_repeated_cell_is_one_element) { const std::string source = flat_sheet(repeated_rows(1048576, 1024)); const std::shared_ptr held = document_of(source); diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp index 7dfc10da4..fb037c9a2 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp @@ -93,9 +93,8 @@ TEST(OoxmlSpreadsheetMerge, a_merge_covers_the_cells_it_spans) { EXPECT_TRUE(sheet.cell(1, 0).is_covered()); } -/// A `ref` may name any range the grid allows, and the whole grid is 17 billion -/// positions - so the covered cells have to be found by walking what was read, -/// not by visiting every position the range names. +/// The whole grid is 17 billion positions, so the covered cells have to be +/// found by walking what was read. TEST(OoxmlSpreadsheetMerge, a_merge_over_the_whole_grid_is_bounded_by_the_file) { const Document document = decode(