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

Cost of IContextKeyService#onDidChangeContext #71642

Closed
1 of 4 tasks
jrieken opened this issue Apr 3, 2019 · 5 comments
Closed
1 of 4 tasks

Cost of IContextKeyService#onDidChangeContext #71642

jrieken opened this issue Apr 3, 2019 · 5 comments
Assignees
@jrieken
Labels
debt Code quality issues perf-startup
Milestone
August 2019

Comments

@jrieken
Copy link
Member

jrieken commented Apr 3, 2019 •
edited

Context keys have become popular, they drive keybindings, menus, and views. The latter two depend on events because they are stateful and need to update when context keys change. Simple instrumentation shows that during startup the event fires ~233 times and costs around 90ms (on my early 2015 MBP). Observations

  • each ViewDescriptorCollection (explorer, search, scm, etc. etc) listens to the event no matter if it's visible or not cc @sandy081
  • there is no way to emit a batch event, e.g producers call myKey.set(value). For instance, EditorModeContext#_update might trigger 19 events, instead of one
  • most events are emitted during lifecycle phases Starting and Ready, so often before the UI shows much. Should the service buffer events till Restored?
  • half of the events are triggered by context key creation and setting their default value. Should we not emit an event then?
@jrieken jrieken added debt Code quality issues perf-startup labels Apr 3, 2019
@jrieken jrieken added this to the April 2019 milestone Apr 3, 2019
jrieken added a commit that referenced this issue Apr 3, 2019
@jrieken
add PauseableEmitter to support #71642
1c1ff99
jrieken added a commit that referenced this issue Apr 3, 2019
@jrieken
adopt PauseableEmitter in ContextKeyService and for EditorModeContext, …
ae83621 
…#71642
jrieken added a commit that referenced this issue Apr 3, 2019
@jrieken
scoped context key services fire on their own emitter but expose a co…
25f9be4 
…mposite event, #71642
@jrieken
Copy link
Member Author

jrieken commented Apr 4, 2019

I have pushed and adopted IContextKeyService#bufferChangeEvents and I also fixed an interesting issue that would trigger events from child/scoped context services on its parent. With those changes, the event still fires 233 times but less listeners are reached and only ~30ms is spent processing them.

I will push some more tweaks to the event itself but not do the event-pausing on lifecycle events.

@jrieken jrieken removed their assignment Apr 4, 2019
@jrieken
Copy link
Member Author

jrieken commented Apr 4, 2019

@sandy081 I leave the ViewDescriptorCollection to you

@sandy081 sandy081 modified the milestones: April 2019, May 2019 Apr 29, 2019
@sandy081 sandy081 modified the milestones: May 2019, June 2019 May 24, 2019
@sandy081 sandy081 modified the milestones: June 2019, July 2019 Jun 25, 2019
@sandy081
Copy link
Member

@jrieken I looked into this a bit deeply to see if I can start listening to context only when viewlet is visible. But there is a usecase in activity bar to hide viewlet icon if it has no active views. So this needs me to build the model by reading and listening to context keys.

@sandy081
Copy link
Member

Since no action can be taken un-assigning myself.

@sandy081 sandy081 assigned jrieken and unassigned sandy081 Jul 30, 2019
@jrieken jrieken modified the milestones: July 2019, August 2019 Jul 30, 2019
@jrieken jrieken closed this as completed Jul 30, 2019
@jrieken
Copy link
Member Author

jrieken commented Jul 30, 2019

no action then

@vscodebot vscodebot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jrieken jrieken

Labels
debt Code quality issues perf-startup
Projects
None yet
Milestone
August 2019
Development

No branches or pull requests
2 participants
@jrieken @sandy081