Optimize BufferLine.copyFrom sparse map copying#6005
Open
Tyriar wants to merge 6 commits into
Open
Conversation
Skip O(cols) _copyCellMapsFrom loop when the source line has no sparse combined/extended map entries (scroll recycle from cached blank line). Copy sparse entries by map keys only when needed. Add CDP profiling script and BufferLine.copyFrom micro-benchmark. Co-authored-by: Daniel Imms <Tyriar@users.noreply.github.com>
Use shared empty-map sentinels with copy-on-write for scroll recycling, and an inlined _data flag scan for populated sparse maps (faster than per-cell function calls on master). Avoid slow Object.assign map copies. Co-authored-by: Daniel Imms <Tyriar@users.noreply.github.com>
Co-authored-by: Daniel Imms <Tyriar@users.noreply.github.com>
Member
Author
Member
Author
EMPTY_SPARSE_EXTENDED was shared across lines via copyFrom; addon-image writes _extendedAttrs directly without copy-on-write, which caused tile eviction failures and broke instanceof Object checks. Use undefined for empty extended maps (keep EMPTY_SPARSE_MAP for _combined). Ensure addon-image forks the map before writes and update the accessor test. Co-authored-by: Daniel Imms <Tyriar@users.noreply.github.com>
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.



Summary
Optimizes
BufferLine.copyFromfor the production scroll-recycle path (BufferService.scroll→recycle().copyFrom(cachedBlankLine)) and for lines with combining characters / extended attributes.Changes
_copySparseMapsFromEMPTY_SPARSE_MAPsentinel (Object.create(null)) with copy-on-write for_combined. Empty_extendedAttrsusesundefined(addon-image writes this map directly without BufferLine APIs)._dataflag scan instead of per-column_copyCellMapsFromcalls (fewer function call overheads than master).addons/addon-image: Fork_extendedAttrswith??= {}before writes; optional chaining on reads (fixes shared-map regression).test/benchmark/BufferLine.benchmark.ts: Micro-benchmarks for scroll recycle and sparse-sourcecopyFrom.copyFromis only called fromBufferService.scrollwhen the circular buffer is full.Micro-benchmark results
500,000
copyFromoperations per case, 5 runs, average runtime (npm run benchmark -- -s "out-test/benchmark/BufferLine.benchmark.js" out-test/benchmark/BufferLine.benchmark.js). Benchmark file from this PR; onlyBufferLine.tsswapped per revision. Measured atdec5452fc.Three-way comparison (
ddf2b7dvsmastervs this PR)ddf2b7dis the last merge onmasterbefore #5972 (fix(buffer): clear stale attrs and combined data in BufferLine), which replacedcopyFrom's key-only map copy with an O(cols)_copySparseMapsFromscan.What each revision does in
copyFromyes/ scroll recycle)for…inover empty_combined/_extendedAttrs+ 2×{}allocfor…inover map keys only_copyCellMapsFromloop every scroll_combinedsentinel identity check (O(1));_extendedAttrs = undefinedTakeaways
CDP results (
timeout 1s yeson demo terminal)Chromium CDP CPU profile, cols=140, rows=38, scrollback=1000, ~2.76s sample window. Attributed time for copy-path symbols (note: V8 often does not nest
copyFromwith_copyCellMapsFromon the same stack frame, so compare the combined copy-path total).copyFrom_copySparseMapsFrom_copyCellMapsFromCopy-path combined time: −75% vs master.
_copyCellMapsFromsamples eliminated on the blank-line scroll path.Design notes (alternatives explored)
undefinedfor_combined: Regressed sparsecopyFromin benchmarks; not used for_combined.undefinedfor_extendedAttrs: Adopted (addon-image writes maps directly; shared sentinel caused integration test failures).Object.freezeon sentinels: Safe with copy-on-write but no measurable perf gain; not adopted.Testing
npm run test-unit -- **/out-esbuild/**/BufferLine.test.js— 51 passingnpm run test-integration -- --suite=addon-image— 209 passingnpm run lint-changes