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: optimize isHydrating #2562

Merged
merged 1 commit into from
Nov 3, 2021
Merged

Conversation

nolanlawson
Copy link
Collaborator

Details

#2442 introduced a small performance regression, notably in the dom-table-create-10k benchmark. After tracking it down, most of the regression can be reduced to this simple change.

My hunch is that adding the get isHydrating changes the object shape in such a way that V8 has a harder time optimizing the other functions on the object, which are frequently called. So switching from a getter to a regular function actually gives us a decent performance win across most of the benchmarks:

Screen Shot 2021-11-02 at 12 19 58 PM

(The above is comparing this PR versus current master – 100 sample size, 1% horizon, 20 minute timeout.)

Interestingly, the dom-light-styled-component-create-1k-same benchmark does indeed seem to be consistently regressed, by around ~2%. (I ran the test again to confirm.) But I'd say it's worth it, given it's small (~1ms) and the other benchmarks are mostly improved.

A further optimization would be to get rid of the renderer object entirely. It only exists so that we can have a nice TypeScript abstract class for the DOM/server renderers. Potentially, we could just hoist all of its properties into the top-level exports on a module.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

GUS work item

W-10099550

@nolanlawson nolanlawson merged commit 8e9c84d into master Nov 3, 2021
@nolanlawson nolanlawson deleted the nolan/optimize-is-hydrating branch November 3, 2021 14:08
@caridy
Copy link
Contributor

caridy commented Nov 8, 2021

This is very interesting. We should probably do an analysis of how many of those getters do we have in internal records.

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.

4 participants