-
Notifications
You must be signed in to change notification settings - Fork 47.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
Don't count the time inside flushes towards lifecycle hooks #6860
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -329,6 +329,62 @@ describe('ReactPerf', function() { | |
}]); | ||
}); | ||
|
||
it('should not count time in a portal towards lifecycle method', function() { | ||
function Foo() { | ||
return null; | ||
} | ||
|
||
var portalContainer = document.createElement('div'); | ||
class Portal extends React.Component { | ||
componentDidMount() { | ||
ReactDOM.render(<Foo />, portalContainer); | ||
} | ||
render() { | ||
return null; | ||
} | ||
} | ||
|
||
var container = document.createElement('div'); | ||
var measurements = measure(() => { | ||
ReactDOM.render(<Portal />, container); | ||
}); | ||
|
||
expect(ReactPerf.getExclusive(measurements)).toEqual([{ | ||
key: 'Portal', | ||
instanceCount: 1, | ||
totalDuration: 6, | ||
counts: { | ||
ctor: 1, | ||
componentDidMount: 1, | ||
render: 1, | ||
}, | ||
durations: { | ||
ctor: 1, | ||
// We want to exclude nested imperative ReactDOM.render() from lifecycle hook's own time. | ||
// Otherwise it would artificially float to the top even though its exclusive time is small. | ||
// This is how we get 4 as a number with the performanceNow() mock: | ||
// - we capture the time we enter componentDidMount (n = 0) | ||
// - we capture the time when we enter a nested flush (n = 1) | ||
// - in the nested flush, we call it twice: before and after <Foo /> rendering. (n = 3) | ||
// - we capture the time when we exit a nested flush (n = 4) | ||
// - we capture the time we exit componentDidMount (n = 5) | ||
// Time spent in componentDidMount = (5 - 0 - (4 - 3)) = 4. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not super happy about this. If it’s too brittle, I can probably do something nasty like call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works for me. |
||
componentDidMount: 4, | ||
render: 1, | ||
}, | ||
}, { | ||
key: 'Foo', | ||
instanceCount: 1, | ||
totalDuration: 1, | ||
counts: { | ||
render: 1, | ||
}, | ||
durations: { | ||
render: 1, | ||
}, | ||
}]); | ||
}); | ||
|
||
it('warns once when using getMeasurementsSummaryMap', function() { | ||
var measurements = measure(() => {}); | ||
spyOn(console, 'error'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be simpler to just store a reference to
currentTimer
here instead of leaving them split up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it silly to worry about extra allocations here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll probably do this, but as a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. :) Doubly so because it's dev-only.