Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix decorations on DOM renderer#4651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Tyriar merged 3 commits intoxtermjs:masterfromjerch:fix_4642
Aug 9, 2023
Merged

Conversation

@jerch
Copy link
Member

Fixes#4642.

Comment on lines +134 to +137
letisDecorated=false;
this._decorationService.forEachDecorationAtCell(x,row,undefined,d=>{
isDecorated=true;
});
Copy link
MemberAuthor

@jerchjerchAug 9, 2023
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@Tyriar Is there a better way to determine, if a cell is decorated? This callback method runs several times for all matched decorations, while all I need is a.hasDecoration(x,y) boolean indicator here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We can add ahasDecoration. A faster way currently would be usinggetDecorationsAtCell and break after the first one, that needs a generator created though.

jerch reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hmm indeed, the generator has worse runtime, even if I break out of the iteration early 😢

Copy link
MemberAuthor

@jerchjerchAug 9, 2023
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@Tyriar I was not able to find a faster access for ahasDecoration method. Tried different things like early exit fromforEachDecorationAtCell, using an iterable instead of a generator, even tried returning an array of decorations to save the second iteration. All beside the iterable perform slightly better if there are no decorations, but the difference is only ~20ms vs. ~1ms over 14s total runtime. The picture changes, if there are decorations - thenforEachDecorationAtCell beats them all being at least 20% faster. Thus I think it is good as it is atm.

The toxic runtime part is the binary search across lines, plus the a linear search within the line, some numbers from myls -lR /usr test (tested with DOM renderer):

  • 0 highlighted matches: ~20 ms forforEachDecorationAtCell
  • 100 matches: ~130 ms forforEachDecorationAtCell
  • 1000 matches: ~500 ms forforEachDecorationAtCell

with ~80% spent in_search (binary search part) and the rest inforEachDecorationAtCell (linear search part). Which makes me wonder, if the binary search part could be omitted by another indirection on the list with O(1) access to the line values.

Btw I was not able to test higher matches, seems the highlighting is capped in the search addon.

Edit: On a sidenote - it might already help to reshape _search into a loop instead of recursive calls.
Edit2: Yeah - changing _search into a loop reduces runtime from 500 ms to 150 ms for 1000 matches (also v8 folds _search intoforEachDecorationAtCell, so I cant separate binary vs. linear search cost anymore)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Any perf improvements are welcome of course, but I wouldn't worry too much about squeezing a lot out of looking up decorations as they are normally not used at all or used sparingly. VS Code only uses it with find (which performs good enough with many results), highlighting things temporarily and adding decorations next to commands for shell integration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice work if a_search refactor speeds things up that much!

Copy link
MemberAuthor

@jerchjerchAug 9, 2023
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The loop version shows these runtimes:

  • 0 matches: ~1 ms
  • 100 matches: ~40 ms
  • 1000 matches: ~120 ms

Btw a linear scan withreturn this._array.map(el => this._getKey(el)).indexOf(key) is still the fastest up to 1000 matches (~90 ms), but certainly will degrade pretty fast after that. It also changes the return value, so I did not bother with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That would also allocate memory for the mapped arrays, but yeah we're after better scaling while should happen with the binary search

@jerch
Copy link
MemberAuthor

@Tyriar I also noted an issue with decorations/selections from the search addon - if I have selections in the viewport and callreset the decoration boxes get not cleared, but stick to the viewport forever. This happens with all renderers (prolly not a rendering but a state holding issue in the decorations?) and cannot be undone by refreshing or clearing the selection:
image

Is this a (known) bug or am I missing here something?

@Tyriar
Copy link
Member

Not a known issue, created#4652

jerch reacted with thumbs up emoji

Comment on lines +134 to +137
letisDecorated=false;
this._decorationService.forEachDecorationAtCell(x,row,undefined,d=>{
isDecorated=true;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We can add ahasDecoration. A faster way currently would be usinggetDecorationsAtCell and break after the first one, that needs a generator created though.

jerch reacted with thumbs up emoji
@TyriarTyriar added this to the5.3.0 milestoneAug 9, 2023
@TyriarTyriar self-assigned thisAug 9, 2023
@Tyriar
Copy link
Member

Looks great provided tests pass 👍

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@TyriarTyriarTyriar approved these changes

Assignees

@TyriarTyriar

Labels

None yet

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

Search matchBackground isn't working in dom renderer

2 participants

@jerch@Tyriar

[8]ページ先頭

©2009-2025 Movatter.jp