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

Native Editor/Interactive - Perf of a really large notebook #3047

Closed
rchiodo opened this issue Sep 19, 2019 · 11 comments
Closed

Native Editor/Interactive - Perf of a really large notebook #3047

rchiodo opened this issue Sep 19, 2019 · 11 comments
Assignees

Comments

@rchiodo
Copy link
Contributor

rchiodo commented Sep 19, 2019

Right now we send all of the cells around every time one of them changes. We probably need to change this to send changes like a normal document system does.

@rchiodo rchiodo self-assigned this Oct 2, 2019
@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 2, 2019

First step is just to try it out. Not necessarily fix anything

@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 2, 2019

1000 cells slows the entire thing down to a halt. Preliminary results indicate:

  • cloneDeep during load cells, which is entirely unnecessary
  • something on the extension side is horribly slow as well

@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 3, 2019

Other problems

  • Monaco editors each individually resize each other. Instead for native just force them all to the same width and let them compute their height (or compute height all at the same time)
  • Also rendering all of them takes up a lot of memory. Change to a hidden one that can be used to compute expected height/width and just make a dummy textbox

@DonJayamanne
Copy link
Contributor

Also rendering all of them takes up a lot of memory. Change to a hidden one that can be used to compute expected height/width and just make a dummy textbox

Suggestion

Why not re-use the monaco editors, rather than creating new ones.
I.e. assume we have 10 monaco editors. As we scroll, we move the monaco editors from the first cell (no longer visible) into the next visible cells and change their content.
Basically a mix of React Portals + Virtualized scroll (technically nothing is virtualized, except for the rendering of the editor. Use text editor when not visible).

This way we always have a fixed number of monaco editors ever, and we can alway use a small number. This should keep the memory at bay as well and improve perf (as we're not rendering or loading monaco everytime, merely updating the code).

Personally 50 is too high, we can only see around 10 at a time (varies based on height of the screen - which we can easily calculate).

@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 3, 2019

That sounds complicated. The sizing would be hard to get right. You'd have to move editors in and out of the dom as your scrolled and because they'd be different size than the code that's being scrolled into view, the view would fluctuate.

@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 3, 2019

I wonder if we could write something that would resize the same as the monaco editor, but just not have all the same parts?

@DonJayamanne
Copy link
Contributor

You'd have to move editors in and out of the dom as your scrolled and because they'd be different size than the code that's being scrolled into view, the view would fluctuate.

We wont need to move them (we use what we have to get the line height). Monaco editor has a fixed line line (it doesn't change based on content).

Here's the flow:

  • Add 10-20 monaco editors to some dom (React Portal)
  • When we load the cells (the first 10 will be visible - at least the first item).
  • Calculate lines in first editor and height, then we have a line height.
  • Next, adjust overall height of the notebook (calculate height of other cells based on their code content).
  • As we scroll into view, use a free monaco editor and use react portal to render the contents (react portal will do that automatically, all we do is set the content into the editor).

@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 3, 2019

This is wrong:

Monaco editor has a fixed line line (it doesn't change based on content).

Wrapping changes that calculus.

@DavidKutu
Copy link

I tested the native editor with 100 and 300 cells.
In both cases the editor opened in a couple of seconds as usual.
With 100 cells it ran them all in a few seconds and with 300 it took about a minute.

Given that the average notebook has around 30~ cells, this looks good to me. I don't expect 300 cells to execute instantly.

@DavidKutu
Copy link

The interactive window runs well with 100 cells. If you start it first and then run all cells below, it takes a while to complete but you are able to see every cell executing.

I did another test of just running 400 cells, it took 27 minutes to complete and 16 minutes for anything to appear on the interactive window. Its still not great but I don't know how far we can take the performance. In my opinion, becoming slow after 100 cells sounds acceptable.

@DavidKutu
Copy link

Validated

@lock lock bot locked as resolved and limited conversation to collaborators Oct 14, 2019
@microsoft microsoft unlocked this conversation Nov 14, 2020
@DonJayamanne DonJayamanne transferred this issue from microsoft/vscode-python Nov 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants