Skip to content
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 the plots sections to reduce the number of useless re-renderings #3341

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Feb 23, 2023

Part of #3334

Demo before

Screen.Recording.2023-02-23.at.4.06.15.PM.mov

Demo now

Screen.Recording.2023-02-23.at.3.54.24.PM.mov

Not a big improvement, but this should add up with the number of plots and it should make the code a little easier to read.

@sroy3 sroy3 self-assigned this Feb 23, 2023
const currentSize = useSelector((state: PlotsState) => state.template.size)
const entries = useSelector(
(state: PlotsState) => state.template.sections[groupIndex].entries
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By not passing the entries as a prop, a section won't update when another section does.

@sroy3 sroy3 marked this pull request as ready for review February 23, 2023 21:19
@codeclimate
Copy link

codeclimate bot commented Feb 23, 2023

Code Climate has analyzed commit 4d0eef2 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.

(sections: PlotGroup[]) => {
/* Although the following dispatch duplicates the work the reducer will do when the state returns
from the extension, this is necessary to not see any flickering in the order as the returned state
sometimes takes a while to come back */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@mattseddon
Copy link
Member

mattseddon commented Feb 23, 2023

[Q] Can we automate re-render performance testing in some way?

Very quick look around found this: testing-library/react-hooks-testing-library#135

@mattseddon mattseddon merged commit de148ed into main Feb 23, 2023
@mattseddon mattseddon deleted the optimize-plot-sections branch February 23, 2023 23:05
@sroy3
Copy link
Contributor Author

sroy3 commented Feb 24, 2023

[Q] Can we automate re-render performance testing in some way?

Very quick look around found this: testing-library/react-hooks-testing-library#135

I'll take a little bit of time to research this today. I think this could be very useful in the future. Even with virtualization, there is a lot of data rendered at once. Making sure we do not worsen re-rendering performance should be on own mind when developing in the plots webview (experiments table as well).

@sroy3
Copy link
Contributor Author

sroy3 commented Feb 24, 2023

Profiler API is probably the best tool for testing re-renders. The problem is that we don't want to test the number of re-renders for an isolated component and we do not want to profile simply the <App /> component either. What would be best, would be being able to wrap a certain component within the whole <App /> component. I'll see if it's possible to create a test helper method to solve that problem.

Might actually be able to use Why Did You Render directly for that as it does not need to wrap around a certain component. We can simply activate it with the name of the component, and there are options that could help with testing (mainly notifier which is basically a callback on re-render).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants