-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
fix: Typing in notebooks is laggy #1977
Conversation
mofojed
commented
May 1, 2024
- 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.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing=true
is the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of the hook instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it to the hook.
|
||
/** | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.