Vscode: Remove dependency on node require for startup code path

[deepak1556]

Refs https://github.com/microsoft/vscode/issues/92164

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.

• [x] 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 https://github.com/microsoft/vscode/pull/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.

• [x] bootstrap-window require, which can be inlined as a <script> @bpasero

  • [x] workbench
  • [x] sharedProcess
  • [x] issueReporter
  • [x] processExplorer

• [x] vscode-loader @alexdima

  • [x] start using <script> tags
  • [x] 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.

[deepak1556]

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]

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]

Adding @bpasero fro the bootstrap-window require.

[jrieken]

I have removed the require-call for the performance util

[bpasero]

@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]

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?

https://github.com/microsoft/vscode/blob/453454b7028211ad5872489e0e504db105531aa8/src/vs/code/browser/workbench/workbench.html#L29-L43

[deepak1556]

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 https://github.com/microsoft/vscode/commit/86b7bbc7daef1d9dfceea5e04dadcf5ea250d82d , 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]

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:

https://github.com/microsoft/vscode/blob/a3f86f0923eeecbc9826d5bd88e9fe0986b79a8f/src/bootstrap-window.js#L87

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]

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]

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]

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.