Add ProfileRoot component for collecting new time metrics #12745
Conversation
Does this include deep |
|
React: size: ReactDOM: size: Details of bundled changes.Comparing: a9abd27...e3a2562 react
react-dom
react-art
react-test-renderer
react-reconciler
react-is
react-native-renderer
Generated by |
Yes, anything that re-renders the root. There are a few tests that touch on this in |
|
Makes sense, just wanted to note that in external communication people generally don’t consider a deep rerender to “re-render a parent/root”. The fact that it’s structured this way is more of a Fiber architecture artefact. I agree it’s not docs but this tripped me a little |
|
Understand and agree. Without this behavior though, it would not be possible to e.g. measure the cost of a <Application>
<ProfileRoot id="Header" callback="...">
<Header />
</ProfileRoot>
<Body />
</Application>Unless I've overlooked something. |
| switch (finishedWork.tag) { | ||
| case Profiler: { | ||
| if (enableProfileModeMetrics) { | ||
| finishedWork.pendingProps.onRender.call( |
sebmarkbage
May 4, 2018
•
Member
let onRender = finishedWork.memoizedProps.onRender;
onRender(...);
I think we should use memoizedProps.
let onRender = finishedWork.memoizedProps.onRender;
onRender(...);I think we should use memoizedProps.
bvaughn
May 4, 2018
•
Author
Contributor
I'm curious- why?
Lint complains about that form (no-useless-call) but I can add an ignore rule if you prefer it.
I'm curious- why?
Lint complains about that form (no-useless-call) but I can add an ignore rule if you prefer it.
bvaughn
May 4, 2018
Author
Contributor
Oh, haha. I misread that 😄
Oh, haha. I misread that
sebmarkbage
May 5, 2018
Member
It’s two issues. We should use memoizedProps and avoid .call
It’s two issues. We should use memoizedProps and avoid .call
bvaughn
May 5, 2018
•
Author
Contributor
Already using memoized.
I'll remove .call - but I'd like to learn more about why it's good to avoid it in this case. 😄
Already using memoized.
I'll remove .call - but I'd like to learn more about why it's good to avoid it in this case.
| } | ||
|
|
||
| // Never bail out early for Profilers. | ||
| // We always want to re-measure the subtree. |
acdlite
May 4, 2018
Member
This seems wrong. You want to measure the subtree only if there is pending work in the subtree. We shouldn't reconcile the children unless there are new props.
This seems wrong. You want to measure the subtree only if there is pending work in the subtree. We shouldn't reconcile the children unless there are new props.
bvaughn
May 4, 2018
Author
Contributor
Maybe my wording is wrong?
This is not forcing children to re-render. It's just measuring the "cost" of re-rendering. If the tree bails out b'c of e.g. sCU then the "actual" render time is small, which is good.
Maybe my wording is wrong?
This is not forcing children to re-render. It's just measuring the "cost" of re-rendering. If the tree bails out b'c of e.g. sCU then the "actual" render time is small, which is good.
acdlite
May 4, 2018
Member
But why are you reconciling the children? You should only do that if the props have changed. Otherwise you should skip reconciling, which is what bailoutOnAlreadyFinishedWork does.
But why are you reconciling the children? You should only do that if the props have changed. Otherwise you should skip reconciling, which is what bailoutOnAlreadyFinishedWork does.
bvaughn
May 4, 2018
Author
Contributor
Gotcha.
I'll take another look at this this evening, after dinner. 👍
Gotcha.
I'll take another look at this this evening, after dinner.
|
|
||
| // Reset actualTime after successful commit. | ||
| // By default, we append to this time to account for errors and pauses. | ||
| finishedWork.stateNode = 0; |
acdlite
May 4, 2018
Member
I think Dan ran some performance tests that showed the stateNode should always use an object or null, so the hidden class is monomorphic, for performance reasons.
I think Dan ran some performance tests that showed the stateNode should always use an object or null, so the hidden class is monomorphic, for performance reasons.
gaearon
May 4, 2018
Member
It's not ideal but since we're already doing this for context I guess this doesn't make it worse.
It's not ideal but since we're already doing this for context I guess this doesn't make it worse.
bvaughn
May 4, 2018
Author
Contributor
It's easy enough to add a wrapper object if that would be better in any way. (I actually had one earlier and removed it last minute.)
It's easy enough to add a wrapper object if that would be better in any way. (I actually had one earlier and removed it last minute.)
| @@ -404,6 +421,20 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| renderExpirationTime: ExpirationTime, | |||
| ): Fiber | null { | |||
| const newProps = workInProgress.pendingProps; | |||
|
|
|||
| if (enableProfileModeMetrics) { | |||
| if (workInProgress.mode & ProfileMode) { | |||
acdlite
May 4, 2018
Member
Can you move this resetExpirationTime in the scheduler? That's where we "bubble up" expiration time, which is conceptually similar.
Can you move this resetExpirationTime in the scheduler? That's where we "bubble up" expiration time, which is conceptually similar.
bvaughn
May 4, 2018
Author
Contributor
Sure. No strong preference.
Sure. No strong preference.
| @@ -451,6 +468,12 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| commitAttachRef(nextEffect); | |||
| } | |||
|
|
|||
| if (enableProfileModeMetrics) { | |||
| if (effectTag & CommitProfile) { | |||
acdlite
May 4, 2018
Member
Can you reuse the Update effect type? I don't think we need a new tag here.
Can you reuse the Update effect type? I don't think we need a new tag here.
acdlite
May 4, 2018
Member
Profiler is the only type of work that might have this effect, right?
Profiler is the only type of work that might have this effect, right?
bvaughn
May 4, 2018
Author
Contributor
Hm. Maybe. Not obvious to me why that would be an improvement.
Hm. Maybe. Not obvious to me why that would be an improvement.
bvaughn
May 4, 2018
Author
Contributor
Our comments got crossed.
Profiler is the only type of work that might have this effect, right?
Yes.
Our comments got crossed.
Profiler is the only type of work that might have this effect, right?
Yes.
acdlite
May 4, 2018
•
Member
You're adding an extra check to every single unit of work. We already check for the Update effect.
You're adding an extra check to every single unit of work. We already check for the Update effect.
bvaughn
May 4, 2018
Author
Contributor
👍
| @@ -216,6 +220,12 @@ export default function<C, CX>( | |||
| } | |||
|
|
|||
| function unwindInterruptedWork(interruptedWork: Fiber) { | |||
| if (enableProfileModeMetrics) { | |||
acdlite
May 4, 2018
Member
Same as previous comment. Can we move this into the switch case for Profilers?
Same as previous comment. Can we move this into the switch case for Profilers?
bvaughn
May 4, 2018
Author
Contributor
👍
I agree that's how it should work, I'm just saying that if we explain it as "when ProfileRoot re-rendered" many people won't realize that's what it means. Sorry I'm bikeshedding, let's discuss later in chat if this doesn't make sense :-) |
| const onRender = finishedWork.memoizedProps.onRender; | ||
| onRender( | ||
| finishedWork.memoizedProps.id, | ||
| finishedWork.alternate === null ? 'mount' : 'update', |
sebmarkbage
May 5, 2018
Member
Use current === null instead. We shouldn’t use alternate in most places. We tend to pass it so that we’re free to switch to a non-alternate based model in the future.
Use current === null instead. We shouldn’t use alternate in most places. We tend to pass it so that we’re free to switch to a non-alternate based model in the future.
| now = function() { | ||
| return Date.now(); | ||
| }; | ||
| } |
sebmarkbage
May 5, 2018
Member
We have this in the host config. Can we reuse that?
We should ideally avoid platform specific code in shared code like this. Some renderers will have different implementations.
We have this in the host config. Can we reuse that?
We should ideally avoid platform specific code in shared code like this. Some renderers will have different implementations.
bvaughn
May 6, 2018
Author
Contributor
I used the host config function originally but removed it because my understanding was that we planned to change that in the future in a way that would not be compatible with this use case. cc @acdlite
I used the host config function originally but removed it because my understanding was that we planned to change that in the future in a way that would not be compatible with this use case. cc @acdlite
bvaughn
May 6, 2018
Author
Contributor
If this is not the case, I can change it back to using the host config function. I'll have to make a change two test renderer to support mocking that function but that should be easy.
If this is not the case, I can change it back to using the host config function. I'll have to make a change two test renderer to support mocking that function but that should be easy.
acdlite
May 6, 2018
Member
@bvaughn I miscommunicated. I thought you were talking about the scheduler’s recalculateCurrentTime function. HostConfig.now is fine to use. My bad!
@bvaughn I miscommunicated. I thought you were talking about the scheduler’s recalculateCurrentTime function. HostConfig.now is fine to use. My bad!
bvaughn
May 7, 2018
Author
Contributor
Ah! No problem. Thanks for clarifying. 😄 I'll revert that particular change then.
Ah! No problem. Thanks for clarifying.
| * If a fiber bails out (sCU false) then its "base" timer is cancelled and the fiber is not updated. | ||
| */ | ||
|
|
||
| let baseStartTime: number | null = null; |
sebmarkbage
May 5, 2018
Member
In wasm this wouldn’t work. Some VMs may want to use a specific representation for numbers based on type info.
We shouldn’t mix null and numbers. Null is meant for missing objects, not missing numbers.
Let’s use 0 or -1 instead of null.
In wasm this wouldn’t work. Some VMs may want to use a specific representation for numbers based on type info.
We shouldn’t mix null and numbers. Null is meant for missing objects, not missing numbers.
Let’s use 0 or -1 instead of null.
| } | ||
|
|
||
| export function markActualRenderTimeStarted(fiber: Fiber): void { | ||
| fiber.stateNode -= now() - totalElapsedPauseTime; |
sebmarkbage
May 5, 2018
Member
Let’s use a temp variable here so we don’t have to read the property again below.
Let’s use a temp variable here so we don’t have to read the property again below.
| if (enableProfileModeMetrics) { | ||
| // Stop "base" render timer again (after the re-thrown error). | ||
| stopBaseRenderTimer(); | ||
| } |
sebmarkbage
May 6, 2018
Member
Does this include both renders in the time spent? Similarly for the double render flag, does it include both?
Does this include both renders in the time spent? Similarly for the double render flag, does it include both?
bvaughn
May 6, 2018
Author
Contributor
"Both renders" meaning- if the side effect flag is enabled?
Yes. The tests disable the side effects flag for this reason.
"Both renders" meaning- if the side effect flag is enabled?
Yes. The tests disable the side effects flag for this reason.
sebmarkbage
May 6, 2018
Member
In this particular case it doesn’t matter I guess because it won’t update the base time since it errored?
In the normal case of double render I suppose we have to include it in the render time because otherwise the relative time to actual time will be misleading.
DEV is already slower than prod so the absolute numbers probably isn’t actionable regardless.
In this particular case it doesn’t matter I guess because it won’t update the base time since it errored?
In the normal case of double render I suppose we have to include it in the render time because otherwise the relative time to actual time will be misleading.
DEV is already slower than prod so the absolute numbers probably isn’t actionable regardless.
bvaughn
May 7, 2018
Author
Contributor
Ah, in the event of an error, if the replayFailedUnitOfWorkWithInvokeGuardedCallback flag is enabled, then when replayUnitOfWork is called- it also stops (and discards) the base time. I think this is right, because that time is kind of meaningless.
Ah, in the event of an error, if the replayFailedUnitOfWorkWithInvokeGuardedCallback flag is enabled, then when replayUnitOfWork is called- it also stops (and discards) the base time. I think this is right, because that time is kind of meaningless.
| @@ -645,6 +660,12 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| } | |||
| } | |||
|
|
|||
| if (__DEV__) { | |||
| if (enableProfileModeMetrics) { | |||
| checkActualRenderTimeStackEmpty(); | |||
sebmarkbage
May 6, 2018
Member
Does this just check that the stack is empty? Aren’t we already doing that? If we need an extra safety check, shouldn’t this just be a generic check of the stack, not specifically to the profiler?
Does this just check that the stack is empty? Aren’t we already doing that? If we need an extra safety check, shouldn’t this just be a generic check of the stack, not specifically to the profiler?
bvaughn
May 6, 2018
Author
Contributor
This is a DEV-only sanity check to make sure we don't forget to pop anything from the timing stack.
We do the same DEV-only check with the context stack in scheduler.
This is a DEV-only sanity check to make sure we don't forget to pop anything from the timing stack.
We do the same DEV-only check with the context stack in scheduler.
| @@ -702,6 +723,19 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| child = child.sibling; | |||
| } | |||
| workInProgress.expirationTime = newExpirationTime; | |||
|
|
|||
| if (enableProfileModeMetrics) { | |||
| if (workInProgress.mode & ProfileMode) { | |||
sebmarkbage
May 6, 2018
Member
Let’s do the expiration time bubbling in here too. You can duplicate that code.
Let’s only do the existing path if we are not on profile mode.
That lets us reuse the same iteration instead of two iterations.
Let’s do the expiration time bubbling in here too. You can duplicate that code.
Let’s only do the existing path if we are not on profile mode.
That lets us reuse the same iteration instead of two iterations.
bvaughn
May 7, 2018
Author
Contributor
I think you're saying:
if (enableProfileModeMetrics) {
if (workInProgress.mode & ProfileMode) {
// Bubble both times
} else {
// Bubble only expiration time
} else {
// Bubble only expiration time
}
I think you're saying:
if (enableProfileModeMetrics) {
if (workInProgress.mode & ProfileMode) {
// Bubble both times
} else {
// Bubble only expiration time
} else {
// Bubble only expiration time
}| if (enableProfileModeMetrics) { | ||
| // If we didn't finish, pause the "actual" render timer. | ||
| // We'll restart it when we resume work. | ||
| if (nextUnitOfWork !== null) { |
sebmarkbage
May 6, 2018
Member
If we did finish, can we always just call pause so we don’t need the check? Simplifies things a bit. pauseActualRenderTimerIfRunning is fast anyway.
If we did finish, can we always just call pause so we don’t need the check? Simplifies things a bit. pauseActualRenderTimerIfRunning is fast anyway.
bvaughn
May 6, 2018
Author
Contributor
I...think so.
I...think so.
| next = beginWork(current, workInProgress, nextRenderExpirationTime); | ||
|
|
||
| // Update "base" time if the render wasn't bailed out on. | ||
| if (isBaseRenderTimerRunning()) { |
sebmarkbage
May 6, 2018
Member
This is a clever way to model the bailout problem. Nice.
This is a clever way to model the bailout problem. Nice.
|
|
||
| // Update "base" time if the render wasn't bailed out on. | ||
| if (isBaseRenderTimerRunning()) { | ||
| workInProgress.selfBaseTime = getElapsedBaseRenderTime(); |
sebmarkbage
May 6, 2018
Member
Because performance.now is throttled in browsers and many components are pretty fast, I wouldn't be surprised if this is actually zero or one most of the time. Either way, when we add them up, it'll be misleading. :(
Not sure how to solve that. Maybe eventually performance.now will be accurate again. At least it'll be better on RN.
Because performance.now is throttled in browsers and many components are pretty fast, I wouldn't be surprised if this is actually zero or one most of the time. Either way, when we add them up, it'll be misleading. :(
Not sure how to solve that. Maybe eventually performance.now will be accurate again. At least it'll be better on RN.
| if (enableProfileModeMetrics) { | ||
| resumeActualRenderTimerIfPaused(); | ||
| } | ||
sebmarkbage
May 6, 2018
Member
Can you move the user timing block above findHighestPriorityRoot so that it is next to this one? Since they do the same thing, it's nice to have them colocated.
Can you move the user timing block above findHighestPriorityRoot so that it is next to this one? Since they do the same thing, it's nice to have them colocated.
bvaughn
May 7, 2018
Author
Contributor
Moving that one up would break things, since findHighestPriorityRoot() sets nextFlushedExpirationTime. I can move this one down though.
Moving that one up would break things, since findHighestPriorityRoot() sets nextFlushedExpirationTime. I can move this one down though.
| if (enableProfileModeMetrics) { | ||
| // If we didn't finish, pause the "actual" render timer. | ||
| // We'll restart it when we resume work. | ||
| if (nextUnitOfWork !== null) { |
sebmarkbage
May 6, 2018
Member
Same question as the other spot. Is this check needed or can we just always call pauseActualRenderTimerIfRunning?
Same question as the other spot. Is this check needed or can we just always call pauseActualRenderTimerIfRunning?
| * It is paused (and accumulated) in the event of an interruption or an aborted render. | ||
| */ | ||
|
|
||
| const {checkThatStackIsEmpty, createCursor, push, pop} = ReactFiberStack(); |
sebmarkbage
May 6, 2018
Member
We should never create new fiber stacks anywhere except once.
We should reuse the one from here:
react/packages/react-reconciler/src/ReactFiberScheduler.js
Lines 161 to 164
in
b548b3c
We should never create new fiber stacks anywhere except once.
We should reuse the one from here:
react/packages/react-reconciler/src/ReactFiberScheduler.js
Lines 161 to 164 in b548b3c
bvaughn
May 7, 2018
•
Author
Contributor
Interesting!
I avoided doing this initially because:
- The existing stack is complete context-centric, so I wasn't sure if it was appropriate to intermingle timing values. (I was also concerned about adding complexity in terms of push/pop timing when adding more hooks into the existing stack.)
- Re-using the existing stack cursor makes the profiler functions more awkward to use (since it requires initializing the module explicitly, and passing the profiler to the begin/unwind/complete modules explicitly).
- The stack seemed like a lightweight enough structure that I didn't think the cost of creating a new one for profiling mode (not production mode) was a big concern.
That being said, I'll make this change 👍
Interesting!
I avoided doing this initially because:
- The existing stack is complete context-centric, so I wasn't sure if it was appropriate to intermingle timing values. (I was also concerned about adding complexity in terms of push/pop timing when adding more hooks into the existing stack.)
- Re-using the existing stack cursor makes the profiler functions more awkward to use (since it requires initializing the module explicitly, and passing the profiler to the begin/unwind/complete modules explicitly).
- The stack seemed like a lightweight enough structure that I didn't think the cost of creating a new one for profiling mode (not production mode) was a big concern.
That being said, I'll make this change
|
|
||
| const {checkThatStackIsEmpty, createCursor, push, pop} = ReactFiberStack(); | ||
|
|
||
| let stackCursor: StackCursor<Fiber> = createCursor(null); |
sebmarkbage
May 6, 2018
Member
This seems like very odd types. I'm not sure how Flow lets you do this.
You're passing a default argument for null which is not compatible with the type Fiber which you're assigning to the StackCursor.
Then later you actually pass a number to it. So it's three way inconsistent.
I think it's not noticed because you never actually use the values stored in this stack.
This seems like very odd types. I'm not sure how Flow lets you do this.
You're passing a default argument for null which is not compatible with the type Fiber which you're assigning to the StackCursor.
Then later you actually pass a number to it. So it's three way inconsistent.
I think it's not noticed because you never actually use the values stored in this stack.
| @@ -1093,6 +1132,9 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| case ContextProvider: | |||
| pushProvider(workInProgress); | |||
| break; | |||
| case Profiler: | |||
| markActualRenderTimeStarted(workInProgress); | |||
sebmarkbage
May 6, 2018
Member
Should this be wrapped in if (enableProfileModeMetrics)?
Should this be wrapped in if (enableProfileModeMetrics)?
| // Let the "complete" phase know to stop the timer, | ||
| // And the scheduler to record the measured time. | ||
| workInProgress.effectTag |= Update; | ||
| } else if (workInProgress.memoizedProps === nextProps) { |
sebmarkbage
May 6, 2018
Member
If enableProfileModeMetrics is enabled, we should still bailout on equality, right? Seems odd to disable that optimization.
If enableProfileModeMetrics is enabled, we should still bailout on equality, right? Seems odd to disable that optimization.
bvaughn
May 7, 2018
Author
Contributor
Yes, good catch. This should have been two ifs rather than an if/else
Yes, good catch. This should have been two ifs rather than an if/else
| @@ -911,6 +959,14 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| while (nextUnitOfWork !== null && !shouldYield()) { | |||
| nextUnitOfWork = performUnitOfWork(nextUnitOfWork); | |||
| } | |||
sebmarkbage
May 6, 2018
Member
Running the base timer for everything in the tree regardless of if this fiber is part of a profiled tree or not makes this a cost paid by everyone, whether you use the feature or not. This is probably fine for now since it's off by default.
If we want to turn it on my default, then we should probably move the startBaseRenderTimer here. That way we can start a work loop that doesn't have a startBaseRenderTimer call in it, unless the first fiber is already in a profiled tree. Then when we hit a Profiler fiber, we can start another work loop that has the startBaseRenderTimer call in it.
We can do that later though. No need to do it in this PR.
Running the base timer for everything in the tree regardless of if this fiber is part of a profiled tree or not makes this a cost paid by everyone, whether you use the feature or not. This is probably fine for now since it's off by default.
If we want to turn it on my default, then we should probably move the startBaseRenderTimer here. That way we can start a work loop that doesn't have a startBaseRenderTimer call in it, unless the first fiber is already in a profiled tree. Then when we hit a Profiler fiber, we can start another work loop that has the startBaseRenderTimer call in it.
We can do that later though. No need to do it in this PR.
|
There’s something odd about not resetting the pause time. The other issue is that you don’t use the stack. I suspect they’re related. Nested profilers will have issues. Let’s talk through the scenarios. |
| if (enableProfileModeMetrics) { | ||
| // Stop "base" render timer again (after the re-thrown error). | ||
| stopBaseRenderTimer(); | ||
| } |
sebmarkbage
May 6, 2018
Member
In this particular case it doesn’t matter I guess because it won’t update the base time since it errored?
In the normal case of double render I suppose we have to include it in the render time because otherwise the relative time to actual time will be misleading.
DEV is already slower than prod so the absolute numbers probably isn’t actionable regardless.
In this particular case it doesn’t matter I guess because it won’t update the base time since it errored?
In the normal case of double render I suppose we have to include it in the render time because otherwise the relative time to actual time will be misleading.
DEV is already slower than prod so the absolute numbers probably isn’t actionable regardless.
| @@ -150,6 +153,10 @@ export type Fiber = {| | |||
| // memory if we need to. | |||
| alternate: Fiber | null, | |||
|
|
|||
| // Profiling metrics | |||
| selfBaseTime: number | null, | |||
sebmarkbage
May 6, 2018
Member
Same thing as the other thing. We should keep this as number only. Use a number like 0 or -1 if you need to indicate missing value.
Same thing as the other thing. We should keep this as number only. Use a number like 0 or -1 if you need to indicate missing value.
bvaughn
May 7, 2018
Author
Contributor
This is just bad Flow typing on my part. I actually do initialize both to 0 below. My bad.
This is just bad Flow typing on my part. I actually do initialize both to 0 below. My bad.
| } | ||
| } | ||
|
|
||
| return baseStartTime === -1 ? 0 : now() - baseStartTime; |
sebmarkbage
May 6, 2018
Member
This baseStartTime === -1 is an error anyway we don’t have to do this check at runtime. If this was all inlined in the same function the compiler could maybe know that but it is broken apart so it might not.
That’s a perfect example why I prefer to avoid abstractions like these functions, and moving to other modules, because it makes it harder to see that you’re actually doing the same comparison twice.
If they were together in the same function it would be obvious.
You could refactor this to pass the fiber instead and do the condition and update in the same function. Like updateBaseTime(workInProgress).
This baseStartTime === -1 is an error anyway we don’t have to do this check at runtime. If this was all inlined in the same function the compiler could maybe know that but it is broken apart so it might not.
That’s a perfect example why I prefer to avoid abstractions like these functions, and moving to other modules, because it makes it harder to see that you’re actually doing the same comparison twice.
If they were together in the same function it would be obvious.
You could refactor this to pass the fiber instead and do the condition and update in the same function. Like updateBaseTime(workInProgress).
|
|
||
| let stackCursor: StackCursor<Fiber> = createCursor(null); | ||
| let timerPausedAt: number = 0; | ||
| let totalElapsedPauseTime: number = 0; |
sebmarkbage
May 6, 2018
Member
This only increases but never gets reset. That seems odd. At some point we need to reset it right?
This only increases but never gets reset. That seems odd. At some point we need to reset it right?
bvaughn
May 7, 2018
•
Author
Contributor
Do we? Why? This time is accounted for in both the start and stop time methods.
I guess I can reset it in commitRoot if you think it's best.
Do we? Why? This time is accounted for in both the start and stop time methods.
I guess I can reset it in commitRoot if you think it's best.
|
For nested profilers I guess the semantics are not really clear. There are two possible semantics: a) We include the time of the nested profiler in both the base time and actual time of the nested profiler. This seems intuitive to me but is also misleading since when you add up the time of several profilers they won’t be representative of the time if the tree. b) The parent time include only the time of the children that are not nested in other profilers. The base time that bubbles up from a profiler would be zero. I think with this strategy we won’t need a stack which is nice. Not sure what is more useful. You can infer some data if you have tree info in the id. So I guess it’s more about when you don’t want to have tree info in the id so that they accumulate across usages in different trees. |
|
I believe I have made all of the edits suggested above. Each one is a single commit, so hopefully it will be easy to review. |
I don't think this is actually an issue, but let's talk about it.
I have tests for nested profilers. Can you think of cases I'm not testing? I'd love to improve the tests.
WRT nested profilers, I think option A makes the most sense (and is the behavior my test cases currently verify). Option B seems like it would be too easy for e.g. on person to mess up another person's timing metrics unintentionally by adding a profiler at a lower level in the tree. |
| @@ -226,7 +226,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| const nextProps = workInProgress.pendingProps; | |||
| if (enableProfileModeMetrics) { | |||
| // Start render timer here and push start time onto queue | |||
| markActualRenderTimeStarted(workInProgress); | |||
| markActualRenderTimeStarted(workInProgress, now); | |||
sebmarkbage
May 7, 2018
Member
We should pass this when we initial the module instead of an argument, so that it can be statically known which function is called. This will be a lot easier when we move to static injection instead of dynamic.
We should pass this when we initial the module instead of an argument, so that it can be statically known which function is called. This will be a lot easier when we move to static injection instead of dynamic.
bvaughn
May 7, 2018
Author
Contributor
Yeah. Duh, me.
Yeah. Duh, me.
| } | ||
| workInProgress.expirationTime = newExpirationTime; | ||
|
|
||
| // (And "base" render timers if that feature flag is enabled) | ||
| if (enableProfileModeMetrics) { | ||
| if (workInProgress.mode & ProfileMode) { |
sebmarkbage
May 7, 2018
Member
If you change this to enableProfileModeMetrics && (workInProgress.mode & ProfileMode) you only need one else block.
If you change this to enableProfileModeMetrics && (workInProgress.mode & ProfileMode) you only need one else block.
bvaughn
May 7, 2018
•
Author
Contributor
But then we won't strip the code when enableProfileModeMetrics is false. I assumed this was more important. Disregard. I'm silly.
But then we won't strip the code when Disregard. I'm silly.enableProfileModeMetrics is false. I assumed this was more important.
| import type {StackCursor, Stack} from './ReactFiberStack'; | ||
|
|
||
| import warning from 'fbjs/lib/warning'; | ||
|
|
sebmarkbage
May 7, 2018
Member
This whole module needs to be wrapped in a closure and initialized because it is stateful. Otherwise these timers will be shared with other renderers that could be rendering interleaved with this one.
That will also let you get a static reference to now instead of passing it as an argument.
Doing that might however mess up closure compiler and lead everything in here to not be inlined which would be bad for file size, compile perf and possibly runtime perf. Who knows?
We need to move to static host configs.
This whole module needs to be wrapped in a closure and initialized because it is stateful. Otherwise these timers will be shared with other renderers that could be rendering interleaved with this one.
That will also let you get a static reference to now instead of passing it as an argument.
Doing that might however mess up closure compiler and lead everything in here to not be inlined which would be bad for file size, compile perf and possibly runtime perf. Who knows?
We need to move to static host configs.
bvaughn
May 7, 2018
•
Author
Contributor
Otherwise these timers will be shared with other renderers that could be rendering interleaved with this one.
D'oh. Good point.
I always forget about this use case.
Otherwise these timers will be shared with other renderers that could be rendering interleaved with this one.
D'oh. Good point.
I always forget about this use case.
| @@ -607,7 +597,12 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| case Mode: | |||
| return null; | |||
| case Profiler: | |||
| updateProfiler(workInProgress); | |||
| if (enableProfileModeMetrics) { | |||
| ((actualRenderTimer: any): ActualRenderTimer).recordElapsedActualRenderTime( | |||
sebmarkbage
May 7, 2018
Member
Destructure these at the top like we do with everything else so that we don't have to look up this property dynamically every time. Hopefully this also lets us inline this call. (We should confirm whether it does.)
Destructure these at the top like we do with everything else so that we don't have to look up this property dynamically every time. Hopefully this also lets us inline this call. (We should confirm whether it does.)
| selfBaseTime: number, | ||
| treeBaseTime: number, | ||
| selfBaseTime?: number, | ||
| treeBaseTime?: number, |
sebmarkbage
May 7, 2018
Member
This is wrong. We should always assign them and they should always a consistent type value. E.g. 0.
Making them optional is a huge no, no since it might break the hidden class of the fiber.
This is wrong. We should always assign them and they should always a consistent type value. E.g. 0.
Making them optional is a huge no, no since it might break the hidden class of the fiber.
bvaughn
May 7, 2018
•
Author
Contributor
I wasn't sure how to type them- given that we only assign these values if the feature flag is enabled. (We don't want to add them to every fiber if the flag isn't even enabled- right?)
I wasn't sure how to type them- given that we only assign these values if the feature flag is enabled. (We don't want to add them to every fiber if the flag isn't even enabled- right?)
bvaughn
May 7, 2018
•
Author
Contributor
To be clear, they are always either on or off. It's based on the feature flag. This will not break the hidden class of a fiber, since this is a bundle-wide thing.
if (enableProfileModeMetrics) {
this.selfBaseTime = 0;
this.treeBaseTime = 0;
}
To be clear, they are always either on or off. It's based on the feature flag. This will not break the hidden class of a fiber, since this is a bundle-wide thing.
if (enableProfileModeMetrics) {
this.selfBaseTime = 0;
this.treeBaseTime = 0;
}
sebmarkbage
May 7, 2018
Member
Hm. I see. I suppose we do that with DEV only too. Seems odd that Flow doesn't force you to check for their existence everywhere then? We shouldn't need to check if they're undefined/null but seems like Flow would want us to.
Hm. I see. I suppose we do that with DEV only too. Seems odd that Flow doesn't force you to check for their existence everywhere then? We shouldn't need to check if they're undefined/null but seems like Flow would want us to.
bvaughn
May 7, 2018
Author
Contributor
I agree. I expected that also.
I agree. I expected that also.
| resumeActualRenderTimerIfPaused(now: Now): void, | ||
| }; | ||
|
|
||
| export function createActualRenderTimer(stack: Stack): ActualRenderTimer { |
sebmarkbage
May 7, 2018
Member
Sorry didn't see that you wrapped this in a later commit. This needs to also include the base time to be safe.
You can pass now to this instead of each function.
Sorry didn't see that you wrapped this in a later commit. This needs to also include the base time to be safe.
You can pass now to this instead of each function.
| @@ -88,8 +93,9 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>( | |||
| hydrationContext: HydrationContext<C, CX>, | |||
| scheduleWork: (fiber: Fiber, expirationTime: ExpirationTime) => void, | |||
| computeExpirationForFiber: (fiber: Fiber) => ExpirationTime, | |||
| actualRenderTimer: ActualRenderTimer | null, | |||
sebmarkbage
May 7, 2018
•
Member
Let's always pass it the module and assume that the compiler will strip it all out. We do assume that everywhere else, whether it does or not.
Let's always pass it the module and assume that the compiler will strip it all out. We do assume that everywhere else, whether it does or not.
…k#12745) Add a new component type, Profiler, that can be used to collect new render time metrics. Since this is a new, experimental API, it will be exported as React.unstable_Profiler initially. Most of the functionality for this component has been added behind a feature flag, enableProfileModeMetrics. When the feature flag is disabled, the component will just render its children with no additional behavior. When the flag is enabled, React will also collect timing information and pass it to the onRender function (as described below).
Add a new component type, Profiler, that can be used to collect new render time metrics. Since this is a new, experimental API, it will be exported as React.unstable_Profiler initially. Most of the functionality for this component has been added behind a feature flag, enableProfileModeMetrics. When the feature flag is disabled, the component will just render its children with no additional behavior. When the flag is enabled, React will also collect timing information and pass it to the onRender function (as described below).
Add a new component type,
Profiler, that can be used to collect new render time metrics. Since this is a new, experimental API, it will be exported asReact.unstable_Profilerinitially.Most of the functionality for this component has been added behind a feature flag,
enableProfileModeMetrics. When the feature flag is disabled, the component will just render its children with no additional behavior. When the flag is enabled, React will also collect timing information and pass it to theonRenderfunction (as described below).This flag will be enabled for the DEV bundle in the future. A special profiling+production build will likely be created as well, in order to use these new metrics in production mode without impacting existing applications.
How is it used?
Profilercan be declared anywhere within a React tree and can be nested to measure multiple components within the same tree.The
onRendercallback is called each time the root renders. It receives the following parameters:id: string- Theidvalue of theProfilertag that was measured.phase: string- Either "mount" or "update" (depending on whether this root was newly mounted or has just been updated).actualTime: number- Time spent rendering theProfilerand its descendants for the most recent "mount" or "update" render. 1baseTime: number- Duration of the most recentrendertime for each individual component within theProfilertree. 11: See below for more detailed information about what this time represents.
Timing metrics
Here is a review of the types of timing React is now capable of reporting:
User Timing API
Measures start/stop times for each component lifecycle.
“Actual” render time (new)
Time spent rendering the
Profilerand its descendants for the most recent render/update.<Profiler>ProfileronlyProfileris re-renderedshouldComponentUpdatefor memoization?“Base” render time (new)
Duration of the most recent
rendertime for each individual component within theProfilertree.<Profiler>ProfileronlyProfilercomponent.shouldComponentUpdateProfilerduring “complete” phaseProfiler(not for individual fibers)Profileris re-rendered2: Until "resume" behavior is implemented, interruptions will not accumulate time.
TODO
getComponentName(so flame graph labels will appear correctly)ReactIstoJSONandtoTreemethods