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

Scrolling in a notebook diff editor is sluggish #109680

Closed
kieferrm opened this issue Oct 29, 2020 · 5 comments · Fixed by #112176
Closed

Scrolling in a notebook diff editor is sluggish #109680

kieferrm opened this issue Oct 29, 2020 · 5 comments · Fixed by #112176
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@kieferrm
Copy link
Member

When you have a reasonably sized notebook (around 30 cells) with changes and you open the notebook diff editor from it in the SCM view, scrolling in the notebook diff editor is very slow. For example, grab the scrollbar thumb and drag it up and down.

@rebornix rebornix added freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues bug Issue identified by VS Code Team member as probable bug labels Oct 29, 2020
@rebornix rebornix added this to the November 2020 milestone Nov 11, 2020
@rebornix
Copy link
Member

We should apply the same optimization we did in the notebook editor in the diff editor too

  • reuse code editors, diff editors for the cell input, metadata or output. Leverage the list template
  • static layout code editor and diff editor
    • test that our listeners to content size change of the diff editor still works

@rebornix
Copy link
Member

rebornix commented Dec 8, 2020

Perf chart before any optimization: when scrolling fast, we often see this 200+ms scrolling handler which tries to build editors again and again, which measures the size of the DOM element and triggers forced reflow

image

The first thing to do is reusing editors for the same type of list item, to avoid creating and disposing over and over again. After leveraging list view template, the perf chart now looks like

image

There are still some red dots there, meaning there are still forced reflow. Zooming in and we found that they are triggered by creating overview ruler (canvas) on every DOM resize. Since we don't want the overview ruler to be rendered for every individual embeded diff editor, we should add an option to disable the unnecessary rerendering.

image

@rebornix
Copy link
Member

rebornix commented Dec 9, 2020

After disabling the overview ruler redundant rendering, we came to this flame chart

image

After triggering a mouse scroll (I'm using MX Master so it scrolls quite a bit), before it stops scrolling, it will render multiple pages of the diff view. From the chart above we can see that there is a 200ms task, which makes the scrolling noticeable sluggish. It's not doing anything wrong but just trying to render monaco editors. On a relatively large monitor (say 27", 150% scale), there would be around 20 monaco editors in the viewport if every cell is single line. Creating tens of monaco editor instances can easily cost hundreds of milliseconds.

Since we can't decide how many editors will be rendered, let's push monaco editor rendering to its limit. In VS Code, each regular monaco editor will initialize 50+ contributions (or plugins), if we only load the necessary ones (suggest, code snippet, accessibility) and we can save tens of millisenconds.

image

Most scroll handlers finish within 120ms and half of them are even below 50ms, considering that performance tracing amplifies the slowness, we can get a refresh rate at around 15to 30. For example, when I do a javascript performance profiling (which has less interference to the rendering), we can have 14 scroll event handling within 400ms with the same machine set up

image

@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2021
@rebornix rebornix added the verification-needed Verification of issue is requested label Jan 26, 2021
@rebornix
Copy link
Member

Verifier: please make changes to github issue notebooks in .vscode\notebooks\my-endgame.github-issues and then open changes in diff editor, please test that when you are scrolling in the diff editor, the performance is reasonable.

@lramos15 lramos15 added the verified Verification succeeded label Jan 26, 2021
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 freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@rebornix @lramos15 @kieferrm and others