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

Do not allocate an array if no before input events #7691

Closed
wants to merge 1 commit into from

Conversation

nhunzaker
Copy link
Contributor

Mostly following up on #7642 (comment).

BeforeInputEventPlugin returns [null, null] a lot - I think every time an event dispatches. I also think this leads to a bunch of null/empty checks later down the event pipeline that could be eliminated, though I didn't address them here (maybe they would get easier to identify as Flow types trickle down).

This also saves an array on every single event dispatch, which may or not be substantial, but the fix is straightforward, so I thought I'd give it a try.

nativeEventTarget
);

return compose && before ? [compose, before] : compose ? compose : before;
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 surprised that changing the return type doesn't break any tests, do you know if there are cases in our test suite that returns [null, null]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[null, null] returns for just about every single event plugin test that executes the EventPluginHub.extractEvents code path.

Quickly scanning (throwing on [null, null]), this is 22 of 24 SimpleEventPlugin tests, 7 of 9 ChangeEvent plugin tests, and a handful of other event system tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if I'm interpreting the coverage correctly, BeforeInputEventPlugin has pretty weak coverage:

screen shot 2016-09-10 at 2 36 42 pm

screen shot 2016-09-10 at 2 35 50 pm

It's possible this is just really hard to test (keyboard stuff). Looks like neither event types (composition or beforeInput) ever instantiate.

Copy link
Contributor

@aweary aweary Sep 10, 2016

Choose a reason for hiding this comment

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

Got it. This is eventually passed to accumulateInto which accepts either an item or array of items, so it works just fine.

@aweary
Copy link
Contributor

aweary commented Sep 10, 2016

Have you measured any non-trivial performance/memory usage improvements with this? I'm hesitant to accept this unless there's a definite win, since it hurts readability a bit.

@nhunzaker
Copy link
Contributor Author

That's a good question. Technically it's a bit of extra memory rust, but I don't think it's worth it for a minor performance gain (Though I'd also be happy to decompress the ternary into an if/else for readability).

Mostly this just made things noisy when stepping through extractEvents and the change is trivial. Most people will probably never have to deal with that.

@nhunzaker
Copy link
Contributor Author

@aweary I'll try to capture something more concrete today. Beyond that, do you think it's worth getting test coverage for more of this module?

@ghost ghost added the CLA Signed label Sep 12, 2016
@nhunzaker
Copy link
Contributor Author

@aweary benchmarking this is kind of an interesting challenge for this, but it looks like what you'd expect: nothing substantial

This is partly intuition, but I wrote up a benchmark here https://github.com/nhunzaker/react-before-input-change-bench but it's not fantastic. Either way, nothing significant.

@ghost ghost added the CLA Signed label Sep 13, 2016
@nhunzaker
Copy link
Contributor Author

Feel free to close this one out, though I am sort of curious if this will come up if Flow types move down into more of the event system.

Thanks for the time, either way.

@aweary aweary closed this Oct 4, 2016
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