Repository navigation
replace _combined map and string cache with _chars variable in LogicalLine - #6160
PerBothner wants to merge 52 commits into
Conversation
Instead to change isWrapped state use a Buffer.setWrapped method. This allows for potential flexibility in how BufferLine and line-wrapping are implemented.
A BufferLine is now the sub-range of a LogicalLine for a specific visible line, while LogicalLine is independent of window width.
This is used by InputHandler.print to "batch" multiple characters.
This typically reduces memory usage, allocation, and copying.
Also restored CircularList.recycle, pending possible future use - though the initial attempt actually slowed things down.
Seems slightly faster to recycle BufferLine but not LogicalLine.
The image addon now builds but has some problems still.
I think this is slightly cleaner and possibly faster, since no cloning needed.
(Still a good chunk to go.)
The CellData_string field is now only valid if STORED_IN_CHARS_MASK is set. We don't cache the result of getChars, since then we would need to invalidate the cache, which would require more code changes.
… addon-search The only time the argument is non-true is in one test-case. This removes one place where translateToString is called with trimRight false/
Added new offsetToString function in LogicalLine. Also, by-pass public API to avoid allocating BufferLineApiView.
Instead, have BufferLine implement IBufferApi by adding a getLine method.
Also, add startColumn to IBufferLine.
It is no longer needed: We just use the asString method from ILogicalLine. That method now caches the result inexpensively.
|
Using this change I was able to optimize and simplify the Further improvements are probably feasible. For example, I suspect the This change has been checked into the |
This fixes a testcase with a wide char shifted right.
Combine reflowLargerCreateNewLayout and reflowLargerApplyNewLayout into a single _reflowLargerNewLayout function. This avoids an extra pass over the lines and an extra array allocation. Also, merge in the rest of BufferReflow.ts into Buffer.ts - it was weird having some of the reflow logic in Buffer.ts and some in a separate module.
The LogicalLine implementation does not support trimmable (final) null characters with attributes (except background), since that can never be created using escape sequences,
Add optional attrs parameter. Tweak handling of no pre-existing character in cell.
Fixes last failing unit test.
This reduces risk of confusion with BufferLine length property.
PerBothner
changed the title
|
I removed the "WIP" (work-in-progress) marker. All the tests pass and the functionality appears to be complete. The search addon has been simplified and optimized greatly. Performance-wise (with the simple |
|
Sorry for creating a such a large change-set to review, but given the lack of reviews and feedback I have no choice but to combine all my "recommended changes" into a single large PR: I cannot juggle and keep updated a large number of change-sets, some of which are independent and some of which depend on each other. I do have some smaller PRs that have been merged into this PR. I will keep them open, but I will not be keeping them updated or resolve conflicts unless explicitly requested. |
|
Summary of status of changes:
I will write up some performance notes later. |
1 participant
[This is a work-in-progress - there are a number of testsuite failures that I haven't debugged yet.]
This is an unfinished implementation of the
_charsproposal from this comment, which is copied/edited below.This builds on the
LogicalLinePR #5797.Quick summary: add a new field
_charstoLogicalLinewhich replaces both the_combinedmap and the string cache.The idea is that each cell is in one of two modes (see the comment before enum Content in common/buffer/Constants.ts): Either the existing way (a codepoint as a 21-bit value in the _data array), or as substring of the
_charsstring. SeegetStringinLogicalLine. All combined characters would use the latter variant. But so do all other characters after translateToString is called, which also sets the _charsIsTextValue of the LogicalLine. If that property is true, all cells use the substring-of-_chars representations and all the substrings are in order. Thus translateToString can trivially and safely return the _chars string. If translateToString is called when _charsIsTextValue is false, we do basically the same computation as the current implementation (using a StringBuilder). So _chars functions as a cache for translateToString, and _charsIsTextValue indicates if the cache is valid. There is no need to trim the string cache because the amount of space for the _chars string is quite modest and I believe more than made up by removing the _combined map and StringCacheEntry, simpler data structures, simpler gc (no need for a bunch of expensive WeakRefs), and better memory locality.@jerch mentioned earlier a concern that using a string for cell character data is very expensive in the presense of updates. But this hybrid approach avoids that issue: We generally don't update the _chars string during normal writes. (A combined characher is handled by just appending at the end of _chars.) However, the _chars array gets normalized when the line is rendered or translateToString is called for other reasons.