Open jrieken opened 4 months ago
Looks like this is coming from here: https://github.com/microsoft/vscode/blob/a49c81edea6647684eee87d204e50feed9c455f6/src/vs/workbench/contrib/notebook/browser/contrib/outline/notebookOutline.ts#L217
Setting up a listener per rendered element isn't great, it's better to have one and share it with the elements
This is the notebook I am testing with: slow.ipynb.txt
same/related https://github.com/microsoft/vscode/issues/211112
This seems to be a general issue for tree menus on large screens. When we support action contributions for tree elements, a menu is created for each visible tree element, and each menu registers a listener to HiddenStates change:
Since each tree menu registers an onChange event listener, the HiddenStates change emitter is no longer lazy.
This issue can be easily reproduced on a large screen with both the outline view and the timeline view opened for a relatively large notebook. Each tree has 50+ visible tree elements (with at least one menu for each), quickly hitting the threshold of 175.
The above analysis explains why this happens. I'm wondering how we can improve this given the constraint that each tree element has its own context, preventing the use of a shared event emitter/listener. One idea is to register the menu change listener lazily: update the actions and register the listener only when the user hovers over the item for the first time.
This seems to be a general issue for tree menus on large screens. When we support action contributions for tree elements, a menu is created for each visible tree element
Pretty sure @alexr00 already fixed this for contributed trees. The menu is only created when a tree element is active. Tho, it might be that the code bunny has spread this pattern onto other trees, document outline has no actions, unsure about notebook outline (tho the stacktrace won't lie) and timeline has.
One idea is to register the menu change listener lazily: update the actions and register the listener only when the user hovers over the item for the first time.
As said, this is kinda how it was fixed for custom trees. Not the menu is lazy but the menu isn't created to begin with
I ended up having to revert the fix for contributed trees as it broke tree checkboxes: https://github.com/microsoft/vscode/pull/217170
The tree doesn't actually care about the hidden states, so maybe we could resurrect https://github.com/microsoft/vscode/pull/214132
The tree doesn't actually care about the hidden states, so maybe we could resurrect https://github.com/microsoft/vscode/pull/214132
I don't think that helps - it means less listeners for hidden states but the equal amount of listeners for the context key service. Leak blaming isn't symmetric but sooner or later the menus/trees will be blamed for having to many CKS listeners
New, and I hope improved, contributed tree change coming with https://github.com/microsoft/vscode/pull/219964. My change won't impact this issue though as it's local to contributed trees. However, the ability to get menu contexts which I'm adding may be of use to you.
Testing #210961
347777655976bbc8460f833449592dd0a9a8f340