Remove dependency on node require for startup code path #98682

deepak1556 · 2020-05-27T20:47:28Z

Things to do:

V8 cached options support (needs Electron patch) @deepak1556
🏃 validate no performance regressions
remove traces of node cached data (e.g. cleanup from shared process, VSCODE_NODE_CACHED_DATA_DIR)
figure out alternative to know when cached data was used or not (here)

While rewriting our asset resolution over file protocol to use a custom protocol https://github.com/microsoft/vscode/tree/robo/vscode-file , I hit this weird error where relative requires failed.

[11790:0527/130416.846222:INFO:CONSOLE(720)] "Uncaught Error: Cannot find module '../../../base/common/performance'
Require stack:
- electron/js2c/renderer_init", source: internal/modules/cjs/loader.js (720)
[11790:0527/130419.759924:INFO:CONSOLE(720)] "Uncaught Error: Cannot find module '../../../../bootstrap'
Require stack:
- electron/js2c/renderer_init", source: internal/modules/cjs/loader.js (720)

I couldn't repro the issue with electron, so not sure whats happening here. As the error is not from the loader but rather failure from node require.

Instead of trying to tackle this, @alexdima suggested more robust approach that will align well with the sandbox renderer. We start removing the direct usage of node require on a case by case basis. Right now the low hanging fruit is the bootstrap, loader code and perf module. I am creating this task to tackle these.

performance module in workbench @jrieken
- Since this needs to be run/available before our loader kicks in, we could either inline it in the workbench.html and make it available via the window global or
- With @bpasero work on sandbox Introduce electron-sandbox #98628, we now have preload scripts that can be used to expose these api, as they are guaranteed to be run before the page script runs.
bootstrap-window require, which can be inlined as a <script> @bpasero
- workbench
- sharedProcess
- issueReporter
- processExplorer
vscode-loader @alexdima
- start using <script> tags
- Remove hardcoded file scheme in https://github.com/microsoft/vscode-loader/tree/master/src/core
  - Currently we return absolute path with file scheme appended https://github.com/microsoft/vscode-loader/blob/master/src/core/moduleManager.ts#L763 , instead what comes out of the loader should just be a <script> with relative paths and let chromium/blink resource loader resolve it. That way we can make it independent of the page's protocol.

These changes will let the app specific resources (scripts, images, css etc) to be handled by the protocol handler and also let chromium use respective caching mechanisms at the loader level.

The text was updated successfully, but these errors were encountered:

deepak1556 · 2020-05-28T07:22:03Z

Another topic that is directly related to the startup path is code caching. Currently we rely on https://nodejs.org/api/vm.html#vm_script_createcacheddata to create the cache data and use for scripts required via node. In the sandbox case, we will have to rely on the code caching from blink. The cache data created in both cases rely on the same v8 api except in blink case certain heuristics are applied.

I would highly recommend to watch https://www.youtube.com/watch?v=YqHOUy2rYZ8 which explains the state of code caching in chromium before reading the solution below.

Tl:dr;

Isolate cache is available for free from v8
Resource cache kicks in from chromium, which is useful when get rid of node require
Persistent code cache is generated based on the runs and the validity of the code (<72 hrs)
- Not all code are eagerly compiled and put into cache even after warm run
  - https://v8.dev/blog/code-caching-for-devs#merge
  - https://v8.dev/blog/code-caching-for-devs#iife
  - there is a flag kV8CacheOptionsFullCodeWithoutHeatCheck that would allow eager compilation but v8 doesn't support it for streaming scripts.
- scripts less than 1KB will not be cached, better to merge smaller scripts together.

Given this, there is still some area of control we can gain when creating code cache with blink. For desktop apps it doesn't make sense to do the cold, warm and hot run checks, also the validity period of the script. This was exactly the case for PWA's and chromium added a new flag that would bypass these heat checks but the catch is that the scripts had to be served from service worker https://bugs.chromium.org/p/chromium/issues/detail?id=768705.

After looking around a bit there is already a command line switch --v8-cache-options, with a little patch to chromium we can propagate a new flag --v8-cache-options=bypassHeatChecks for regular scripts.

We can take a chrome trace once the pieces are in place and compare how much is the saving with/without the heat checks.

The crust of the code caching heuristics are here if needed.

Also one more info, scripts from dedicated worker, shared worker have hardcoded cache type which currently defaults to the heuristic based caching. I am not addressing this, since I don't think there is much gain here.

jrieken · 2020-05-28T07:34:04Z

performance module in workbench

I can take care of this. By now we can use the performance-api (https://developer.mozilla.org/en-US/docs/Web/API/Performance) which also has a counter part in node.js

alexdima · 2020-06-01T09:27:58Z

Adding @bpasero fro the bootstrap-window require.

jrieken · 2020-06-08T09:31:21Z

I have removed the require-call for the performance util

bpasero · 2020-06-18T08:57:07Z

@deepak1556 @alexdima this needs a bit of guidance to understand what should happen. bootstrap-window.js depends on the boostrap.js helper and there is a lot of require usages in there, including node.js dependencies. How would these get loaded via script tags. That is not clear to me. An example would help.

Given the number of dependencies to node.js, I wonder if it would be better to introduce a new bootstrap-sandbox.js that is free of any node.js dependencies. Of course this would require adoption until we restore the current functionality and would not be something I would push to existing users.

So I guess the bottom line is: how can we move forward on dropping the file protocol for resources in renderers independent from the sandbox efford?

bpasero · 2020-06-18T12:23:09Z

I need a hint what the path should be, e.g. naively putting this in to workbench.html does not work obviously:

<script src="../../../../bootstrap.js"></script>
<script src="../../../../bootstrap-window.js"></script>
<script src="workbench.js"></script>

Does this require loader configuration similar to web?

vscode/src/vs/code/browser/workbench/workbench.html

Lines 29 to 43 in 453454b

    
           <script> 
        
           	// NOTE: Changes to inline scripts require update of content security policy 
        
           	self.require = { 
        
           		baseUrl: `${window.location.origin}/static/out`, 
        
           		paths: { 
        
           			'vscode-textmate': `${window.location.origin}/static/node_modules/vscode-textmate/release/main`, 
        
           			'vscode-oniguruma': `${window.location.origin}/static/node_modules/vscode-oniguruma/release/main`, 
        
           			'xterm': `${window.location.origin}/static/node_modules/xterm/lib/xterm.js`, 
        
           			'xterm-addon-search': `${window.location.origin}/static/node_modules/xterm-addon-search/lib/xterm-addon-search.js`, 
        
           			'xterm-addon-unicode11': `${window.location.origin}/static/node_modules/xterm-addon-unicode11/lib/xterm-addon-unicode11.js`, 
        
           			'xterm-addon-webgl': `${window.location.origin}/static/node_modules/xterm-addon-webgl/lib/xterm-addon-webgl.js`, 
        
           			'semver-umd': `${window.location.origin}/static/node_modules/semver-umd/lib/semver-umd.js`, 
        
           		} 
        
           	}; 
        
           </script>

deepak1556 · 2020-06-18T21:58:45Z

Sorry for not being clear, answers inline

how can we move forward on dropping the file protocol for resources in renderers independent from the sandbox efford?

The problem we are trying to solve in this issue is to eliminate the node requires with relative paths, as these are not working properly with custom protocols for unknown reason

[11790:0527/130416.846222:INFO:CONSOLE(720)] "Uncaught Error: Cannot find module '../../../base/common/performance'
Require stack:
- electron/js2c/renderer_init", source: internal/modules/cjs/loader.js (720)
[11790:0527/130419.759924:INFO:CONSOLE(720)] "Uncaught Error: Cannot find module '../../../../bootstrap'
Require stack:
- electron/js2c/renderer_init", source: internal/modules/cjs/loader.js (720)

The problematic ones are only bootstrap-window, bootstrap at the moment.

bootstrap-window.js depends on the boostrap.js helper and there is a lot of require usages in there, including node.js dependencies. How would these get loaded via script tags. That is not clear to me. An example would help.

The node.js dependencies inside the bootstrap script can be present the way they are as long as they are not hardcoded relative paths, the loading of these modules will not be affected by the custom protocol transition. Based on my search in the codebase, we only had require('../../../../bootstrap'), require('../../../../bootstrap-window'), so I think the rest are fine.

Does this require loader configuration similar to web?

No we don't need it for fixing only the bootstrap paths, since the workbench.js on desktop expects the boostrap code to be available, it would be sufficient to have something like

<script src="./static/out/vs/bootstrap-window.js"></script>
<script src="workbench.js"></script>

or

<script>
 Inlined bootstrap code
</script>
<script src="workbench.js"></script>

I try to resolve the resources in the protocol handler wrt the app resources path 86b7bbc , so the above should work just fine.

I wonder if it would be better to introduce a new bootstrap-sandbox.js that is free of any node.js dependencies. Of course this would require adoption until we restore the current functionality and would not be something I would push to existing users.

Yes this should be done along side sandbox workbench, which will have the loader config similar to web. I think this is the end goal, but as you noted its not something we can push to users currently.

bpasero · 2020-06-24T07:13:56Z

I have merged my changes which:

load bootstrap.js via script tags as window.MonacoBootstrap in browser environments (we still need the node.js variant for other places where we load this file)
load bootstrap-window.js via script tags as window.MonacoBootstrapWindow in all of our windows (workbench, issue reporter, process explorer) except the shared process which I think will remain privileged even in the future

I think the next steps are to fix the remaining require call with a relative path for loader.js @alexdima:

vscode/src/bootstrap-window.js

Line 87 in a3f86f0

const amdLoader = require(`${configuration.appRoot}/out/vs/loader.js`);

As discussed, the loader probably needs changes to be loaded through script tags while still understanding that it runs in a node.js environment to preserve our node.js script loading and cached data. A next step would be to actually drop our cached data solution and rely on the new V8 cache options from @deepak1556. But I think we can take one step at a time:

load loader.js via script tags
enable custom protocol for all our static assets
drop our custom script cache solution and use V8 flags + script loading for all our scripts

alexdima · 2020-06-26T07:35:02Z

With the latest changes I pushed and the unblock of the fetch from @deepak1556 , the workbench now loads fine in the branch robo/vscode-file. There are a couple of shortcuts I took that we should clean up, but this is good for testing I think

bpasero · 2020-06-26T07:43:56Z

Very cool, given we are approaching endgame soon, I wonder if the switch to custom protocol should happen in debt week to give us a bit more insiders coverage.

deepak1556 · 2020-06-26T07:48:46Z

Yup @bpasero that would be the way to go, there is still some polishing left and also we are currently iterating E8 perf numbers, it is ideal to switch this in debt week.

deepak1556 · 2021-03-17T03:12:27Z

With:

https://domoreexp.visualstudio.com/Teamspace/_git/electron-build/pullrequest/321429
https://domoreexp.visualstudio.com/Teamspace/_git/electron-build/pullrequest/327038

Code caching now works with custom protocols, look for the cacheConsumeOptions and cacheRejected values in the below snap

Here are some additional observations,

With v8Cacheoptions: none for vscode-file
| workbench ready                                                 | 3197     | [main->renderer]          | -                                      |
| renderer ready                                                  | 2493 
with v8Cacheoptions: bypassHeatCheck for vscode-file
| workbench ready                                                 | 2804     | [main->renderer]          | -                                      |
| renderer ready                                                  | 2140  
with workbench loaded by node.js (current default)
| workbench ready                                                 | 2566     | [main->renderer]          | -                                      |
| renderer ready                                                  | 1899

There is still a 300ms difference between loading via node.js and chromium. The difference is mainly attributed to the network layer that loads the files rather than the v8 compilation stage which now uses the cache. For ex: In a typical script loading flow that starts from ThrottlingURLLoader::OnReceiveResponse and ends with ResourceDispatcher::OnRequestComplete as seen below approx takes 0.126ms and we load ~1700 scripts before the workbench finishes loading, this covers the difference as in the node.js scenario there is no ipc hop to a different process for loading a file.

Will compare the product build numbers once they are out.

Side Note:

With Chrome >75 there is a feature called script streaming which allows v8 to parse the scripts from network on a background thread instead of having to wait on the main thread https://v8.dev/blog/v8-release-75#script-streaming-directly-from-network, this doesn't work for custom protocols but have fixed that in the above PR, so we now have script streaming too. There is catch until chrome > 91 or atleast till https://chromium-review.googlesource.com/c/chromium/src/+/2725097 lands , script steaming only works for scripts larger than 30kb. So to try this feature in OSS build, start the app with --enable-features="SmallScriptStreaming"

bpasero · 2021-03-17T06:42:08Z

👏

LeszekSwirski · 2021-03-19T07:54:58Z

I'm happy to help our here where I can. I'd recommend turning on SmallScriptStreaming along with V8OffThreadFinalization, otherwise the overheads tend to outweigh the benefits of streaming small scripts. This should be fine to do on electron built on top of chromium M88 or above.

deepak1556 · 2021-03-20T06:11:25Z

@LeszekSwirski thanks for the pointer on the V8OffThreadFinalization. When running with no code cache and --enable-features="SmallScriptStreaming,V8OffThreadFinalization" on electron built with chromium 89.0.4389.82, I see the startup numbers same as when background compilation is not enabled. Attaching the trace for reference

chrometrace.log.zip

I see the V8.CompileCodeBackground task is run for each streamed script but interesting is all these scripts get a v8.Compile, v8.CompileCode tasks on the main thread a while later. Is this supposed to happen ? What makes a script compile on both background and main thread ?

LeszekSwirski · 2021-03-22T13:37:49Z

The V8.CompileCode tasks you're seeing are lazy compilations during execution; the streaming finalization is all inside v8.compile just before v8.run. You might be able to move those to the background by using the IIFE hack. I also see, later on, synchronous V8.ScriptCompiler executions within that big V8.Execute block; I'm guessing those are evals? If those don't need to be synchronous, you could try to replace them with "proper" script loads and see if you can get them streaming too?

In general though, I wouldn't expect streaming to help unless you're loading multiple scripts in parallel, or able to execute scripts in parallel with loading; otherwise, you're just moving the same operations from one thread to another, but your end-to-end latency stays the same. In the linked trace, it looks like there's not much being loaded alongside workbench.js, and it looks like actually parsing workbench.js is pretty short, so improvements may be modest.

…isabled (#98682)

)

bpasero · 2021-06-08T07:25:21Z

9636518 enables vscode-file:/ and bypassHeatCheck code caching solution.

Code cache folder is partitioned per commit here:

vscode/src/vs/code/electron-main/app.ts

Line 164 in 2cb7b42

    
           defaultSession.setCodeCachePath(join(this.environmentMainService.codeCachePath, 'chrome'));

We store the code cache into the same folder as we do for our node.js based cache to benefit from reusing our clean up task (src/vs/code/electron-browser/sharedProcess/contrib/codeCacheCleaner.ts) that takes care of removing old code cache folders.

I had to find an alternative solution for figuring out if cached data is used or not because we currently have no API for that. My strategy is to assume cached data is present whenever VSCode is running for the Nth+1 time for a commit:

vscode/src/vs/workbench/services/timer/electron-sandbox/timerService.ts

Lines 103 to 113 in 745e354

    
           if (typeof _didUseCachedData !== 'boolean') { 
        
           	if (!environmentService.configuration.codeCachePath || !productService.commit) { 
        
           		_didUseCachedData = false; // we only produce cached data whith commit and code cache path 
        
           	} else if (storageService.get(lastRunningCommitStorageKey, StorageScope.GLOBAL) === productService.commit) { 
        
           		_didUseCachedData = true; // subsequent start on same commit, assume cached data is there 
        
           	} else { 
        
           		storageService.store(lastRunningCommitStorageKey, productService.commit, StorageScope.GLOBAL, StorageTarget.MACHINE); 
        
           		_didUseCachedData = false; // first time start on commit, assume cached data is not yet there 
        
           	} 
        
           } 
        
           return _didUseCachedData;

//cc @jrieken @deepak1556

bpasero · 2021-06-15T09:15:16Z

Moving to July for enabling this permanently. For June we aim to ship this with a flag to disable it in case needed.

deepak1556 assigned deepak1556, jrieken and alexdima May 27, 2020

deepak1556 added the sandbox Running VSCode in a node-free environment label May 27, 2020

egamma mentioned this issue May 28, 2020

Implement a node.js-free, sandboxed, context isolated renderer #92164

Closed

2 tasks

deepak1556 mentioned this issue May 29, 2020

feat: add V8CacheOptions webpreference electron/electron#23842

Merged

6 tasks

alexdima assigned bpasero Jun 1, 2020

bpasero added this to the June 2020 milestone Jun 3, 2020

jrieken removed their assignment Jun 8, 2020

kieferrm mentioned this issue Jun 15, 2020

Iteration Plan for June 2020 #100100

Closed

57 tasks

bpasero mentioned this issue Jun 23, 2020

Drop node.js require for bootstrap and bootstrap-window #100857

Merged

bpasero removed their assignment Jun 24, 2020

bpasero added the plan-item VS Code - planned item for upcoming label Jun 24, 2020

alexdima modified the milestones: June 2020, July 2020 Jun 29, 2020

alexdima mentioned this issue Jul 7, 2020

Use vscode-file:// instead of file:// #101837

Closed

kieferrm mentioned this issue Jul 10, 2020

Iteration Plan for July 2020 #102174

Closed

49 tasks

deepak1556 modified the milestones: July 2020, August 2020 Jul 29, 2020

bpasero added this to the On Deck milestone Jan 8, 2021

bpasero modified the milestones: On Deck, March 2021 Mar 3, 2021

bpasero modified the milestones: March 2021, April 2021 Mar 22, 2021

bpasero added a commit that referenced this issue Apr 6, 2021

sandbox - enable vscode-file// protocol by default (#98682)

5818668

digitarald pushed a commit that referenced this issue Apr 8, 2021

sandbox - enable vscode-file// protocol by default (#98682)

30adb29

gjsjohnmurray pushed a commit to gjsjohnmurray/vscode that referenced this issue Apr 8, 2021

sandbox - enable vscode-file// protocol by default (microsoft#98682)

b96eca2

bpasero modified the milestones: April 2021, May 2021 Apr 21, 2021

bpasero modified the milestones: May 2021, June 2021 Jun 1, 2021

bpasero added a commit that referenced this issue Jun 8, 2021

sandbox - implement didUseCachedData when node.js code loading is d…

745e354

…isabled (#98682)

bpasero added a commit that referenced this issue Jun 8, 2021

code - enable vscode-file protocol and bypassHeatCheck caching (#98682

9636518

)

disco0 mentioned this issue Jun 11, 2021

Loading modules in home directory via extension configuration fails, causes issues with Insiders iocave/monkey-patch#27

Open

bpasero modified the milestones: June 2021, July 2021 Jun 15, 2021

bpasero mentioned this issue Jun 25, 2021

Test: running with vscode-file and V8 cached data #127168

Closed

3 tasks

bpasero modified the milestones: July 2021, August 2021 Jul 1, 2021

bpasero mentioned this issue Aug 4, 2021

Remove dependency on node require for startup code path #130088

Merged

bpasero closed this as completed in #130088 Aug 4, 2021

github-actions bot locked and limited conversation to collaborators Sep 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency on node require for startup code path #98682

Remove dependency on node require for startup code path #98682

deepak1556 commented May 27, 2020 •

edited by bpasero

Loading

deepak1556 commented May 28, 2020 •

edited

Loading

jrieken commented May 28, 2020 •

edited

Loading

alexdima commented Jun 1, 2020

jrieken commented Jun 8, 2020

bpasero commented Jun 18, 2020 •

edited

Loading

bpasero commented Jun 18, 2020

deepak1556 commented Jun 18, 2020

bpasero commented Jun 24, 2020 •

edited

Loading

alexdima commented Jun 26, 2020

bpasero commented Jun 26, 2020

deepak1556 commented Jun 26, 2020

deepak1556 commented Mar 17, 2021 •

edited

Loading

bpasero commented Mar 17, 2021

LeszekSwirski commented Mar 19, 2021

deepak1556 commented Mar 20, 2021 •

edited

Loading

LeszekSwirski commented Mar 22, 2021

bpasero commented Jun 8, 2021 •

edited

Loading

bpasero commented Jun 15, 2021

Remove dependency on node require for startup code path #98682

Remove dependency on node require for startup code path #98682

Comments

deepak1556 commented May 27, 2020 • edited by bpasero Loading

deepak1556 commented May 28, 2020 • edited Loading

jrieken commented May 28, 2020 • edited Loading

alexdima commented Jun 1, 2020

jrieken commented Jun 8, 2020

bpasero commented Jun 18, 2020 • edited Loading

bpasero commented Jun 18, 2020

deepak1556 commented Jun 18, 2020

bpasero commented Jun 24, 2020 • edited Loading

alexdima commented Jun 26, 2020

bpasero commented Jun 26, 2020

deepak1556 commented Jun 26, 2020

deepak1556 commented Mar 17, 2021 • edited Loading

bpasero commented Mar 17, 2021

LeszekSwirski commented Mar 19, 2021

deepak1556 commented Mar 20, 2021 • edited Loading

LeszekSwirski commented Mar 22, 2021

bpasero commented Jun 8, 2021 • edited Loading

bpasero commented Jun 15, 2021

deepak1556 commented May 27, 2020 •

edited by bpasero

Loading

deepak1556 commented May 28, 2020 •

edited

Loading

jrieken commented May 28, 2020 •

edited

Loading

bpasero commented Jun 18, 2020 •

edited

Loading

bpasero commented Jun 24, 2020 •

edited

Loading

deepak1556 commented Mar 17, 2021 •

edited

Loading

deepak1556 commented Mar 20, 2021 •

edited

Loading

bpasero commented Jun 8, 2021 •

edited

Loading