microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
160.15k stars 28.05k forks source link

Regression in `require(main.bundle.js)` timings #212769

Open bpasero opened 1 month ago

bpasero commented 1 month ago

Our perf machines show much longer time spend in the require of our electron-main bundle ever since Electron 29 landed in insiders:

Linux image

macOS image

Windows image

Linux is the worst performing platform here, but it is also the slowest machine.

Since Electron 29 comes with a major node.js update, I wonder if something broke in the area of code caching, where I believe we are relying on node.js functions.

deepak1556 commented 1 month ago

I have a feeling that it might be related to the io_uring backend, lets wait for the next electron update that has my change to make the backend opt-in and evaluate the impact on linux. For other platforms, need to investigate.

bpasero commented 1 month ago

@deepak1556 good point!

Code loading from renderer did not regress:

image

bpasero commented 1 month ago

At least the main bundle code caching file seems almost the same, though slightly larger, comparing stable to insiders:

Screenshot 2024-05-15 at 15 22 48

deepak1556 commented 1 month ago

--perf-prof on linux shows impact from cached data read operation but it is not consistent across runs, I will investigate this further on macOS or windows for better results.

-   44.24%     0.00%  code-insiders    libc.so.6                      [.] __libc_start_call_main                                                                                ◆
   - __libc_start_call_main                                                                                                                                                     ▒
      - 15.86% uv_run                                                                                                                                                           ▒
         - 5.50% v8::Function::Call                                                                                                                                             ▒
            - 2.40% Builtin:JSEntry                                                                                                                                             ▒
               - 2.38% Builtin:JSEntryTrampoline                                                                                                                                ▒
                  - 1.62% JS:~readFileAfterClose node:internal/fs/read/context:46:28                                                                                            ▒
                     + 1.60% JS:~ /home/parallels/Downloads/VSCode-linux-x64/resources/app/out/main.js:5:3976
deepak1556 commented 1 month ago

I have updated Electron with the change that disables io_uring backend yesterday, would like to compare the perf results of linux in the next run with today's insider.

bpasero commented 1 month ago

Unfortunately the updated Electron version did not improve the situation:

image

bpasero commented 1 month ago

@deepak1556 I wonder if this node.js change that came in 20.10.0 might have an impact: https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V20.md#vm-fix-v8-compilation-cache-support-for-vmscript

image

deepak1556 commented 1 month ago

Oh nice find! @bpasero

https://github.com/nodejs/node/commit/1d220b55ac397f15758e83d1143789608e3fca4a seems like a general perf win we should backport to Electron 29. I will take care of it this week.

However it seems to address a regression that was introduced in Node.js 18.x and doesn't answer the regression we are seeing after update to Node.js 20.9.0.

bpasero commented 3 weeks ago

Using chrome tracing there is no evidence that the cached script fails to load. The size of the scripts increased on disk (about 10%) which could explain slower loading times.

Closing as non actionable.

deepak1556 commented 3 weeks ago

Hey @bpasero I plan to investigate this a bit more for better data points around cache data serialization cost before and after the update, to confirm if it is purely code size increase or something else. Will reopen for that.

jrieken commented 3 weeks ago

The size of the scripts increased on disk (about 10%) which could explain slower loading times.

Sceptical since our overall code size usually increases around 5-10% each release