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.

Sign up for GitHub

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

Jump to bottom

Startup perf regression in window.loadUrl() => begin to require(workbench.desktop.main.js) #240767

Closed
bpasero opened this issue Feb 14, 2025 · 3 comments · Fixed by #243032
Closed

Startup perf regression in window.loadUrl() => begin to require(workbench.desktop.main.js) #240767

bpasero opened this issue Feb 14, 2025 · 3 comments · Fixed by #243032
Assignees
@deepak1556
Labels
bug Issue identified by VS Code Team member as probable bug electron-34-update freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues macos Issues with VS Code on MAC/OS X perf perf-startup unreleased Patch has not yet been released in VS Code Insiders upstream-issue-linked This is an upstream issue that has been reported upstream windows VS Code on Windows issues
Milestone
March 2025

Comments

@bpasero
Copy link
Member

bpasero commented Feb 14, 2025

The Electron 34 update seems to have made window opening slower again:

Image
@bpasero bpasero added electron-34-update freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues labels Feb 14, 2025
@bpasero bpasero added macos Issues with VS Code on MAC/OS X windows VS Code on Windows issues labels Feb 14, 2025
@deepak1556 deepak1556 added this to the February 2025 milestone Feb 14, 2025
This was referenced Feb 23, 2025
Closed
Draft
@deepak1556 deepak1556 added macos Issues with VS Code on MAC/OS X and removed macos Issues with VS Code on MAC/OS X labels Feb 23, 2025
@deepak1556
Copy link
Collaborator

deepak1556 commented Feb 23, 2025 •
edited
Loading

The regression comes from font loading. With Electron 34 they seem to happen early on the startup in response to an ipc blink.PageBroadcast.UpdateWebPreferences which adds around 50ms to startup path regression, specifically WebContentsImpl::Loading goes from 120ms to 170ms which matches the perf bot numbers.

Image

@deepak1556
Copy link
Collaborator

Based on the above I checked the stack that leads to the above calls,

electron.exe!content::DWriteFontCollectionProxy::FindFamilyName(std::__Cr::basic_string<char16_t,std::__Cr::char_traits<char16_t>,std::__Cr::allocator<char16_t> > const &,unsigned int *,int *) [dwrite_font_proxy_win.cc : 159 + 0x40]
 6  electron.exe!content::DWriteFontCollectionProxy::FindFamilyName(wchar_t const *,unsigned int *,int *) [dwrite_font_proxy_win.cc : 150 + 0x11]
 7  electron.exe!SkFontMgr_DirectWrite::onMatchFamily(char const * const) [SkFontMgr_win_dw.cpp : 341 + 0x17]
 8  electron.exe!SkFontMgr::matchFamily(char const * const) [SkFontMgr.cpp : 106 + 0x12]
 9  electron.exe!SkFontMgr_Custom::onMatchFamilyStyle(char const * const,SkFontStyle const &) [SkFontMgr_win_dw.cpp : 352 + 0xf]
10  electron.exe!SkDrawable::getBounds() [SkDrawable.cpp : 72 + 0xd]
11  electron.exe!gfx::FontList::FirstAvailableOrFirst(std::__Cr::basic_string<char,std::__Cr::char_traits<char>,std::__Cr::allocator<char> > const &) [font_list.cc : 270 + 0x2f]
12  electron.exe!blink::FontCache::FirstAvailableOrFirst(WTF::String const &) [font_cache.cc : 230 + 0x1a]
13 electron.exe!blink::GenericFontFamilySettings::GenericFontFamilyForScript(WTF::HashMap<int,WTF::AtomicString,WTF::IntHashTraits<int,-1,-3>,WTF::HashTraits<WTF::AtomicString>,WTF::PartitionAllocator> const &,UScriptCode) [generic_font_family_settings.cc : 98 + 0x14]
14  electron.exe!blink::GenericFontFamilySettings::UpdateStandard(WTF::AtomicString const &,UScriptCode) [generic_font_family_settings.cc : 139 + 0x8]
15  electron.exe!blink::WebSettingsImpl::SetStandardFontFamily(blink::WebString const &,UScriptCode) [web_settings_impl.cc : 60 + 0x19]
16  electron.exe!blink::WebView::ApplyWebPreferences(blink::web_pref::WebPreferences const &,blink::WebView *) [web_view_impl.cc : 1549 + 0x104]
17  electron.exe!blink::WebViewImpl::UpdateWebPreferences(blink::web_pref::WebPreferences const &) [web_view_impl.cc : 3662 + 0xb]
18  electron.exe!blink::mojom::blink::PageBroadcastStubDispatch::Accept(blink::mojom::blink::PageBroadcast *,mojo::Message *) [page.mojom-blink.cc : 1573 + 0x18]

The updating webpreferences part early on the process start is not new, it has always been there. What is interesting are the calls to font cache that eventually leads to the direct write ipc calls which didn't happen so early with Electron 32.

It was then easy to find the recent changes in that code path and the behavior changed with the following feature enabled since Chromium 130 https://chromium-review.googlesource.com/c/chromium/src/+/5848769. The feature flag has now been removed since Chromium 132 https://chromium-review.googlesource.com/c/chromium/src/+/5964277 so we cannot disable it.

The crbug link associated with this feature is private, so I don't have much context on this feature. https://chromium-review.googlesource.com/c/chromium/src/+/5334708 is the change that originally added this feature. The gist seems that the font families are loaded early on startup to avoid relayout in prerenderers. The metrics validated are mostly related to page loads that happens after the browser starts hence it won't affect upstream. I will check if this affects PWA and see if I can bring back the feature flag to disable for embedders.

@deepak1556 deepak1556 modified the milestones: February 2025, March 2025 Feb 27, 2025
@deepak1556
Copy link
Collaborator

Upstream issue https://issues.chromium.org/issues/400473071

@deepak1556 deepak1556 added the upstream-issue-linked This is an upstream issue that has been reported upstream label Mar 4, 2025
@deepak1556 deepak1556 mentioned this issue Mar 10, 2025
Merged
@vs-code-engineering vs-code-engineering bot added the unreleased Patch has not yet been released in VS Code Insiders label Mar 11, 2025
@bpasero bpasero added the bug Issue identified by VS Code Team member as probable bug label Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deepak1556 deepak1556

Labels
bug Issue identified by VS Code Team member as probable bug electron-34-update freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues macos Issues with VS Code on MAC/OS X perf perf-startup unreleased Patch has not yet been released in VS Code Insiders upstream-issue-linked This is an upstream issue that has been reported upstream windows VS Code on Windows issues
Projects
None yet
Milestone
March 2025
Development

Successfully merging a pull request may close this issue.

chore: update [email protected] microsoft/vscode
2 participants
@bpasero @deepak1556