Stop building a million nodes for a million-element array - #53
Merged
Merged
Conversation
A dictionary holding a 1000x1000 double did not open. The parse was never the problem — it took a second — but the ingest then built one node per element: 1,000,001 nodes and ~690 MB for that one entry, whose labels alone took ~161 s, and a consumer's table projection of the subtree was ~413 MB of rows. Two causes, both here. An array past MAX_EXPANDED_ELEMENTS (10,000) now expands into NO element children. The cap has to clear a consumer's own grid cap — the vscode extension grids a matrix only when it has one child per element, at most 4096 — so the two limits compose rather than cancel: <=4096 gets rows and a grid, <=10,000 gets rows, past that the summary alone. It is all-or-nothing by necessity: `_elements` and the children are two copies of one value and every reader spells the choice `children.length > 0 ? children.map(...) : _elements`, so a partial expansion would be read through its children and would save the first N elements as the whole value. Checked on the file that prompted this: the capped entry serializes back byte-identical to the 4 MB of value text the file itself holds, all 1,000,000 elements, and still summarizes as `<1000x1000 double>`. Not a ParseWarning, by ParseWarning's own rule — nothing was lost to report. Cells are exempt, and say why at the builder: a cell's children are its ONLY copy, so a cap there would make _serializeCellXml write `Dimension="0*0"`. The cap also needed somewhere to live. Six parse paths wrote their own element loop, with the `length > 1` guard spelled three times and missing three times; the comment at one of them already said every element builder should state the rule identically, but said it by convention, which lasted exactly until there were two rules to agree on. All six now call _buildArrayChildren, the only place an array grows children, with an element-class override for the one container whose elements are not of its own class. Second cause: an element's label derived its subscript from `parent.children.indexOf(this)`, O(n) per element and so O(n^2) per array — 1.5 us at index 0, 325 us at index 999,999. The struct-element path eight lines below already read a stored index and was 2000x faster on the same data. Elements now read their slot from the 1-based name every builder stamps, and VERIFY it holds this node before trusting it, falling back to the scan when children have been reordered without reindexing. Both halves are pinned, the fast one by a scan count rather than a clock. Ingest of that file: 1,001,276 nodes to 1,276, 690 MB to 95 MB, 288 ms total.
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.
A
.slddholding a 1000x1000 double did not open in the VS Code extension: blank tab, extension host pinned for ~2.5 minutes, then a misleadingFailed to parse ...: Maximum call stack size exceeded. The parse was never the problem — it took one second. The ingest then built one node per element.Measured on that file, before: 1,001,276 nodes, ~690 MB for the single entry, ~161 s in element labels alone, and a consumer's table projection of ~413 MB of rows. After: 1,276 nodes, 95 MB, 288 ms.
Two causes, both in this repo
1. Unbounded expansion. An array past the new
MAX_EXPANDED_ELEMENTS(10,000) expands into no element children; the value lives in_elements, which every reader already falls back to.children.length === elementCount, at most 4,096. A cap at or below that would silently kill the grid for exactly the matrices it exists for. Above it the two limits compose —<=4096rows + grid,<=10,000rows, past that the summary alone._elementsand the children are two copies of one value, and every reader spells the choicechildren.length > 0 ? children.map(...) : _elements. A partial expansion would be read through its children and would save the first N elements as the whole value.<1000x1000 double>.ParseWarning, byParseWarning's own rule: the value was read and is saved completely, so nothing was lost to report._serializeCellXmlwriteDimension="0*0".2. Quadratic element labels.
BaseNode.displayNamederived an element's subscript fromparent.children.indexOf(this)— O(n) per element, O(n^2) per array: 1.5 us at index 0 rising to 325 us at index 999,999. The struct-element path eight lines below already read a stored index and was 2000x faster on the same data. Elements now read their slot from the 1-basednameevery builder stamps and verify it holds this node before trusting it, falling back to the scan when children were reordered without reindexing.The cap needed one home
Six parse paths wrote their own element loop, with the
length > 1guard spelled three times and missing three times. One of them already carried a comment saying every element builder should state the rule identically — by convention, which held right up until there were two rules to agree on, and a cap honoured by five builders out of six is not a cap. All six now call_buildArrayChildren, with an element-class override for the one container whose elements are not of its own class (a complex array stores_scalarTypedouble; its elements arecomplex).Tests
test/largeArrayNotExpanded.test.ts, 10 tests: the cap clears 4,096; past it no children;Value,serializeXmlanddisplayValuestay complete (noDimension="0*0"); the boundary is inclusive; a cell past the cap still expands; labels correct at a high index in a non-square array and when a child sits at a slot its name does not predict; and the fast path does no scan — asserted as a count, not a clock.npm run verifygreen: 5,122 tests, plus typecheck, build, pack, leak and browser-safety checks.No consumer change is required by this PR. The host-side
rows.push(...)argument overflow that produced the user-visible message is a separate fix in the extension repo.