-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Use mergeFast in a hotspot #9654
Conversation
In profiling our app, we found that the usage of `merge` in `Text.js` was showing up as a hotspot. We've replaced this usage of `merge` with `mergeFast`. We've also updated `mergeFast` to use `merge` in dev mode so that developers can benefit from the extra validation it does on its arguments.
By analyzing the blame information on this pull request, we identified @JoelMarcey and @MattFoley to be potential reviewers. |
This is the first time I've been poked by Github bot, but this change looks good to me. The only suggestion I would have is to use |
It would be better either to use |
As per @ide's suggestion, mergeFast should behave the same in dev and production.
@ide you make a good point about dev vs production. I updated my PR to use |
@rigdern updated the pull request - view changes |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to Phabricator to review internal test results. |
5d7227a
In profiling our app, we found that the usage
of
merge
inText.js
was showing up as ahotspot. We've replaced this usage of
merge
with
mergeFast
.Test plan (required)
This change is used in my team's app.
Adam Comella
Microsoft Corp.