perf(xlsx): index merged ranges per frame - #243
Conversation
|
All contributors have signed the CLA — thank you! ✍️ ✅ Posted by the CLA bot. |
Greptile SummaryThe PR replaces per-cell linear merge and hyperlink scans with per-frame interval-stabbing indexes while retaining original vector-order precedence.
Confidence Score: 4/5The PR appears safe to merge after the non-blocking comment-concision issue is addressed or accepted. The indexed lookups preserve original vector-order precedence, viewport filtering remains valid for every queried visible anchor, and no blocking rendering or security failure remains. Files Needing Attention: crates/xlsx-render/src/lib.rs
|
| Filename | Overview |
|---|---|
| crates/xlsx-render/src/lib.rs | Introduces per-frame interval indexes and extensive behavioral/performance tests; lookup semantics appear preserved, with only non-blocking comment-concision feedback. |
| .changeset/xlsx-render-merge-index.md | Adds an appropriate patch changeset describing the XLSX rendering optimization. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
S[Worksheet merges and hyperlinks] --> M[Build MergeIndex]
S --> H[Build viewport-filtered HyperlinkIndex]
M --> A[Filter visible cell anchors]
M --> G[Resolve merged cell geometry]
M --> B[Render borders]
H --> T[Resolve hyperlink styling and text]
A --> D[Display list]
G --> D
B --> D
T --> D
Reviews (1): Last reviewed commit: "perf(xlsx): index merges and hyperlinks ..." | Re-trigger Greptile
| } | ||
|
|
There was a problem hiding this comment.
Trim redundant index docstrings
The new index and test-hook docstrings repeat implementation details already apparent from the code rather than remaining extremely concise, increasing maintenance surface and the chance that commentary drifts from behavior. The same pattern appears on HyperlinkIndex and contains_probe.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
6e30d8a to
217a965
Compare
5ac7786 to
62ff5a1
Compare
Every painted cell resolved its merge by scanning the sheet's whole merge vector, once per cell per paint pass. A per-frame index now sorts the ranges by start row alongside a running maximum of end rows, so a lookup only examines ranges whose rows can reach the probe. Sorting costs about n log n, which a frame with few merge lookups never earns back, so lookups keep scanning linearly until they have spent that much and the sort has paid for itself. A frame that paints nothing therefore sorts nothing. Once sorted, a sheet-order walk runs alongside the stab, one step per stabbed entry, so a lookup never takes more than twice the scan's containment checks even when the sort pushes the winner to the far end of the window. First-match semantics are unchanged. Co-Authored-By: codex <codex@openai.com>
62ff5a1 to
25f1226
Compare
|
Closing. Scope reduction got this a long way but did not get it to a clean win. Dropping the hyperlink index removed every loss that revision carried (0.65x, 0.74x, 0.80x all became parity) and left large wins: 33x on 5,000 clustered merges, 3.0-3.4x on 5,000 merges spread one per row, 2.1x on a full-height merge, 1.08x on an ordinary 8-merge sheet. Making the sort pay for itself before it happens then removed the sparse regressions the eager version had (+3.4 ms/frame at 100,000 merges with nothing painted, +0.24 ms at 10,000 merges with one anchor). What is left is a shape that is ordinary rather than adversarial. On a sheet with 100,000 merges not listed in row order, 16 visible cells run at 11.1 ms/frame against main's 19.3 ms, and 17 visible cells run at 37.3 ms against main's 19.9 ms - 0.52x. The seventeenth lookup spends the last of the probe budget, pays for the sort, and the frame ends with nothing left to amortize it over. It is not a knife-edge: the frame stays slower from roughly 17 to 40 visible cells, and it repeats every frame because the index is per-frame. Raising the budget moves the cliff and makes the wasted work bigger; the sort/don't-sort decision has to be made mid-frame without knowing how many lookups remain, so some frame always lands wrong. Measurements are interleaved, min-of-N, at 1-minute load 7.0. One thing worth taking from this separately: about 1.3-1.6x of the branch's gain on scan-heavy sheets is not the index at all. Adding nothing but a no-op wrapper function around Full numbers, the differential result (38,400 frames hash-identical to main on both lookup paths) and the two withdrawn claims from the original description are in the PR body. |
TL;DR:
Summary:
covering_mergewasmerges.iter().find(|m| m.contains(at))— a linear scan of the sheet's whole merge vector, run once per painted cell in every paint pass, fromvisible_anchors,cell_boxandemit_bordersalike. This branch resolved merges through a per-frame index instead: entries sorted by start row alongside a running maximum of end rows, so a lookup only examined ranges whose rows could reach the probe.Sheet::hyperlink_atkept its original linearfind. That removed the 0.65x, 0.74x and 0.80x rows the earlier revision carried.n log ncontainment checks, and only then is the sort done. That removed both sparse regressions.mainon both lookup paths, with overlapping, duplicated, inverted, whole-column, whole-row and whole-sheet ranges under frozen panes.Closing this without merging. The wins are large and real, but a loss column survived every round of scope reduction, and the last one is on an ordinary shape rather than an adversarial one.
Measured interleaved, min-of-N,
mainand branch binaries alternated within each run, 1 thread, 10-core M-series. Ratios are main / branch, so above 1.00 is faster.The last two rows are the blocker. One additional visible cell takes that frame from 11.1 ms to 37.3 ms while
maingoes from 19.3 ms to 19.9 ms: the sixteenth lookup spends the last of the probe budget, the seventeenth pays for a 100,000-entry sort, and the frame then ends with nothing left to amortize it over. The regression is not a knife-edge — it spans roughly 17 to 40 visible cells on this shape before the sort starts paying for itself again, and every frame repeats it because the index is per-frame. Raising the budget only moves the cliff and makes the wasted work larger in absolute terms; the decision of whether to sort has to be made mid-frame without knowing how many lookups are still coming, so some frame always lands on the wrong side of it.Two other things worth recording:
contains_probewrapper aroundCellRange::containstomain— no index, same algorithm, identical display lists — makesmain1.57x faster on the bottom-parked shape, 1.65x on the clustered-merge shape and 1.33x on the spread shape. Under this workspace'sopt-level = "s"the wrapper flips an inlining decision in the scan loop. That is a one-line change worth having on its own.mainthat already has that wrapper, the index itself is 20.2x on the clustered-merge shape and 2.32x on the spread shape, but 0.79x on the bottom-parked shape — the2mcontainment-check bound being hit, because one full-height merge poisons the running maximum, the stab window becomes the whole set, and the sheet-order walk doubles the checks. The walk is what buys the bound, so that cost is not removable without reintroducing an unbounded worst case.A start-row-sorted stab window cannot close either shape; both want a structure that prunes on columns as well as rows and that is not rebuilt per frame. That is a different change from this one.
Test plan:
cargo test -p betteroffice-xlsx-rendergreen (50 unit + 10 integration)cargo test --workspacegreen (1,719 tests, 0 failures)cargo clippy --workspace --all-targets --all-features -- -D warningsandcargo fmt --all -- --checkcleanmainover 38,400 randomized frames, on both the scan path and the stab path