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

Performance improvements for initialization #6172

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

svenefftinge
Copy link
Contributor

@svenefftinge svenefftinge commented Sep 12, 2019

What it does

Avoid some heavy scripting during loading.

  1. avoid monaco contribution calling expensive getPreferences repeatedly on many small changes during loading.
  2. avoid constructing many URIs unnecessarily.

The main change is that I have replaced taking copies of preferences and keeping them in-sync with using a proxy that directly uses the underlying data.

Also this PR cleans up and simplifies the test for the preference-service and preference-proxy.

How to test

Measure performance for scripting during load before and after the change & compare.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added monaco issues related to monaco performance issues related to performance labels Sep 12, 2019
@svenefftinge svenefftinge changed the title Performance improvements for initialization [WIP] Performance improvements for initialization Sep 13, 2019
@svenefftinge svenefftinge force-pushed the se/loading_perf_impr branch 3 times, most recently from 371f968 to 4587669 Compare September 16, 2019 11:14
@svenefftinge svenefftinge changed the title [WIP] Performance improvements for initialization Performance improvements for initialization Sep 16, 2019
@svenefftinge svenefftinge added the preferences issues related to preferences label Sep 16, 2019
@svenefftinge
Copy link
Contributor Author

svenefftinge commented Sep 16, 2019

  • fixing the tests

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I haven't looked at the code, only did a comparison before and after the change:

Tests (3 Tests / Scenario)

Use Case Avg Before (sec) Avg After (sec) Avg Improvement (sec) Avg % Improvement
No workspace 1.642 1.514 -0.128 👍 ~8% improvement
With workspace (theia + views open) - browser 6.015 3.109 -2.906 👍 ~48% improvement
With workspace (theia + views open) - electron 6.418 3.941 -2.477 👍 ~39% improvement

Additional Info

I modified the following code on master and this pr to capture the times:

frontend-application.ts

async start(): Promise<void> {
  const start = new Date();
  await this.startContributions();
  this.stateService.state = 'started_contributions';
  
  const host = await this.getHost();
  this.attachShell(host);
  await new Promise(resolve => requestAnimationFrame(() => resolve()));
  this.stateService.state = 'attached_shell';

  await this.initializeLayout();
  this.stateService.state = 'initialized_layout';
  await this.fireOnDidInitializeLayout();

  await this.revealShell(host);
  this.registerEventListeners();
  this.stateService.state = 'ready';
  const end = new Date();
  console.log(`total time till ready: ${(end.getTime() - start.getTime()) / 1000} sec`);
}

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

very cool that we can simplify and speed up configurations, but i am not sure about some changes, please see comments

@svenefftinge svenefftinge force-pushed the se/loading_perf_impr branch 5 times, most recently from 2b90d43 to e16d80d Compare September 17, 2019 11:48
@svenefftinge
Copy link
Contributor Author

@vince-fugnitto thanks for the measurements. If you put a bunch of VS code extensions into the mix it becomes even more visible. Also there's lots of scripting going on that is not strictly blocking the frontend startup, but it makes the browser busy for the first seconds.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

Everything works nicely except getEOL.

packages/monaco/src/browser/monaco-editor-provider.ts Outdated Show resolved Hide resolved
@akosyakov
Copy link
Member

akosyakov commented Sep 18, 2019

I wonder whether we can remove updating editor options:

protected updateMonacoEditorOptions(editor: MonacoEditor, event?: EditorPreferenceChange): void {
if (event) {
const preferenceName = event.preferenceName;
const overrideIdentifier = editor.document.languageId;
const newValue = this.editorPreferences.get({ preferenceName, overrideIdentifier }, undefined, editor.uri.toString());
editor.getControl().updateOptions(this.setOption(preferenceName, newValue, this.preferencePrefixes));
} else {
const options = this.createMonacoEditorOptions(editor.document);
delete options.model;
editor.getControl().updateOptions(options);
}
}

Since we hook our preferences into the configuration service, maybe Monaco can react to changes from the configuration service and update editors already without us doing the double work. It could be investigated separately.

@svenefftinge
Copy link
Contributor Author

Since we hook our preferences into the configuration service, maybe Monaco can react to changes from the configuration service and update editors already without us doing the double work. It could be investigated separately.

I tried and it didn't seem to work. But of course someone could look into it.

Signed-off-by: Sven Efftinge <sven.efftinge@typefox.io>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

getEOL works nicely now too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco performance issues related to performance preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants