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

Make ReactPerf.start() work during reconciliation #7208

Merged
merged 7 commits into from
Jul 7, 2016
Merged

Make ReactPerf.start() work during reconciliation #7208

merged 7 commits into from
Jul 7, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 6, 2016

Currently ReactPerf.start() and ReactPerf.stop() calls don’t work correctly during reconciliation.
As far as I know this was occasionally the case before 15.x too—they weren’t working reliably.

Generally we advise that people call ReactPerf methods from the console but React components for this appears to be a popular pattern so it’s best we support it.

I took the failing test from #7191 and built this PR on top of it.
This should fix the problem so people can write “measurer” components like this:

    var Measurer = React.createClass({
      componentWillMount() {
        ReactPerf.start();
      },
      componentDidMount() {
        ReactPerf.stop();
      },
      componentWillUpdate() {
        ReactPerf.start();
      },
      componentDidUpdate() {
        ReactPerf.stop();
      },
      render() {
        // Force reconciliation despite constant element
        return React.cloneElement(this.props.children);
      },
    });

    var container = document.createElement('div');
    ReactDOM.render(<Measurer><App /></Measurer>, container);
    expect(ReactPerf.getWasted()).toEqual([]);

    ReactDOM.render(<Measurer><App /></Measurer>, container);
    expect(ReactPerf.getWasted()).toEqual(/* ... */);

Why didn’t this work before? We used to only start and stop timers while isProfiling is true. However this doesn’t work correctly if the user flips isProfiling during reconciliation. Then we have mismatching begin/end timer calls.

To fix this, I decided to always measure methods regardless of whether we are in profiling mode. I suppose calls to performanceNow() can’t be expensive, can they? And we are only doing this in __DEV__ anyway.

The logic is changed so that we don’t record the measurements if isProfiling is false. So we just make them and throw them away (without pushing them into the array). This lets us avoid writing the logic to recover from isProfiling flipping in the middle of reconciliation.

I also changed the host history tool to only record DOM operations while profiling. Previously they were recorded into an array before being discarded at the end of the flush.

Finally, I fixed a few places (ART and error boundaries) where warning assertions weren’t true. They started firing because we now measure regardless of isProfiling. This is actually good because it means we’re testing that ReactPerf doesn’t get confused by bad event ordering during our whole test suite.

This looks cleaner even though it is not strictly necessary.
We still call them manually for unmounting because it doesn't have a transaction.
@@ -315,10 +315,6 @@ var ReactMount = {
shouldReuseMarkup,
context
) {
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.

See last commit—I replaced most of these blocks with transactions. (I can revert just that commit if you’re not into this.)

@sophiebits
Copy link
Collaborator

lgtm. Do you have a test with the onError behavior?

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 7, 2016

Yes, if I remove this behavior ReactErrorBoundaries-test breaks because it causes erroneously emitted warnings. The reason it didn’t break before is because we didn’t run those assertions while not profiling. But now we run them in all tests, we just don’t use the actual profiles.

@gaearon gaearon merged commit 1a0e3a3 into facebook:master Jul 7, 2016
@gaearon gaearon deleted the reactperf-start-during-reconcile branch July 7, 2016 18:42
zpao pushed a commit that referenced this pull request Jul 8, 2016
* Add failing test demonstrating a ReactPerf warning

* Make the failing ReactPerf test more precise

* Make ReactPerf.start() work during reconciliation

* Reorder lifecycle methods for greater clarity

* Fix memory leak

* Error boundaries should not break ReactPerf

* Put onBeginFlush/onEndFlush into transaction wrappers

This looks cleaner even though it is not strictly necessary.
We still call them manually for unmounting because it doesn't have a transaction.

(cherry picked from commit 1a0e3a3)
@zpao zpao modified the milestones: 15-next, 15.2.1 Jul 8, 2016
usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016
* Add failing test demonstrating a ReactPerf warning

* Make the failing ReactPerf test more precise

* Make ReactPerf.start() work during reconciliation

* Reorder lifecycle methods for greater clarity

* Fix memory leak

* Error boundaries should not break ReactPerf

* Put onBeginFlush/onEndFlush into transaction wrappers

This looks cleaner even though it is not strictly necessary.
We still call them manually for unmounting because it doesn't have a transaction.
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.

4 participants