-
Notifications
You must be signed in to change notification settings - Fork 29
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
Optimize plot re-rendering #3337
Conversation
…igger re-rendering
…code-dvc into optimize-snappoints
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 good.
Is there any way to generate some kind of before/after report with wdyr?
webview/src/plots/components/templatePlots/templatePlotsSlice.ts
Outdated
Show resolved
Hide resolved
const nbRevisions = | ||
(multiView && | ||
plotDataStore[Section.TEMPLATE_PLOTS][plot].revisions?.length) || | ||
1 |
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.
[Q] Why set this to || 1
?
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.
I didn't change this nbRevisions
tells the plot the number of column it should span, if it's multiview, it spans the number of revisions it has. If it's not multiview, we can say it contains one revision. I'll change the name of the variable to colSpan
or something.
action: PayloadAction<number> | ||
) => { | ||
state.maxPlotSize = action.payload | ||
state.snapPoints = [ |
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.
[Q] Can we make this a function to capture what is going on. E.g
buildSnapPoints = (maxWidth: number): SnapPoints =>
(Took me a while to work it out)
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.
I should have read the description properly... but it won't be right there embedded in the code
Code Climate has analyzed commit ad2428e and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.0% (0.0% change). View more on Code Climate. |
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.
As someone with a VSCode that runs on the slower side, I could see how working with plots is indeed faster! Great work!
Demo on main with wdyrScreen.Recording.2023-02-23.at.9.56.37.AM.movDemo now with wdyrScreen.Recording.2023-02-23.at.9.49.50.AM.movAs you can see all useless re-rendering were dropped for first render and for re-sizing. There could still be an optimization for re-ordering plots and adding/removing. |
Part of #3334
Demo
Screen.Recording.2023-02-22.at.2.28.33.PM.mov
It's hard to see any improvements currently with main because the demo is still fairly fast. But we can see that everything is still the same behaviour wise.I was working with the demo before the scatter plot optimization and I did see a bit of improvement. There is still some work to do, but this is the big part of the refactor.
Main change here, we do no drill down the content of the plots anymore, we use an id and get the plot from the id at the last layer (
ZoomablePlot
). Other change here, instead of saving themaxWidth
and calculating the snap points of each section, we save the snap points directly and use this everywhere as they were always the same and were also drilled down.