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 #7202

Closed
wants to merge 4 commits into from
Closed

Make ReactPerf.start() work during reconciliation #7202

wants to merge 4 commits into from

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. This lets us avoid writing the logic to recover from isProfiling flipping in the middle of reconciliation.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 6, 2016

Looks like memory usage grew too much nevertheless (Travis is failing).

@gaearon gaearon closed this Jul 6, 2016
@gaearon gaearon deleted the nathanmarks-reactperf-component-wrapper-test branch July 6, 2016 22:24
@gaearon
Copy link
Collaborator Author

gaearon commented Jul 6, 2016

GitHub wasn’t picking up changes so I continued in #7208.

@gaearon gaearon removed this from the 15-next milestone Jul 6, 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.

3 participants