Skip to content

Perf: Update editor configuration only after extensions are registered #138302

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

Merged
merged 2 commits into from
Dec 2, 2021

Conversation

sandy081
Copy link
Member

@sandy081 sandy081 commented Dec 1, 2021

Editor configurations are getting updated very aggressively (atleast 20 times) while the extensions are getting registered. This is consuming atleast 200ms.

Insiders (0cc0904c56) joh/pausedEvents 7907361 PR c4bd3f8
builtin extensions 185.6 71 (-114.6ms) 94.5 (-90.2ms)
builtin extensions and 30 more 448.8 148.2 (-300.6ms) 163.8 (-285ms)

Thanks to @jrieken for comparing this change with the current insiders and also with his generic solution.

Please see #138259 for more information and perf numbers.

This changes editor configuration to update only after extensions are registered, therefore configuration is being updated only once by this.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just registerWorkbenchContribution on the LifecyclePhase.Restored phase rather than waiting for extensions explicitly? If this is about startup, that should be fine. We can also think about introducing another phase signaling that extensions are registered, I am sure we would find more places.

@jrieken
Copy link
Member

jrieken commented Dec 2, 2021

Why not just registerWorkbenchContribution on the LifecyclePhase.Restored phase rather than waiting for extensions explicitly?

The Restored phase is reached before scanning extensions. The issue is that while registering extension contributions too many configuration changes are fired, esp. from custom editors and notebooks. This PR makes sure that editor-land triggers the event only once

@sandy081
Copy link
Member Author

sandy081 commented Dec 2, 2021

Main intention of this change is to reduce number of times this editor configuration updates configuration. I do not think LifecyclePhase.Restored is what when after extensions are registered. This editor auto lock configuration is listening to editor registrations and updating config, so the right phase is when extensions are registered. I think we can introduce new LifecyclePhase for extensions registered but is not this what IExtensionService.extensionService.whenInstalledExtensionsRegistered is and the new phase is going to be duplicate right.

@bpasero
Copy link
Member

bpasero commented Dec 2, 2021

I think we can introduce new LifecyclePhase for extensions registered

👍

but is not this what IExtensionService.extensionService.whenInstalledExtensionsRegistered

Yeah it is the same, but it would not duplicate anything, rather make it super easy for workbench contributions to register to this phase. That was the main intent of having lifecycle phases actually. Of course everyone could await promises but the phases avoid a lot of boilerplate code.

@bpasero bpasero merged commit cbab504 into main Dec 2, 2021
@bpasero bpasero deleted the sandy081/perf-delayUpdatingEditorConfigurations branch December 2, 2021 08:23
@jrieken jrieken changed the title Update editor configuration only after extensions are registered Perf: Update editor configuration only after extensions are registered Dec 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants