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

perf: TableGraphTable: tweak rendering internal state #17521

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

hoorayimhelping
Copy link
Contributor

@hoorayimhelping hoorayimhelping commented Apr 1, 2020

Rendering graphs in table view is kind of painful. Scrolling is a bit rough. This is a quick, small, low hanging fruit.

Moved two fields out of React state into internal component state this.state.foo ➡️ this.foo so that changing them doesn't force re-renders needlessly.

Here is a comparison of the flamegraphs using React's profiling tools. These flame graphs represent scrolling the table at a normal speed. The graphs take place during the same slice of time - the start of the table scroll.

master
master

branch
branch

Not a yuge win, but definitely less bursty on the flame graph. A couple of these types of small, easy wins will cumulatively make this feel much better.

I can share the performance .json files if you'd like to take a look yourself.

@hoorayimhelping hoorayimhelping requested review from desa and a team April 1, 2020 00:54
@hoorayimhelping hoorayimhelping force-pushed the bs_table_scrolling branch 3 times, most recently from 6f203e3 to 7f4afbc Compare April 1, 2020 17:23
fields that are often re-updated but rarely change value were moved out of react state
and into internal component state. changing these values won't force re-renders.
@hoorayimhelping hoorayimhelping merged commit 83c0f29 into master Apr 1, 2020
@hoorayimhelping hoorayimhelping deleted the bs_table_scrolling branch April 1, 2020 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants