[Performance][Gantt chart] Throttle updates #22723
Conversation
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 1663426. |
| const end = Date.now(); | ||
|
|
||
| // Multiply by 4 so that if this is slow to compute we don't use up all of the wall time. | ||
| delayRef.current = Math.max((end - start) * 4, 32); |
There was a problem hiding this comment.
I could probably jack this up even higher. Maybe 6? I don't see much downside to doing it, it would just make the page even more responsive.
There was a problem hiding this comment.
Might be worth pulling these numbers out into named consts and adding some context with comments -- it's a bit hard to understand why these were the values chosen.
There was a problem hiding this comment.
4 was chosen because that's what we used in queryRefresh. 32 was chosen because that corresponds to 30fps which is a reasonably smooth frame rate for the UI.
There was a problem hiding this comment.
+1 for names here that inline your explanations - maybe const MIN_DELAY_30FPS = 32 and const PREFERRED_DELAY_MULTIPLIER = 4 (or something? that one is kinda hard to name)
There was a problem hiding this comment.
No objection to being more aggressive about memoizing components, as long as it's having the desired effect. (Some long props lists in there...)
Will let @bengotow take a look too, since I think he probably knows this code better than I do.
| const end = Date.now(); | ||
|
|
||
| // Multiply by 4 so that if this is slow to compute we don't use up all of the wall time. | ||
| delayRef.current = Math.max((end - start) * 4, 32); |
There was a problem hiding this comment.
Might be worth pulling these numbers out into named consts and adding some context with comments -- it's a bit hard to understand why these were the values chosen.
| metadata || EMPTY_RUN_METADATA, | ||
| scale, | ||
| nowMs, | ||
| const layout = useThrottledMemo( |
There was a problem hiding this comment.
This looks good to me, but I'm scared that this creates scenarios where a box can exist in props.layout and it does NOT exist in the adjusted layout for several renders (until this throttle fires). I don't know offhand that that breaks anything, but it's not really a case this code was tested for.
Before this merges, it'd be nice to write a bit of code that takes one of the boxes in the returned layout and deletes it, just to see what happens to the remainder of the rendering downstream.
I think this use case may happen in the wild with dynamic fan-out of ops where we create the boxes as the logs appear that reference them.
There was a problem hiding this comment.
This looks good to me, but I'm scared that this creates scenarios where a box can exist in props.layout and it does NOT exist in the adjusted layout for several renders (until this throttle fires). I don't know offhand that that breaks anything, but it's not really a case this code was tested for.
I read through the code a few times while adding this and there shouldn't be any issue here because props.layout isn't passed to any other part of the UI so there's no chance for there to be a parallel and inconsistent data structure.
| const end = Date.now(); | ||
|
|
||
| // Multiply by 4 so that if this is slow to compute we don't use up all of the wall time. | ||
| delayRef.current = Math.max((end - start) * 4, 32); |
There was a problem hiding this comment.
+1 for names here that inline your explanations - maybe const MIN_DELAY_30FPS = 32 and const PREFERRED_DELAY_MULTIPLIER = 4 (or something? that one is kinda hard to name)
…throttle-gantt-chart-updates
Summary & Motivation
For very large GanttChart's we end up computing a very expensive layout very frequently causing us to flood the mainthread with JS execution and making the page unresponsive. This PR throttles the updates to keep the page responsive. To do this I added a new feature to
useThrottledMemowhere leaving out the delay argument results in a dynamic delay based on how long the execution of the factory takes. Right now we multiply however long it takes by 4 (chosen because we use 4 in queryRefresh) with a maximum of 30fps.Also memoized a bunch of stuff to make the frames faster.
Also
no-cacheon the logs query since it can return 10,000 entries at once, and on the RunAssetsQuery since it's unbounded and in this case returning a couple thousand assetsHow I Tested These Changes
Before (barely any idle time):
After (lots of idle time) (longest frame is 9ms, most frames are 3ms):
