Skip to content

fix(xlsx) Caching of formats, fonts, fills and borders on Excel rendering#190

Merged
goworm merged 2 commits into
iOfficeAI:mainfrom
andulv:plan001-excel-html-style-index
Jul 10, 2026
Merged

fix(xlsx) Caching of formats, fonts, fills and borders on Excel rendering#190
goworm merged 2 commits into
iOfficeAI:mainfrom
andulv:plan001-excel-html-style-index

Conversation

@andulv

@andulv andulv commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

I noticed slow rendering of some machine generated xlsx files where style was set for each cell.
This PR adds caching to speed up rendering of such files.

File before (main) after (fix) speedup output
Synthetic (cellXfs=16000, 1000×28) 36.7s min / 54.8s med 0.36s min / 0.43s med ~100× byte-identical
Real trigger file (Mal til AI …(1).xlsx) 9.3s min / 9.5s med 0.69s min / 0.77s med ~13× byte-identical
# Before the fix (main):
bash bench/excel-html-stylesheet/bench.sh --cellxfs 16000 --rows 1000 --cols 28 --runs 3
# → min_seconds=36.734  median_seconds=54.767

# After the fix:
bash bench/excel-html-stylesheet/bench.sh --cellxfs 16000 --rows 1000 --cols 28 --runs 3
# → min_seconds=0.361  median_seconds=0.431   (~100× faster)

# Output is byte-identical (same file rendered on main vs fix → diff is empty):
diff <(git stash && dotnet build ... && officecli view f.xlsx html && git stash pop) ...

@goworm

goworm commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Thanks — the diagnosis is right (the SDK's Elements<T>().Count() / .ElementAt(i) are O(N) live walks, so style-bloated workbooks render at O(cells × styles)), and we verified the speedup and want this in. Two changes needed before merge:

1. The cache must be scoped to a single render, not static.

ConditionalWeakTable keyed on the collection instance outlives the render. officecli normally runs mutations through a long-lived resident process that keeps one document DOM alive across commands, and style writes append new Font/CellFormat entries to the same Fonts/CellFormats instances — so the cached arrays go stale and the next HTML render silently drops the new styles. Reproduced on this branch (green on main, red here):

// same live ExcelHandler instance throughout (resident-server shape)
excel.Set("/Sheet1/A1", new() { ["font.italic"] = "true" });
var first = excel.ViewAsHtml();          // caches the stylesheet arrays
excel.Set("/Sheet1/B1", new() { ["font.bold"] = "true", ["fill"] = "#FF8800" });
var second = excel.ViewAsHtml();
// FAILS: second contains no "font-weight:bold" — the appended xf/font
// is invisible because the cached arrays predate the mutation.

Fix: materialize the four arrays once per render (e.g. build them at the ViewAsHtml entry / store them on SheetRenderContext) instead of the static ConditionalWeakTable. The hot cost you measured is the per-cell re-walk within one render, so per-render scope keeps the full speedup and the staleness problem disappears structurally.

2. Please trim the PR to src/officecli/Handlers/Excel/ExcelHandler.HtmlPreview.cs only.

  • CHANGELOG.md no longer exists in the repo — please rebase onto current main and drop it.
  • The officecli.csproj version bump conflicts with main; versioning is handled on the maintainer side.
  • We won't take bench/ into the tree (repro/bench tooling stays local by convention) — the PR description already documents the methodology, which is what matters. FWIW bench.sh is also Linux-only as written: mapfile needs bash ≥ 4 and BSD date has no %N, so it doesn't run on macOS.

We'll add regression tests for the style-lookup fast path on our side when this lands.

@andulv andulv force-pushed the plan001-excel-html-style-index branch from d51a49c to 2046519 Compare July 10, 2026 09:32
@andulv

andulv commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

I have now rebased and reapplied the fix.

@goworm goworm merged commit cac644c into iOfficeAI:main Jul 10, 2026
@andulv andulv deleted the plan001-excel-html-style-index branch July 10, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants