Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve decoration rendering efficiency #145751

Closed
Tyriar opened this issue Mar 22, 2022 · 6 comments · Fixed by #149486
Closed

improve decoration rendering efficiency #145751

Tyriar opened this issue Mar 22, 2022 · 6 comments · Fixed by #149486
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders perf terminal-find Relating the terminal's find widget terminal-shell-integration Shell integration, command decorations, etc. upstream Issue identified as 'upstream' component related (exists outside of VS Code) upstream-issue-fixed The underlying upstream issue has been fixed upstream-issue-linked This is an upstream issue that has been reported upstream verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Mar 22, 2022

Testing #145629

Could repro this slowness by filling the buffer and then searching for a single letter, this is on a 12th gen i7 (fast modern CPU):

image

A similar thing happens when pressing enter in this state since we don't do any diffing, just clear and re-render.

Ideas to explore:

  • Move from rendering each decoration to calculating colored zones which merge similar rectangles into the same zone and render them
  • If the buffer is full, explore shifting the texture up depending on how much scrolling occurs (might be tricky to get right)
@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug upstream Issue identified as 'upstream' component related (exists outside of VS Code) perf terminal-find Relating the terminal's find widget terminal-shell-integration Shell integration, command decorations, etc. labels Mar 22, 2022
@Tyriar Tyriar added this to the Backlog milestone Mar 22, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Mar 22, 2022

xterm.js issue: xtermjs/xterm.js#3703

@Tyriar Tyriar added the upstream-issue-linked This is an upstream issue that has been reported upstream label Mar 22, 2022
@Tyriar Tyriar added the upstream-issue-fixed The underlying upstream issue has been fixed label Apr 1, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Apr 1, 2022

After update no more overlap problems:

image

@Tyriar Tyriar closed this as completed in 1735a8f Apr 1, 2022
@rzhao271 rzhao271 modified the milestones: Backlog, April 2022 Apr 26, 2022
@connor4312 connor4312 added the verified Verification succeeded label Apr 29, 2022
@connor4312
Copy link
Member

connor4312 commented Apr 29, 2022

I'm not sure how to verify this, it seems like decorations don't show consistently, e.g. notice that only the first "vscode" is highlighted here:

image

When I try to run node -e "console.log('hello\n'.repeat(10 * 2000))" to fill up the buffer and look for "hello", nothing gets highlighted at all. And while I can click the up and down arrows to navigate between matches, the UI looks disabled as if there were no matches (but doesn't have the "no matches" text)

image

Reopening since I think something might have broken here.

@meganrogge
Copy link
Contributor

@connor4312 that is by design - we don't show the count or decorations beyond 1000 results

@meganrogge
Copy link
Contributor

you'll need to set workbench color customizations to see the results highlighted.

here are some examples:

        "terminal.findMatchBorder": "#ce1a1a",
        "terminal.findMatchHighlightBorder": "#00ffbf",
        "terminalOverviewRuler.findMatchForeground": "#00c8ff",

@connor4312
Copy link
Member

Got it. With 900 results I'm still seeing a pretty decent delay on my macbook, which looks similar to the original problematic one Daniel posted

image

@connor4312 connor4312 removed the verified Verification succeeded label Apr 29, 2022
@meganrogge meganrogge reopened this Apr 29, 2022
@meganrogge meganrogge modified the milestones: April 2022, May 2022 Apr 29, 2022
@meganrogge meganrogge removed upstream-issue-linked This is an upstream issue that has been reported upstream upstream-issue-fixed The underlying upstream issue has been fixed labels Apr 29, 2022
@meganrogge meganrogge changed the title Optimize rendering of decorations in terminal overview ruler refreshDecorations is slow May 5, 2022
@meganrogge meganrogge changed the title refreshDecorations is slow improve decoration rendering efficiency May 9, 2022
Tyriar added a commit to Tyriar/xterm.js that referenced this issue May 9, 2022
@Tyriar Tyriar added upstream-issue-linked This is an upstream issue that has been reported upstream upstream-issue-fixed The underlying upstream issue has been fixed labels May 13, 2022
Tyriar added a commit that referenced this issue May 13, 2022
- A bunch of changes to xterm.js including bg/fg decoration overrides
- Tweak find colors to use background instead of border, align with the editor
- Change high contrast themes to also align, including selection bg
- Clear the find active result decoration on blur and when the selection changes

Fixes #147013
Fixes #145751
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels May 13, 2022
@Tyriar Tyriar added the verified Verification succeeded label Jun 3, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2022
lemanschik pushed a commit to code-oss-dev/code that referenced this issue Nov 25, 2022
lemanschik pushed a commit to code-oss-dev/code that referenced this issue Nov 25, 2022
- A bunch of changes to xterm.js including bg/fg decoration overrides
- Tweak find colors to use background instead of border, align with the editor
- Change high contrast themes to also align, including selection bg
- Clear the find active result decoration on blur and when the selection changes

Fixes microsoft#147013
Fixes microsoft#145751
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders perf terminal-find Relating the terminal's find widget terminal-shell-integration Shell integration, command decorations, etc. upstream Issue identified as 'upstream' component related (exists outside of VS Code) upstream-issue-fixed The underlying upstream issue has been fixed upstream-issue-linked This is an upstream issue that has been reported upstream verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@Tyriar @connor4312 @rzhao271 @meganrogge @vscodenpa and others