-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
notebook open optimizations #13488
notebook open optimizations #13488
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine! Thanks!
packages/plugin-ext/src/main/browser/notebooks/notebook-documents-main.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Jonah Iden <[email protected]>
… text models Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
dd4fa8a
to
680b784
Compare
would love to have a second review on this PR. especially a second opinion on the rebinding of the |
packages/notebook/src/browser/service/notebook-monaco-text-model-service.ts
Outdated
Show resolved
Hide resolved
You asked for it ;-) 🤷 |
packages/notebook/src/browser/service/notebook-monaco-text-model-service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two things that bother me about this PR:
- We are overriding a service in the framework itself. This should not be necessary: we should be able refactor the framework until we have the desired configurability. What if a second module in Theia would do the same? That module and the notebook support could not work in parallel (both trying to impose their implementation).
- Whether we replicate text documents between front-end and back-end is purely a matter concerning the plugin system. The logic should be confined to the plugin-ext package, IMO.
What we're trying to achieve, in the end, is to optimize the special case where some documents exist implicitly, so no need to replicate them explicitly. This idea is valid. However, we also have other browser-side documents that also are not replicated to the extension side: for example the editors for breakpoint conditions (not sure how that is achieved, though). But if this is a capability multiple clients of the editor service require, we should build a corresponding extension point into the service instead of overriding it.
Thanks alot for the feedback! I'll if i can find a nicer solution with contribution point |
Signed-off-by: Jonah Iden <[email protected]>
thinking more about this contribution points are a bit overkill for this aren't they? I just added a |
A "contribution point" for me is a way to add/change behavior in the framework without replacing the component in the framework. Not a technical term. |
What it does
One larger performance issue for notebooks was the creation of the
MonacoEditorModels
used by the cell editors. The problem was that they needed to be created when the notebook is first loaded to be available to the extensions but where created one by one which required firing a network request for each singleMonacoEditorModel
. For a notebook with 100 cells this took up to 500ms.This PR changes that so that the documents are created seperately between backend and frontend.
In the backend they are created when the document for the notebook itself is created. In the frontend they are created afterwards in
createNotebookModel
in thenotebook-service.ts
Im not sure how nice the solution with rebinding the
MonacoTextModelService
is. Would love some feedback on thisHow to test
Since its about performance, best way to test is to check if working with notebooks is still working.
open, execute cell, change cell, execute again
Follow-ups
Review checklist
Reminder for reviewers