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

[perf] consider to yield when creating workbench contributions #169937

Closed
jrieken opened this issue Dec 23, 2022 · 4 comments
Closed

[perf] consider to yield when creating workbench contributions #169937

jrieken opened this issue Dec 23, 2022 · 4 comments
Assignees
@bpasero
Labels
insiders-released Patch has been released in VS Code Insiders perf
Milestone
January 2023

Comments

@jrieken
Copy link
Member

jrieken commented Dec 23, 2022 •
edited

We have many workbench contributions for the phase Restored and that's visible in telemetry. The median times are quite bad - half our users "wait" for longer than 80ms.

Screenshot 2022-12-23 at 17 50 16

IMO we should consider to yield while this is happening, e.g instantiate contributions for some time (15 or 30ms) and then schedule a task for more creation work
@jrieken jrieken added the perf label Dec 23, 2022
@bpasero
Copy link
Member

bpasero commented Dec 27, 2022

@jrieken open for suggestions, the slight risk is that a contribution will no longer be guaranteed to be instantiated when layout is restored, but later.

For the LifeCyclePhase.Eventually we already have a concept of pushing the instantiation out to idle:

vscode/src/vs/workbench/browser/workbench.ts

Lines 432 to 436 in 229c95c

// Set lifecycle phase to `Eventually` after a short delay and when idle (min 2.5sec, max 5sec)
const eventuallyPhaseScheduler = this._register(new RunOnceScheduler(() => {
this._register(runWhenIdle(() => lifecycleService.phase = LifecyclePhase.Eventually, 2500));
}, 2500));
eventuallyPhaseScheduler.schedule();

Would you suggest doing the same for LifecyclePhase.Restored?

@jrieken
Copy link
Member Author

jrieken commented Jan 3, 2023

I had something like this or a static allowed time (20ms) in mind so that instantiating all contributions doesn't block for too long. And yes there is a risk of the behavioural change because some extension can become instantiated slightly later but I believe that's manageable

bpasero added a commit that referenced this issue Jan 6, 2023
@bpasero
[perf] consider to yield when creating workbench contributions (fix #…
44de5f1 
…169937)
@bpasero
Copy link
Member

bpasero commented Jan 7, 2023 •
edited

@jrieken one thing I just realise is that we seem to be marking code/didStartWorkbench only after setting lifecycleService.phase = LifecyclePhase.Restored:

vscode/src/vs/workbench/browser/workbench.ts

Lines 429 to 451 in 70c45ba

// Set lifecycle phase to `Restored`
lifecycleService.phase = LifecyclePhase.Restored;
// Set lifecycle phase to `Eventually` after a short delay and when idle (min 2.5sec, max 5sec)
const eventuallyPhaseScheduler = this._register(new RunOnceScheduler(() => {
this._register(runWhenIdle(() => lifecycleService.phase = LifecyclePhase.Eventually, 2500));
}, 2500));
eventuallyPhaseScheduler.schedule();
// Update perf marks only when the layout is fully
// restored. We want the time it takes to restore
// editors to be included in these numbers
function markDidStartWorkbench() {
mark('code/didStartWorkbench');
performance.measure('perf: workbench create & restore', 'code/didLoadWorkbenchMain', 'code/didStartWorkbench');
}
if (this.isRestored()) {
markDidStartWorkbench();
} else {
this.whenRestored.finally(() => markDidStartWorkbench());
}

As such, we seem to actually be waiting for each workbench contribution that is registered on LifecyclePhase.Restored to be instantiated before marking the time. Maybe this could explain a bit of the variance we see with the perf bot, shouldn't we immediately mark code/didStartWorkbench and only then set the phase to Restored?

@bpasero
Copy link
Member

bpasero commented Jan 7, 2023

Oh interesting, looks like we are lucky given this then usage:

vscode/src/vs/workbench/common/contributions.ts

Line 91 in 70c45ba

lifecycleService.when(phase).then(() => this.doInstantiateByPhase(instantiationService, logService, environmentService, phase));

Even though calling lifecycleService.phase = LifecyclePhase.Restored; will emit the event that the phase is reached, given we await a Promise the contributions are instantiated only after.

Anyway, I would maybe still make this more explicit and move the code that marks code/didStartWorkbench earlier?

@bpasero bpasero closed this as completed in c1f4c0c Jan 9, 2023
@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Jan 9, 2023
@VSCodeTriageBot VSCodeTriageBot added this to the January 2023 milestone Jan 9, 2023
@VSCodeTriageBot VSCodeTriageBot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jan 10, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bpasero bpasero

Labels
insiders-released Patch has been released in VS Code Insiders perf
Projects
None yet
Milestone
January 2023
Development

No branches or pull requests
3 participants
@bpasero @jrieken @VSCodeTriageBot