-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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 regression in insiders #177257
Comments
Looks like code loading times went (consistently) up with b60db78. The size of workbench-main didn't really change - it's 10.3MB for both versions. So, this is either slower code loading by electron (cc @deepak1556) or more code execution while loading |
change between those two builds: 502ac5b...b60db78 |
I can reproduce locally when running the builds in comparison. There is a slowdown on first run (which doesn't have cached data) and on subsequent runs. The slowdown seems constant which indicates towards a code execution problem
|
I also did some testing today. First of all, even on the macOS perf machine, we lost time in the I did compare startup profiles on macOS for the fast version on March 1st and then today but could not spot something obvious. My hope was that maybe the The findings around cached data are good, I did not specifically look into that. To my knowledge there was no Electron update, not even a minor one. But we should double check that for example no Profile-Guided-Optimization change sneaked into the insiders build. |
Runtime has not changed since
No change in this area as well |
@bpasero I'd like to add something that I've noticed in Insiders since around March 9-10, which might be related to the perf regression you've measured since then. My point here is that around March 9 (maybe a couple of days before) the popup stopped appearing in Insiders - mostly - not appearing for several hours, even days on end. However, I think the error still occurs in the background, without a notification being shown. As additional information, if you don't want to search the long thread, here's a typical example of a complete error notification appearing before March 9 in Insiders and also in the official VSCode 1.76 on a regular basis (ongoing since June 2022) #130320 (comment) I'm not sure if this has anything to do with the performance regression, I thought I'd mention it in case it proves to be somehow related. |
Starting to bisect this using OSS and vscode-perf. |
This is very nuanced... fluctuations are very small between commits... a bisect pointed to ef1542d, but I'm not entire sure of it either. The fact is that that commit range produced 864 (1,406 additions and 542 deletions) new lines of code... could this just be a lot of new code? |
I take it back. There seems to be no difference when running out of sources, I fear we might have to run out of builds from that commit range:
|
I think I am throwing the towel on this one. The closest I can find of an issue is that the task to evaluate and compile the workbench main script got slower. At the same time, It is also very much possible that new code we added now spends more time in the module factory. That would explain why overall we got slower, but when looking into the Attaching the profiles used for this measurement: |
👌 I suggest moving this out to March. |
April? 😄 |
Using One thing I had forgotten: we did update to Electron 22 on Feb 28 and reverted only on March 2nd. That explains why we had very fast runs and slower ones shortly after: I did a quick test of reverting a few commits that I found suspicious and experimented with startup times without much luck. One idea for next things to try is to run with cached data disabled just to see if this is cached data related, though I highly doubt that given my local testing. |
^ This might be it. I remember the Electron 22 Exploration builds (from Jan-Feb, before the few day stint in Insiders) seemed way snappier when testing, see #166265 (comment) and #166265 (comment) |
Ah yeah 👍 . We plan to bring Electron 22 back next week. |
Another idea is to print detailed loader stats in the perf view editor. This was for some reason disabled in 2092016 but I have 2 builds running (old and latest) to restore this momentarily for perf testing purposes. |
Almost
Also, loader status confirm the slower
The larger amount of time spend to load a library such as |
The slowdown in loading times especially around
Why would there be more than |
@bpasero Strange indeed... Don't want to nag or anything, but is it possible for this issue to have anything to do with my observation above (reproducible)? #177257 (comment) That issue has been going on for more than 9 months now and then it suddenly stops (mostly) at the beginning of March? I'm inclined to think the timing is not a coincidence. |
I am not sure, this performance regression happened over the course of 2 weeks I would say, not on a specific date. In my testing I see startup times getting slower and slower around the code loading and execution as outlined in comments above. Maybe I had misunderstood your reaction, but I had thought that the Electron 22 revert might have caused what you saw? Though that happened earlier in beginning of March. |
At least |
7f329fe should make things better, but we can keep this open a bit to verify. |
The latest Insiders 1.78 with Electron 22.4.3 is running great for me! Thank you @bpasero and @deepak1556 |
Overall ellapsed trend:
Evident increase in time to load workbench bundle:
The text was updated successfully, but these errors were encountered: