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

Use V8 snapshots for the amd-loader #28492

Closed
jrieken opened this issue Jun 12, 2017 · 6 comments
Closed

Use V8 snapshots for the amd-loader #28492

jrieken opened this issue Jun 12, 2017 · 6 comments
Assignees
Labels
engineering VS Code - Build / issue tracking / etc. feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Jun 12, 2017

To improve startup performance we should use v8-snapshots. That helps with code-loading but also with initialising (static) state. There are some challenges and this issue is about identifying and assessing them.

@jrieken jrieken self-assigned this Jun 12, 2017
@jrieken jrieken added this to the June 2017 milestone Jun 12, 2017
@jrieken jrieken added the feature-request Request for new features or functionality label Jun 12, 2017
@jrieken
Copy link
Member Author

jrieken commented Jun 13, 2017

Challenges and things we need to tackle

  • Today the snapshot data (snapshot_blob.bin) is loaded in any v8-context, e.g the main process, the extension host process, renderer processes etc, etc. There might be a way of configuring this but I haven't found it yet
  • We statically execute code at load time we cannot execute in a AOT-compile, e.g. things like this _isWindows = (process.platform === 'win32'); or create shared/probing dom nodes: const sharedStyle = <any>createStyleSheet(); . The challenge is to first identify all static code that is context dependent, rewriting things shouldn't be too hard.
  • Our NLS story is implemented as loader-plugin. Similar to statically executed code this isn't compatible with AOT compiled code.
  • We could try something a lot more shallow and just compile the "outer" define script, without executing the factories tho it's questionable how much that helps with performance...

@jrieken
Copy link
Member Author

jrieken commented Jun 14, 2017

We could try something a lot more shallow and just compile the "outer" define script, without executing the factories tho it's questionable how much that helps with performance...

Some results are in for this, since it is easy'ish to try. The idea is take all of workbench.main.js and wrap it into a function like so function Run_Workbench_Main() { ... actual file contents ... } and create a snapshot of that. Because nothing is executed the snapshot will just be another (more efficient?) representation of that script. Turns out hat the benefit isn't great. Loading workbench-main via a snapshot loads in ~1sec, loading the code from JS-source and generating cached data is ~1.4sec, and loading things from cached data is ~0.5sec. So, the very startup will be faster, all subsequent startups will be slower. Little surprisingly, the conclusion is that snapshots only really pay off if we can capture more (computed) state, like contents of our registries et al

@jrieken jrieken added the engineering VS Code - Build / issue tracking / etc. label Jun 26, 2017
@jrieken jrieken modified the milestones: July 2017, June 2017 Jun 26, 2017
@jrieken
Copy link
Member Author

jrieken commented Jun 26, 2017

We will finalise this in July

jrieken added a commit that referenced this issue Jul 10, 2017
jrieken added a commit that referenced this issue Jul 11, 2017
@marvinhagemeister
Copy link
Contributor

Note that v8 snapshots have been disabled because of security concerns as of today. https://nodejs.org/en/blog/vulnerability/july-2017-security-releases/

Not sure if this affects electron, too.

@jrieken
Copy link
Member Author

jrieken commented Jul 13, 2017

re #28492 (comment). Interesting. This won't have an immediate impact on us as electron is a little behind the latest version of node.js. Let's hope a better, less emergency-style, fix comes out soon. As far as I understand this isn't an issue with snapshots per se but with snapshotting seeds.

@jrieken
Copy link
Member Author

jrieken commented Jul 13, 2017

After understanding the extend of work needed to make snapshots for all of our sources and static state we have re-scoped this issue to only snapshot the amd-loader. That made it a good exercise but performance wins are marginal. This is how snapshots and cached data are now used:

  • there is a snapshot of the amd-loader backed into our distro
  • the loader utilises v8 cached data when loading the actual sources

When starting up, the loader is more less already there and with the help of cached data loading the main chunk of our code is fast too (~500ms instead of ~1500ms). However, on the very first startup there is no cached data yet (is being generated then) and cached data cannot capture heap-state like snapshots do.

We will keep an eye on snapshot, also new features for this landed in v8 recently and we are eager to have them in electron. Until then snapshot work is considered to be done.

@jrieken jrieken closed this as completed Jul 13, 2017
@jrieken jrieken changed the title V8 snapshot support Use V8 snapshots for the amd-loader Jul 13, 2017
@jrieken jrieken added verification-needed Verification of issue is requested verified Verification succeeded labels Jul 24, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engineering VS Code - Build / issue tracking / etc. feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

2 participants