Skip to content

Improve decoration cell lookup performance#5886

Closed
tt-a1i wants to merge 1 commit into
xtermjs:masterfrom
tt-a1i:codex/perf-decoration-cell-lookup
Closed

Improve decoration cell lookup performance#5886
tt-a1i wants to merge 1 commit into
xtermjs:masterfrom
tt-a1i:codex/perf-decoration-cell-lookup

Conversation

@tt-a1i

@tt-a1i tt-a1i commented May 20, 2026

Copy link
Copy Markdown

Summary

Refs #5176.

This speeds up decoration lookup for the common single-line decoration case. When no registered decoration spans multiple rows, forEachDecorationAtCell now asks the sorted decoration list for the current line instead of scanning every decoration for every cell.

Multi-line decorations continue to use the existing full scan path so their behavior is unchanged.

Validation

  • npm run build && npm run esbuild
  • npm run test-unit -- out-esbuild/common/services/DecorationService.test.js out-esbuild/common/SortedList.test.js
  • npm run lint-changes

Notes

A local micro-benchmark with 10k single-line decorations over an 80x24 viewport for 20 iterations went from about 2479ms on the full-scan path to about 6.9ms with the line-keyed fast path.

@tt-a1i tt-a1i marked this pull request as ready for review May 20, 2026 13:54
@PerBothner

Copy link
Copy Markdown
Contributor

I believe the changes in PR #5853 (as also discussed in issue #5176) may be more general. It would be much appreciated if you could try them out.

@tt-a1i

tt-a1i commented May 24, 2026

Copy link
Copy Markdown
Author

Thanks Per, I tried the latest #5853 (3dbd370) this morning.

I don't have the full search-addon Playwright harness polished yet, so I started with a small synthetic benchmark that isolates the renderer-side hot path: 100k single-line decorations, scan a 30x80 visible grid, medians of 7 runs after 2 warmups.

branch registration + iterator flush visible-grid decoration lookup
master (e749cb6) 121 ms 7962 ms via forEachDecorationAtCell
this PR (a6b7960) 97 ms 0.276 ms via forEachDecorationAtCell
#5853 latest (3dbd370) 102 ms 0.270 ms via forEachDecorationAtCellLine

So yes, with the latest Marker.line / linked-list changes, #5853's new line-aware path does seem to solve the same renderer-side problem in the more general direction. The old forEachDecorationAtCell path on #5853 is still linear in this microbench, but the DOM/WebGL renderers use forEachDecorationAtCellLine on that branch, which is the important bit for scrolling/rendering.

My read is: #5886 is still a tiny master-compatible stopgap for the current API; #5853 looks like the broader fix once that larger buffer/marker refactor is ready. I'm happy either way: keep this PR scoped as the small interim path, or close it if maintainers would rather focus review on #5853.

@Tyriar

Tyriar commented May 26, 2026

Copy link
Copy Markdown
Member

Thanks for the PR! I looked at this earlier and it rubbed me the wrong way that there are essentially fast and slow modes, where a single multi-line decoration would make everything slow from them on. This was inspiration for #5902 which solves the problem in a more general way

@Tyriar Tyriar closed this May 26, 2026
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.

3 participants