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

Replace ReactPerf with new implementation #6647

Merged
merged 11 commits into from
May 5, 2016
Merged

Replace ReactPerf with new implementation #6647

merged 11 commits into from
May 5, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 28, 2016

This removes the old ReactPerf implementation and replaces it with the one from #6046.
Corresponding React Native PR: facebook/react-native#7283

To be exact:

  • The old ReactPerf, ReactDefaultPerf, and ReactPerfAnalysis are removed.
  • ReactDebugTool (added in Add new ReactPerf #6046) takes care of measurements, and ReactPerfAnalysis is renamed to just ReactPerf.

@ghost
Copy link

ghost commented Apr 29, 2016

@gaearon updated the pull request.

@@ -91,14 +90,6 @@ function inject() {
);

ReactInjection.Component.injectEnvironment(ReactComponentBrowserEnvironment);

if (__DEV__) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This block has moved to ReactDebugTool.

@ghost
Copy link

ghost commented Apr 29, 2016

@gaearon updated the pull request.

3 similar comments
@facebook-github-bot
Copy link

@gaearon updated the pull request.

@ghost
Copy link

ghost commented Apr 29, 2016

@gaearon updated the pull request.

@ghost
Copy link

ghost commented Apr 29, 2016

@gaearon updated the pull request.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 29, 2016

@sebmarkbage

This is ready for review now, and has tests salvaged from the previous implementation.
I tested this with facebook/react-native#7283, and it works well.

@gaearon gaearon changed the title Delete the old ReactPerf Replace ReactPerf with new implementation Apr 29, 2016
gaearon added 11 commits May 3, 2016 14:21
Technically this shouldn't happen but it seems possible with ReactNativeMount.unmountComponentAtNode().
For now, let's just ignore these lifecycle events because ReactPerf makes a hard assumption that all lifecycle hooks happen inside batches.
We can revisit later when we have a comprehensive test suite for ReactPerf itself.
It is necessary to exclude just mounted components from wasted calculation.
@gaearon
Copy link
Collaborator Author

gaearon commented May 3, 2016

Rebased on the current master.
@sebmarkbage Can you please take a look until this gets merge conflicts again? 😄

@ghost
Copy link

ghost commented May 3, 2016

@gaearon updated the pull request.

@sebmarkbage
Copy link
Collaborator

What's the summary of the current state of the new perf? Where have it been deployed? As anyone other than us tested it?

Seems good to at least have some eyes on it before we kill the old one forever, no?

On May 3, 2016, at 6:22 AM, Dan Abramov notifications@github.com wrote:

Rebased on the current master.
@sebmarkbage Can you please take a look until this gets merge conflicts again? 😄


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@gaearon
Copy link
Collaborator Author

gaearon commented May 3, 2016

I tested it on some of the open source apps/examples that I maintain. The results are consistent with what I’d expect them to be. Internally we don’t really use the console version of ReactPerf so I’m not sure what you mean by having it deployed.

Has anyone other than us tested it?

No, but we can cut 15.1.0-alpha and ask people to try.

Seems good to at least have some eyes on it before we kill the old one forever, no?

Sure, I agree. I still think it’s safe to land in master because if we find bugs, we can add more test cases for them. I don’t think we’ll find bugs though until we release this in some form.

@gaearon
Copy link
Collaborator Author

gaearon commented May 4, 2016

Ping @sebmarkbage, anything I can do on my side?

@sebmarkbage
Copy link
Collaborator

Ok. Hope this works! :)

@gaearon gaearon merged commit b6a6078 into facebook:master May 5, 2016
@gaearon gaearon deleted the bye-bye-reactperf branch May 5, 2016 01:57
@gaearon
Copy link
Collaborator Author

gaearon commented May 5, 2016

Let’s cut 15.1.0-alpha with this? I have some people in mind who might help us test it!
cc @slorber

@slorber
Copy link
Contributor

slorber commented May 5, 2016

I'd like to @gaearon but won't be there for the next 2 weeks :)

zpao pushed a commit that referenced this pull request May 10, 2016
Replace ReactPerf with new implementation
(cherry picked from commit b6a6078)
@zpao zpao modified the milestones: 15.1.0, 15.y.0 May 20, 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.

None yet

5 participants