Delay preview creation till it is actually needed. #15823
Conversation
till when it is actually needed
| { | ||
| delayPreviewControlTimer = new DispatcherTimer(DispatcherPriority.Background, Dispatcher); | ||
| delayPreviewControlTimer.Interval = TimeSpan.FromMilliseconds(300); | ||
| delayPreviewControlTimer.Tick += DelayPreviewControlAction; |
There was a problem hiding this comment.
can you unsubscribe this handler somewhere that makes sense - maybe inside the DelayPreviewControl Action handler?
There was a problem hiding this comment.
or in OnNodeViewMouseLeave maybe.
There was a problem hiding this comment.
It's a static timer and there's only going to be one instance per process. Plus the handler does not create any new references. Is it really necessary to unsubscribe it?
There was a problem hiding this comment.
yes, unless you want to prove that it doesn't cause leaks / new retention paths with a memory profiler when run against our serial test suite.
There was a problem hiding this comment.
Got it. I think it makes the most sense to put it inside the RemoveViewModelsubscriptions method of the workspace view
| /// node views, instead of each having its own timer with additional overhead. | ||
| /// </summary> | ||
| private static DispatcherTimer delayPreviewControlTimer = null; | ||
| private static NodeView previewControlHost = null; |
There was a problem hiding this comment.
I am not so sure the previewControlHost is necessary.
There was a problem hiding this comment.
it's necessary if we want to have a single timer as a shared resource between nodes. We use that field to store a ref to the last hovered node and we clear it at the end of the timer event action.
It's not necessary with the action debouncer
| } | ||
|
|
||
| // Always set old ZIndex to the last value, even if mouse is not over the node. | ||
| oldZIndex = NodeViewModel.StaticZIndex; |
There was a problem hiding this comment.
it looks like now we are setting the oldZIndex in more places than before.
Is it intentional ? could it cause issues?
There was a problem hiding this comment.
no, it's exactly the same as before - twice. I only moved that code from the TryShowPreviewBubbles to one level up in the new InitialTryShowPreviewBubble. That's because the nested method will only run conditionally but we need to get the stored Z height every time. Every time the mouse leaves the node control, the Z height gets set again and if we don't first read it, it defaults to -1(or was it 0?) and the node ends up hidden behind everything else.
| /// needed. It is static so that it can be shared for all node views, instead of | ||
| /// each node having its own debouncer with additional overhead. | ||
| /// </summary> | ||
| private static ActionDebouncer delayPreviewControl = null; |
There was a problem hiding this comment.
An alternative to having a static that you need to manage for every WorkspaceModel change would be to store the delayPreviewControl debouncer as an internal (non-static) member inside the WorkspaceViewModel class.
That way you can better control the life cycle (initialize on workspace constructor and cleanup on dispose).
NodeView has access to the workspace view model (ex. viewModel?.WorkspaceViewModel?.delayPreviewControl?.Debounce) and you still get a single instance per Workspace
There was a problem hiding this comment.
Great idea, it worked just as well and produced better structured code.
|
I think we should wrap this behavior under a feature flag. |
Purpose
Node preview bubbles, while incredibly useful, eat up a lot of resources the first time they are instantiated. Currently, when a graph loads the previews are not initialized (great!), but as soon as the mouse passes over the node, even for a fraction of a second, you have to pay the loading cost (not great :( ). Let's say you're just trying to pan a large graph from one side to the other. You're just trying to navigate, to a different spot in the graph and for some reason there are constant spikes of lag.
This PR tries to resolve the problem in two ways - first, we make sure that just passing over a node with the mouse does not immediately instantiate the preview. Instead we start a short timer and initialize the preview only if the mouse focus stays until it fires. Secondly, we make sure the state machine is updated correctly when you pan the graph with middle mouse button (which is by far the most common navigation method; I don't have the numbers but I'm sure a lot more people use the MMB instead of the pan switch in the top right corner) and disable all popups during panning.
Metrics
When trying to quickly navigate the MaRS graph (~700 nodes) from start to finish, preview instantiation can easily take up over 10% of the CPU time:
We can completely negate that with the proposed updates (I did get an extra ping in the second run because I was a bit slower and hovered over a node for too long):
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
@pinzart90 , @mjkkirschner
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of