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

Implement event-specific pooling for SyntheticEvent #10237

Merged

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Jul 20, 2017

WIP. I think this is mostly done, still looking at pooling in TopLevelCallbackBookKeeping

This PR removes PooledClass from the synthetic event system in favor of a simpler, event-specific pooling system.

  • It maintains the same pooling API (getPooled, release) to minimize churn in the event system.
  • Instead of PooledClass.addPoolingTo it now uses a helper addEventPoolingTo defined in the SyntheticEvent module
  • Removes pooling all-together from FallbackCompositionState. It was only being used in a single event plugin, and the way it's structured makes it so there's only ever a single instance anyway. This means we don't need any sort of pooling. Instead, the composition state is now tracked in a single object which can be initialized and reset.

@gaearon
Copy link
Collaborator

gaearon commented Jul 20, 2017

Nice! Can you also get it out of ReactDOMEventListener? Then we can move it to the Stack folder.

@aweary
Copy link
Contributor Author

aweary commented Jul 20, 2017

Yupp! That's the one last thing I have to do, will remove the WIP tag and ping when I get that done 😄

@jquense
Copy link
Contributor

jquense commented Jul 21, 2017

neat! What is the benefit of splitting these two out if I might ask.

@aweary
Copy link
Contributor Author

aweary commented Jul 21, 2017

@jquense there's some context here: #9325 The goal is to get rid of PooledClass all together in Fiber, in favor of a more focused approach. For example, we can now consider implementing a much more targeted pooling strategies for events (one instance per event type).

It does lead to a bit of duplication, but I also think it makes it clearer what's going on.

@aweary aweary force-pushed the remove-pooled-class-synthetic-events branch from 084f2f4 to d9f717d Compare July 21, 2017 14:55
@aweary
Copy link
Contributor Author

aweary commented Jul 21, 2017

We also end up with a slightly smaller build as well, since PooledClass already contained a bunch of duplication with the various poolers it exports.

edit: smaller react-dom bundle, but I guess a slightly larger react bundle, so I guess it evens out.

@gaearon
Copy link
Collaborator

gaearon commented Jul 21, 2017

slightly larger react bundle

Why?

@aweary
Copy link
Contributor Author

aweary commented Jul 21, 2017

I'm not sure, I'll look into it though.

@gaearon
Copy link
Collaborator

gaearon commented Jul 21, 2017

I wouldn't expect changes in your branch to affect the react size. It's just that we don't record sizes on every commit so they fluctuate. If you want an accurate measurement of your commit specifically, you can roll back to merge base, record sizes there, then apply your changes and record them again.

@aweary
Copy link
Contributor Author

aweary commented Jul 21, 2017

Ah yeah, you're right @gaearon, the current size report is just out of date. With this change the react size remains the same at 2759 and react-dom goes down from 40030 to 39828.

@gaearon
Copy link
Collaborator

gaearon commented Jul 21, 2017

Sounds good. Is this still WIP?

@aweary aweary changed the title [WIP] Implement event-specific pooling for SyntheticEvent Implement event-specific pooling for SyntheticEvent Jul 21, 2017
@aweary
Copy link
Contributor Author

aweary commented Jul 21, 2017

I think it's ready for review!

@aweary aweary requested review from jquense and nhunzaker July 21, 2017 17:57
@@ -22,6 +21,9 @@ var getEventTarget = require('getEventTarget');

var {HostRoot} = ReactTypeOfWork;

var CALLBACK_BOOKKEEPING_POOL_SIZE = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 10?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was default in the old code.

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

This is great. 👍

@gaearon
Copy link
Collaborator

gaearon commented Jul 21, 2017

I'm good with this but please verify that pooling works as expected by manual inspection. E.g. make a page listening to mousemove and verify the values are up to date but instances are reused. And that persist() still works.

@nhunzaker
Copy link
Contributor

nhunzaker commented Jul 22, 2017

I added some event pooling test fixtures:
http://pooled-event-fixtures.surge.sh/event-pooling

Curious what the best manual test of this is, but I found these two tests helpful when throwing breakpoints in the source.

Edit: I also sent a PR to @aweary's branch: aweary#1

@gaearon
Copy link
Collaborator

gaearon commented Jul 26, 2017

Could you please fix conflicts?

Brandon Dail added 5 commits August 1, 2017 15:02
The only module that uses FallbackCompositonState is BeforeInputEventPlugin. The way its structured means there can only be a single instance of FallbackCompositionState at any given time (stored in a local variable at the top-level) so we don't really need pooling here at all. Instead, a single object is now stored in FallbackCompositionState, and access (initializing, reseting, getting data) is gaurded by the exported helper object.
@aweary aweary force-pushed the remove-pooled-class-synthetic-events branch from 175d201 to ceee4ca Compare August 1, 2017 20:05
@aweary
Copy link
Contributor Author

aweary commented Aug 1, 2017

@gaearon conflicts resolved 🚀

@gaearon
Copy link
Collaborator

gaearon commented Aug 2, 2017

Hmmm. CI still failing.

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2017

This looks good to me. Previously, only Prettier failed so I’m merging it without waiting for CI. (I fixed Prettier).

@gaearon gaearon merged commit 755724a into facebook:master Aug 4, 2017
@aweary
Copy link
Contributor Author

aweary commented Aug 4, 2017

Thanks for fixing that @gaearon!

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2017

Thanks for doing this ❤️

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.

5 participants