fix: Typing in notebooks is laggy #1977
Conversation
mofojed
commented
May 1, 2024
mofojed
commented
- We were checking the diff of the layouts on each keystroke
- DashboardLayout should debounce checking that diff
- Tested by displaying FPS counter, having a Code Studio with a bunch of tables in it, adding a notebook, and then typing in it. FPS is much higher after the change
- We were checking the diff of the layouts on each keystroke - DashboardLayout should debounce checking that diff - Tested by displaying FPS counter, having a Code Studio with a bunch of tables in it, adding a notebook, and then typing in it. FPS is much higher after the change
|
Does the layout contain the entire session or something in DHE? While it should be devounced, it still seemed like it was taking way too long on the deep equals. Are we deep comparing something huge? |
|
@mattrunyon this is slow in DHC as well, not just DHE.
2. IrisGridPanel exports state that it doesn't necessarily need to, we could use defaults for all this empty stuff. A little more concern about breaking something if changing this:
I'll file tickets for those two, but this should be debounced still. As the dashboard grows, doing a comparison of the layouts is going to take longer and longer, we can't be doing it every keystroke/every state update. |
|
And actually, we may want to throttle instead of debounce... otherwise if there's a panel that emits events more often than the debounce period, the dashboard will never save... |
- Otherwise it's possible for a dashboard to never save if it is constantly updating
|
That |
|
This seems like the perfect place to use a |
|
For reference, here's the JSON string of the layout that's being checked: Oddly, sometimes it seems to take 1ms and other times it's taking 40ms?? 🤔 |
|
It probably sucks more with multiple notebooks each with very large scripts in them. |
|
Also according to this benchmark, we are using one of the slowest deep-equal implementations. https://www.npmjs.com/package/fast-deep-equal#performance-benchmark |
|
So we already listen to an event here before comparing. Do we even need to compare the layout? How do you get that event to emit without the layout actually changing in some way that you want to save? We could just save on a throttle/debounce if the save is cheaper than the comparison. |
|
Or is that state changed a react state change? |
| @@ -228,6 +231,11 @@ export function DashboardLayout({ | |||
| } | |||
| }, [dehydrateComponent, isItemDragging, lastConfig, layout, onLayoutChange]); | |||
|
|
|||
| const throttledHandleLayoutStateChanged = useMemo( | |||
| () => throttle(handleLayoutStateChanged, STATE_CHANGE_DEBOUNCE_MS), | |||
There was a problem hiding this comment.
This will need the trailing option I think so the final call is always run. Not sure if that's a default or how the specifics work, but make sure the final call is always executed.
There was a problem hiding this comment.
trailing=true is the default.
There was a problem hiding this comment.
Looks like the default is leading=true and trailing=true. So I think it should be fine? But might want to add a flush call on unmount just in case there's a pending update
There was a problem hiding this comment.
Yep so this will currently call immediately, then again at the end of the timeout if the throttled function has been called again. flush will call whatever the last invoked was ignoring timeout and will not trigger a call at the end unless it has been invoked again.
So flush on unmount to ensure there's no queued saves
|
@dsmmcken |
Sorry, lame. Looks like it's in TP now at least. |
|
DEBOUNCE_PANEL_STATE_UPDATE is 400ms in the notebook panel, that could probably also be bumped up in time. |
|
@dsmmcken the |
|
Actually the |
- Need to use refs so that the callbacks don't change each time, and the flush/cancel are stable - Fun!
| useEffect( | ||
| () => () => throttledProcessDehydratedLayoutConfig.flush(), | ||
| [throttledProcessDehydratedLayoutConfig] | ||
| ); |
There was a problem hiding this comment.
Should this be part of the hook instead?
|
|
||
| /** | ||
| * Wraps a given callback in a cancelable, throttled function. The throttled | ||
| * callback is automatically cancelled if the callback reference changes or the |
There was a problem hiding this comment.
Something like this? Since flushOnUnmount also triggers with the callback changing
| * callback is automatically cancelled if the callback reference changes or the | |
| * callback is automatically cancelled or flushed if flushOnUnmount is set if the callback reference changes or the |
There was a problem hiding this comment.
So I don't think we want to actually flush or cancel when the callback reference changes, we only want to cancel or flush on unmount, and we want the throttled callback to always call the latest callback reference.
I've changed it to always return a stable callback... as I think the behaviour is too ambiguous/confusing if you try to introduce cancel/flush whenever the callback reference changes (do you want to cancel or flush? Should the throttle timer reset? etc).
There was a problem hiding this comment.
Also of note the useDebouncedCallback should probably be structured similarly, as right now it could continually cancel if the callback passed in changes and then never gets called.
