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

Cache Child Props #1314

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cache Child Props #1314

wants to merge 1 commit into from

Conversation

Xiot
Copy link
Contributor

@Xiot Xiot commented May 2, 2020

Cache the child props to prevent React from re-rendering unnessesarily.
The biggest culprits were the default accessors. The defaults for each
attribute are now computed once, and re-used when required.

@CLAassistant
Copy link

CLAassistant commented May 2, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Chris Thomas seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Xiot
Copy link
Contributor Author

Xiot commented May 2, 2020

Using the 5th graph on the showcase (Line Chart Canvas - in svg mode) I was able to increase the fps while hovering over the graph from about 5 fps to 60 fps.

To see this I changed the Plots section to just show the one graph. Otherwise the GPU renderer thread in chrome struggled to keep up.

@@ -56,7 +56,7 @@ test('data-utils #addValueToArray', t => {
test('data-utils #transformValueToString', t => {
t.deepEqual(transformValueToString(0), 0,
'Shouldn\'t transform the number value');
t.deepEqual(transformValueToString(new Date(0)), 'Thu Jan 01 1970',
t.deepEqual(transformValueToString(new Date(43200000)), 'Thu Jan 01 1970',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value is 12:00 PM which should play nicer with timezone differences.

@@ -187,6 +187,22 @@ class XYPlot extends React.Component {
}
};

_childPropCache = Object.create(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit hestiant to add a cache instead of relying on useMemo. How about we wait till the modernization to React 16.8 (#1316) and useMemo instead (after some refactoring of this prop cloning behavior perhaps to using context)?

Alternatively, if you'd like to try out what this looks like via the modernize branch. Feel free to go for it.

@Xiot Xiot mentioned this pull request Jun 4, 2020
@Xiot Xiot force-pushed the child-prop-cache branch 4 times, most recently from d747435 to a2e3f1c Compare June 9, 2020 16:52
Cache the child props to prevent React from re-rendering unnessesarily.
The biggest culprits were the default accessors.  The defaults for each
attribute are now computed once, and re-used when required.
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.

None yet

3 participants