Add new ReactPerf #6046

Merged
merged 1 commit into from Apr 29, 2016

Conversation

Projects
None yet
@gaearon
Member

gaearon commented Feb 16, 2016

This is a work in progress on implementing new ReactPerf as discussed in #6015.

Per @sebmarkbage’s request, I decided to focus on removing dependencies on internal method names. Data will be explicitly passed to the perf tool from React methods, and we will attempt to not rely on the execution order.

Rather than refactor the existing code, I chose to create a new tool side by side so I can compare their output until I’m confident about correctness. I will later add PROFILE feature gates to the calls.

  • Add barebones implementation of new ReactPerf
  • It should count totalTime for flushes
  • It should not count totalTime twice for nested flushes (fixes a minor bug in ReactPerf)
  • It should not rely on the rendering and mounting stack matching parent hierarchy
  • Decide how inclusive measurements work
  • Add a safety mechanism to avoid accidentally forgetting endMeasure()
  • It should count exclusive time for every lifecycle method
  • It should include displayNames and other component information
  • It should reconstruct the parent tree
  • It should count counts and created for components
  • It should count exclusive times for components
  • It should count inclusive times for components based on parent tree
  • Make it the DebugTool
  • Make sure teams that replaced ReactDefaultPerf with wtf can keep doing so
  • It should implement printDOM()
  • It should implement printWasted()
  • Treat stateless components correctly
  • Expose the new ReactPerf as react-addons-perf
  • Make sure wasted measurements are useful (something’s off right now)
  • Do we want to rely on owner?
  • TESTS
  • Remove the old ReactPerf code
  • Introduce the new PROFILE gate and put calls behind it
  • Consider the implications of using WeakMap in __PROFILE__ builds
  • Expose React.unstable_Instrumentation
  • Make sure we have new get*() methods and deprecated printDOM() and getMeasurementSummaryMap() are still there
  • Expose whatever React Native needs for systrace integration and enabling PROFILE
  • Verify compatibility with React ART (e.g. add stuff like this)
  • Ensure we throw a meaningful error when start() is called inside the lifecycle or, better, consider providing support for that. See also #2095, #3436, https://github.com/lostthetrail/react-ssr-perf
  • New ReactPerf is correct, tested, has no effect in production, is hard to break accidentally when refactoring, and does not rely on implementation details
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 16, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 16, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 16, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 17, 2016

Member

I want to verify whether my understanding of the constraints in #6015 (comment) is correct. Here is an example I came up with. @sebmarkbage @spicyj Can you please confirm whether this is right?

Requirements

Say we’ve got three components. A includes B, which includes C and D.
Let’s see how the Perf tool receives the events over time.

Regular Recursive Batch

Components mount (or updated) recursively like they normally do. Numbers correspond to the current time, phrases to events the Perf tool receives. The columns show whether the time period is counted towards inclusive and exclusive scores of the corresponding components.

TIME                       A             B           C          D

0 (begin A)          
|                       exc+inc         
1                   
|                       exc+inc     
  2 (begin B)
  |                       inc         exc+inc
  3
  |                       inc         exc+inc
  4
  |                       inc         exc+inc
    5 (begin C)
    |                     inc           inc       exc+inc
    6 (end C)
  |                       inc         exc+inc
    7 (begin D)
    |                     inc           inc                  exc+inc
    8
    |                     inc           inc                  exc+inc
    9 (end D)
  |                       inc         exc+inc
  10 (end B)
|                       exc+inc    
11 (end A)

Funny Batch

We need to make sure that it is easy to introduce out-of-stack updates later. For example, we want a top-level update of C to count towards B and A inclusive counters.

As an extra safety measure, we will absurdly include an update of A inside the C update. This probably doesn’t make sense technically but we want to make sure the calculations are completely independent of the stack order no matter how bonkers it is.

TIME                      A             B           C          D

11 (begin C)
|                        inc           inc       exc+inc
12
|                        inc           inc       exc+inc
  13 (begin A)
  |                    exc+inc
  14 (end A)
|                        inc           inc       exc+inc
15
|                        inc           inc       exc+inc
16
|                        inc           inc       exc+inc
17 (end C)

Note how the C updates is still counted towards A and B inclusive time even though they are not on the stack. On the other hand, the A update is only counted towards A even though C is currently on the stack.

Formal Input

Relationships

A.owner = undefined
B.owner = A
C.owner = B
D.owner = B

Events

(0, begin, A)
(2, begin, B)
(5, begin, C)
(6, end, C)
(7, begin, D)
(9, end, D)
(10, end, B)
(11, end, A)
(11, begin, C)
(13, begin, A)
(14, end, A)
(17, end, C)

Desired Output

Exclusive

  • exc(A) = (2-0) + (11-10) + (14 - 13) = 2 + 1 + 1 = 4
  • exc(B) = (5-2) + (7-6) + (10-9) = 3 + 1 + 1 = 5
  • exc(C) = (6-5) + (13-11) + (17-14) = 1 + 2 + 3 = 6
  • exc(D) = (9-7) = 2

exc(A) + exc(B) + exc(C) + exc(D) = 4 + 5 + 6 + 2 = 17 = total

Inclusive

  • inc(D) = exc(D) = 2
  • inc(C) = exc(C) = 6
  • inc(B) = exc(B) + inc(C) + inc(D) = 5 + 6 + 2 = 13
  • inc(A) = exc(A) + inc(B) = 4 + 13 = 17

Do these calculations look right? Is this the desired output for these scenarios?

Member

gaearon commented Feb 17, 2016

I want to verify whether my understanding of the constraints in #6015 (comment) is correct. Here is an example I came up with. @sebmarkbage @spicyj Can you please confirm whether this is right?

Requirements

Say we’ve got three components. A includes B, which includes C and D.
Let’s see how the Perf tool receives the events over time.

Regular Recursive Batch

Components mount (or updated) recursively like they normally do. Numbers correspond to the current time, phrases to events the Perf tool receives. The columns show whether the time period is counted towards inclusive and exclusive scores of the corresponding components.

TIME                       A             B           C          D

0 (begin A)          
|                       exc+inc         
1                   
|                       exc+inc     
  2 (begin B)
  |                       inc         exc+inc
  3
  |                       inc         exc+inc
  4
  |                       inc         exc+inc
    5 (begin C)
    |                     inc           inc       exc+inc
    6 (end C)
  |                       inc         exc+inc
    7 (begin D)
    |                     inc           inc                  exc+inc
    8
    |                     inc           inc                  exc+inc
    9 (end D)
  |                       inc         exc+inc
  10 (end B)
|                       exc+inc    
11 (end A)

Funny Batch

We need to make sure that it is easy to introduce out-of-stack updates later. For example, we want a top-level update of C to count towards B and A inclusive counters.

As an extra safety measure, we will absurdly include an update of A inside the C update. This probably doesn’t make sense technically but we want to make sure the calculations are completely independent of the stack order no matter how bonkers it is.

TIME                      A             B           C          D

11 (begin C)
|                        inc           inc       exc+inc
12
|                        inc           inc       exc+inc
  13 (begin A)
  |                    exc+inc
  14 (end A)
|                        inc           inc       exc+inc
15
|                        inc           inc       exc+inc
16
|                        inc           inc       exc+inc
17 (end C)

Note how the C updates is still counted towards A and B inclusive time even though they are not on the stack. On the other hand, the A update is only counted towards A even though C is currently on the stack.

Formal Input

Relationships

A.owner = undefined
B.owner = A
C.owner = B
D.owner = B

Events

(0, begin, A)
(2, begin, B)
(5, begin, C)
(6, end, C)
(7, begin, D)
(9, end, D)
(10, end, B)
(11, end, A)
(11, begin, C)
(13, begin, A)
(14, end, A)
(17, end, C)

Desired Output

Exclusive

  • exc(A) = (2-0) + (11-10) + (14 - 13) = 2 + 1 + 1 = 4
  • exc(B) = (5-2) + (7-6) + (10-9) = 3 + 1 + 1 = 5
  • exc(C) = (6-5) + (13-11) + (17-14) = 1 + 2 + 3 = 6
  • exc(D) = (9-7) = 2

exc(A) + exc(B) + exc(C) + exc(D) = 4 + 5 + 6 + 2 = 17 = total

Inclusive

  • inc(D) = exc(D) = 2
  • inc(C) = exc(C) = 6
  • inc(B) = exc(B) + inc(C) + inc(D) = 5 + 6 + 2 = 13
  • inc(A) = exc(A) + inc(B) = 4 + 13 = 17

Do these calculations look right? Is this the desired output for these scenarios?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 17, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 17, 2016

Member

Yea, this looks right. I can't see how steps 13-14 could possibly happen but if it could, then yea, that would be the right call.

The point of decoupling this is so that in theory, someone else could've implemented this perf tool without changing the core. Since you're effectively mapping the names of methods 1:1, it doesn't create much decoupling. If we change the algorithm, you would still have to go change the ReactNewPerf implementation.

A good guideline is to put things that are likely to change together into the same file. So, if we can't decouple, we might as well put all of ReactNewPerf into ReactCompositeComponent etc. to make it easy to refactor.

I understand that this is your later step "It should not rely on the rendering and mounting stack matching parent hierarchy".

One suggestion that I have is that you could, have start and stop timer associated with component time in ReactNewPerf. E.g. startTime(internalInstance) and then call stopTime(internalInstance) right before the recursive call. Then you call startTime(internalInstance) again right after the recursive call completes. You only calculate exclusive time. Then at the end, the summary, you sum them up according to the tree to get the inclusive time.

That way we can restructure the order completely without changing ReactNewPerf.

Member

sebmarkbage commented Feb 17, 2016

Yea, this looks right. I can't see how steps 13-14 could possibly happen but if it could, then yea, that would be the right call.

The point of decoupling this is so that in theory, someone else could've implemented this perf tool without changing the core. Since you're effectively mapping the names of methods 1:1, it doesn't create much decoupling. If we change the algorithm, you would still have to go change the ReactNewPerf implementation.

A good guideline is to put things that are likely to change together into the same file. So, if we can't decouple, we might as well put all of ReactNewPerf into ReactCompositeComponent etc. to make it easy to refactor.

I understand that this is your later step "It should not rely on the rendering and mounting stack matching parent hierarchy".

One suggestion that I have is that you could, have start and stop timer associated with component time in ReactNewPerf. E.g. startTime(internalInstance) and then call stopTime(internalInstance) right before the recursive call. Then you call startTime(internalInstance) again right after the recursive call completes. You only calculate exclusive time. Then at the end, the summary, you sum them up according to the tree to get the inclusive time.

That way we can restructure the order completely without changing ReactNewPerf.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 17, 2016

Member

One suggestion that I have is that you could, have start and stop timer associated with component time in ReactNewPerf. E.g. startTime(internalInstance) and then call stopTime(internalInstance) right before the recursive call. Then you call startTime(internalInstance) again right after the recursive call completes. You only calculate exclusive time. Then at the end, the summary, you sum them up according to the tree to get the inclusive time.

We could pass something like startTime(internalInstance, 'render'), startTime(internalInstance, 'mount'), so that the profiler does not really know about the lifecycle other than that “work denoted by some arbitrary label is being done for this component”. This makes it easy to add other “types” of work later that the analysis code can interpret as desired (e.g. “A spent X ms on render, Y ms on mount, Z ms on layout”). This also makes the profiler API easier to understand because, rather than memorize which methods require Perf calls you just remember to track whatever you think is worth tracking with startTime and stopTime methods.

Is this what you’re getting at?

Member

gaearon commented Feb 17, 2016

One suggestion that I have is that you could, have start and stop timer associated with component time in ReactNewPerf. E.g. startTime(internalInstance) and then call stopTime(internalInstance) right before the recursive call. Then you call startTime(internalInstance) again right after the recursive call completes. You only calculate exclusive time. Then at the end, the summary, you sum them up according to the tree to get the inclusive time.

We could pass something like startTime(internalInstance, 'render'), startTime(internalInstance, 'mount'), so that the profiler does not really know about the lifecycle other than that “work denoted by some arbitrary label is being done for this component”. This makes it easy to add other “types” of work later that the analysis code can interpret as desired (e.g. “A spent X ms on render, Y ms on mount, Z ms on layout”). This also makes the profiler API easier to understand because, rather than memorize which methods require Perf calls you just remember to track whatever you think is worth tracking with startTime and stopTime methods.

Is this what you’re getting at?

@jstrimpel jstrimpel referenced this pull request in jstrimpel/react-perf-utils Feb 17, 2016

Open

watch new perf work #2

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 17, 2016

Member

Even better!

Member

sebmarkbage commented Feb 17, 2016

Even better!

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 17, 2016

Member

Ideally the tree information would be exposed through the new devtools API, so perhaps it would be sufficient for the perf tools to use that rather than building another one specifically for perf?

Member

sebmarkbage commented Feb 17, 2016

Ideally the tree information would be exposed through the new devtools API, so perhaps it would be sufficient for the perf tools to use that rather than building another one specifically for perf?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 17, 2016

Member

This looks good so far. I'm going to mark this as needs-revision just because we don't want to land it until it is gated behind something.

Member

sebmarkbage commented Feb 17, 2016

This looks good so far. I'm going to mark this as needs-revision just because we don't want to land it until it is gated behind something.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 17, 2016

Member

Ideally the tree information would be exposed through the new devtools API, so perhaps it would be sufficient for the perf tools to use that rather than building another one specifically for perf?

Can do that. But it’s not exposed yet, is it? Meaning I’d need to add these events to the devtool code myself.

Currently I don’t have enough context about what kind of information DevTools want. I can start by firing notifications on the devtool for every lifecycle hook with the internal instance. Would that be sufficient? ReactNewPerf would tune in to "mounted" and "unmounted" events and use this as a chance to track the owners.

If I do this, we’ll need to gate devtool API by something like __DEV__ || __PROFILE__. Right now it’s __DEV__-only.

Member

gaearon commented Feb 17, 2016

Ideally the tree information would be exposed through the new devtools API, so perhaps it would be sufficient for the perf tools to use that rather than building another one specifically for perf?

Can do that. But it’s not exposed yet, is it? Meaning I’d need to add these events to the devtool code myself.

Currently I don’t have enough context about what kind of information DevTools want. I can start by firing notifications on the devtool for every lifecycle hook with the internal instance. Would that be sufficient? ReactNewPerf would tune in to "mounted" and "unmounted" events and use this as a chance to track the owners.

If I do this, we’ll need to gate devtool API by something like __DEV__ || __PROFILE__. Right now it’s __DEV__-only.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 17, 2016

Member

Currently these events are exposed through the devtools (search for hook.emit):

https://github.com/facebook/react-devtools/blob/master/backend/attachRenderer.js

(Note that the terminology is a bit confusing there. The element refers to an internal instance in React.)

The "backend" part of the devtools is an event protocol designed by @jaredly to be sufficient to track the tree. A good start might be to mirror that API.

use this as a chance to track the owners.

I think you mean the parents. We use the term "owner" for the thing that created the element which is not the same as the thing mounting it - which is the parent. To track inclusive time you need the parent.

It is unfortunate that tracking the tree might add a lot of overhead but it is probably nice to be able to visualize this in terms of a full tree.

ReactDOMInstrumentation is only for DOM specific operations so you'd want to replicate it with a ReactInstrumentation file that does the same thing.

The idea is that we can check for if (ReactInstrumentation.debugTool) to quickly bailout if nobody is listening.

Currently the devtools work in production mode because it just uses monkey patching. That sucks for optimizations and package size. Maybe a __DEV__ || __PROFILE__ check would be better.

Member

sebmarkbage commented Feb 17, 2016

Currently these events are exposed through the devtools (search for hook.emit):

https://github.com/facebook/react-devtools/blob/master/backend/attachRenderer.js

(Note that the terminology is a bit confusing there. The element refers to an internal instance in React.)

The "backend" part of the devtools is an event protocol designed by @jaredly to be sufficient to track the tree. A good start might be to mirror that API.

use this as a chance to track the owners.

I think you mean the parents. We use the term "owner" for the thing that created the element which is not the same as the thing mounting it - which is the parent. To track inclusive time you need the parent.

It is unfortunate that tracking the tree might add a lot of overhead but it is probably nice to be able to visualize this in terms of a full tree.

ReactDOMInstrumentation is only for DOM specific operations so you'd want to replicate it with a ReactInstrumentation file that does the same thing.

The idea is that we can check for if (ReactInstrumentation.debugTool) to quickly bailout if nobody is listening.

Currently the devtools work in production mode because it just uses monkey patching. That sucks for optimizations and package size. Maybe a __DEV__ || __PROFILE__ check would be better.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 17, 2016

Member

Note that a __PROFILE__ flag might be difficult to add to our internal FB build but if we start deploying the from the npm package internally, that wouldn't be an issue.

Member

sebmarkbage commented Feb 17, 2016

Note that a __PROFILE__ flag might be difficult to add to our internal FB build but if we start deploying the from the npm package internally, that wouldn't be an issue.

@iamdustan

This comment has been minimized.

Show comment
Hide comment
@iamdustan

iamdustan Feb 17, 2016

Contributor

:) I am really enjoying this thread. I like to refer as the current devtools work as duck punch monkey patching since it is doing duck punch detection for ReactDOM 0.13, 0.14, 0.15, and RN.

Contributor

iamdustan commented Feb 17, 2016

:) I am really enjoying this thread. I like to refer as the current devtools work as duck punch monkey patching since it is doing duck punch detection for ReactDOM 0.13, 0.14, 0.15, and RN.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 17, 2016

Member

Currently these events are exposed through the devtools (search for hook.emit):

Thank you, this is very helpful.

I think you mean the parents. We use the term "owner" for the thing that created the element which is not the same as the thing mounting it - which is the parent. To track inclusive time you need the parent.

Thanks for correcting me. I thought owners should be used for the inclusive calculation but I trust you that parents make more sense here. I don’t really understand the problem well enough yet to see why.

ReactDOMInstrumentation is only for DOM specific operations so you'd want to replicate it with a ReactInstrumentation file that does the same thing.

👍

Currently the devtools work in production mode because it just uses monkey patching. That sucks for optimizations and package size. Maybe a DEV || PROFILE check would be better.

Since this is all very confusing I’ll finish the implementation first, and then we’ll decide how to proceed with the feature flags both for Perf and tree tracking.

Member

gaearon commented Feb 17, 2016

Currently these events are exposed through the devtools (search for hook.emit):

Thank you, this is very helpful.

I think you mean the parents. We use the term "owner" for the thing that created the element which is not the same as the thing mounting it - which is the parent. To track inclusive time you need the parent.

Thanks for correcting me. I thought owners should be used for the inclusive calculation but I trust you that parents make more sense here. I don’t really understand the problem well enough yet to see why.

ReactDOMInstrumentation is only for DOM specific operations so you'd want to replicate it with a ReactInstrumentation file that does the same thing.

👍

Currently the devtools work in production mode because it just uses monkey patching. That sucks for optimizations and package size. Maybe a DEV || PROFILE check would be better.

Since this is all very confusing I’ll finish the implementation first, and then we’ll decide how to proceed with the feature flags both for Perf and tree tracking.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 17, 2016

Member

then we’ll decide how to proceed with the feature flags both for Perf and tree tracking.

Sure. If you want, you can also just add a temporary constant flag (always false). That way you can incrementally land your changes in master so that you don't need to deal with merge conflicts.

Member

sebmarkbage commented Feb 17, 2016

then we’ll decide how to proceed with the feature flags both for Perf and tree tracking.

Sure. If you want, you can also just add a temporary constant flag (always false). That way you can incrementally land your changes in master so that you don't need to deal with merge conflicts.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 18, 2016

Member

The idea is that we can check for if (ReactInstrumentation.debugTool) to quickly bailout if nobody is listening.

Currently, in ReactDOMInstrumentation, ReactDOMInstrumentation.debugTool is always defined because it’s a thing that lets other devtools register themselves. Is the plan to reorganize this in some way, or do it differently for ReactInstrumentation? Otherwise I don’t see how you could exit early by checking it.

Member

gaearon commented Feb 18, 2016

The idea is that we can check for if (ReactInstrumentation.debugTool) to quickly bailout if nobody is listening.

Currently, in ReactDOMInstrumentation, ReactDOMInstrumentation.debugTool is always defined because it’s a thing that lets other devtools register themselves. Is the plan to reorganize this in some way, or do it differently for ReactInstrumentation? Otherwise I don’t see how you could exit early by checking it.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 18, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 19, 2016

Member

E.g. startTime(internalInstance) and then call stopTime(internalInstance) right before the recursive call. Then you call startTime(internalInstance) again right after the recursive call completes

So far I haven’t been able to do this if startTime(internalInstance) and stopTime(internalInstance) are only called for composites. In this case it seems too hard to tell when DOM components below should have effect on the composite’s time. We can’t just “exit before recursive call” because we want DOM component mounts to count towards closest parent composite’s exclusive time. So we have to thread the closest parent composite instance all the way down so DOM components can call startTimer(closestParentCompositeInternalInstance) while they do the DOMy things.

I’m not sure whether this is the approach you had in mind but it seems like a lot of changes so I want to double-check. Maybe we can use context for passing closestParentCompositeInternalInstance in dev. Another thing I dislike about this approach is it offloads logging the time for a composite onto the specific native nodes. So if DOM components do this, we need to teach iOS components to also do this. This feels off.

Alternatively we can change the logic so that even DOM components get their own exclusive time measurements with startTime(internalInstance). Maybe this is what you meant? Since we have the tree information, we should be able to calculate exclusive composite timing from their really-exclusive timings + exclusive timings of DOM components which have those composites as the closest parents. So this would be a three-step calculation based on the tree: first collect all really-exclusive timings, then map DOM components to composites and calculate exclusive-but-not-really-because-including-DOM timings of composites, then calculate inclusive-because-also-including-child-composites timings based on the parent tree. Does this make more sense? In this case other native components such as RN would still have time startTime(internalInstance) but at least they would pass themselves rather than some composite they received. Additionally they would have the freedom to specify something else as we discussed (startTime(internalInstance, 'svg-node-update') etc).

Member

gaearon commented Feb 19, 2016

E.g. startTime(internalInstance) and then call stopTime(internalInstance) right before the recursive call. Then you call startTime(internalInstance) again right after the recursive call completes

So far I haven’t been able to do this if startTime(internalInstance) and stopTime(internalInstance) are only called for composites. In this case it seems too hard to tell when DOM components below should have effect on the composite’s time. We can’t just “exit before recursive call” because we want DOM component mounts to count towards closest parent composite’s exclusive time. So we have to thread the closest parent composite instance all the way down so DOM components can call startTimer(closestParentCompositeInternalInstance) while they do the DOMy things.

I’m not sure whether this is the approach you had in mind but it seems like a lot of changes so I want to double-check. Maybe we can use context for passing closestParentCompositeInternalInstance in dev. Another thing I dislike about this approach is it offloads logging the time for a composite onto the specific native nodes. So if DOM components do this, we need to teach iOS components to also do this. This feels off.

Alternatively we can change the logic so that even DOM components get their own exclusive time measurements with startTime(internalInstance). Maybe this is what you meant? Since we have the tree information, we should be able to calculate exclusive composite timing from their really-exclusive timings + exclusive timings of DOM components which have those composites as the closest parents. So this would be a three-step calculation based on the tree: first collect all really-exclusive timings, then map DOM components to composites and calculate exclusive-but-not-really-because-including-DOM timings of composites, then calculate inclusive-because-also-including-child-composites timings based on the parent tree. Does this make more sense? In this case other native components such as RN would still have time startTime(internalInstance) but at least they would pass themselves rather than some composite they received. Additionally they would have the freedom to specify something else as we discussed (startTime(internalInstance, 'svg-node-update') etc).

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 19, 2016

Member

I updated the PR to show my current approach with DOM instances reporting their own time.

Member

gaearon commented Feb 19, 2016

I updated the PR to show my current approach with DOM instances reporting their own time.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 19, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 19, 2016

Member

The mental model I have is that DOM components are just another composite component. Their time shouldn't count to the exclusive time of the composite above them. If it does in the old perf tools, that seems like an artifact/bug rather than desired behavior.

If a composite passes a bunch of children to another composite. Then it would be misleading to include those in the exclusive time of the parent when the owner really created it.

It seems fine to put these calls into the native components.

However, note that I also want to unify the way native components are rendered so that a lot of that wouldn't vary by renderer eventually. The recursion will move into shared code. That way we could put these measurements in a generic way. That would preclude renderer specific tags like svg-node-update but simplify each renderer. For now it is fine to put them in each renderer.

Member

sebmarkbage commented Feb 19, 2016

The mental model I have is that DOM components are just another composite component. Their time shouldn't count to the exclusive time of the composite above them. If it does in the old perf tools, that seems like an artifact/bug rather than desired behavior.

If a composite passes a bunch of children to another composite. Then it would be misleading to include those in the exclusive time of the parent when the owner really created it.

It seems fine to put these calls into the native components.

However, note that I also want to unify the way native components are rendered so that a lot of that wouldn't vary by renderer eventually. The recursion will move into shared code. That way we could put these measurements in a generic way. That would preclude renderer specific tags like svg-node-update but simplify each renderer. For now it is fine to put them in each renderer.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Feb 19, 2016

Member

However, note that I also want to unify the way native components are rendered so that a lot of that wouldn't vary by renderer eventually. The recursion will move into shared code.

👍

If a composite passes a bunch of children to another composite. Then it would be misleading to include those in the exclusive time of the parent when the owner really created it.

This might be why I thought we should use the owner tree for calculating inclusive time.

Their time shouldn't count to the exclusive time of the composite above them. If it does in the old perf tools, that seems like an artifact/bug rather than desired behavior.

Interesting. I definitely think that it does in the old perf tools. In the old perf tools, anything until the next composite is on the mounting stack counts towards the previous top composite on the stack.

What would be the point of exclusive metrics than don’t count DOM components towards the composites? Would information about DOM components be included in the metrics at all?

Currently, ReactPerf groups all information by composites only. Aside from the log of all DOM changes (which goes separately from the inclusive/exclusive tables), ReactPerf currently does not provide you specific information about the DOM components. If we include DOM components in the measurements, would we not have a useless table dominated by <div>s with thousands of entries? I’m not sure. (But I can check.)

Also, if we only measure composites’ own time, what will we be measuring exactly? Time spent in constructor and componentWillMount? I would think that “mounting” time for composites as reported by ReactPerf should refer to the DOM work of mounting. What are your thoughts on this?

(Now I decided to do my own homework and read the docs. Apparently what you’re saying is exactly what we’ve been promising all along:)

"Exclusive" times don't include the times taken to mount the components: processing props, getInitialState, call componentWillMount and componentDidMount, etc.

So I guess this is what I’ll do then.

Member

gaearon commented Feb 19, 2016

However, note that I also want to unify the way native components are rendered so that a lot of that wouldn't vary by renderer eventually. The recursion will move into shared code.

👍

If a composite passes a bunch of children to another composite. Then it would be misleading to include those in the exclusive time of the parent when the owner really created it.

This might be why I thought we should use the owner tree for calculating inclusive time.

Their time shouldn't count to the exclusive time of the composite above them. If it does in the old perf tools, that seems like an artifact/bug rather than desired behavior.

Interesting. I definitely think that it does in the old perf tools. In the old perf tools, anything until the next composite is on the mounting stack counts towards the previous top composite on the stack.

What would be the point of exclusive metrics than don’t count DOM components towards the composites? Would information about DOM components be included in the metrics at all?

Currently, ReactPerf groups all information by composites only. Aside from the log of all DOM changes (which goes separately from the inclusive/exclusive tables), ReactPerf currently does not provide you specific information about the DOM components. If we include DOM components in the measurements, would we not have a useless table dominated by <div>s with thousands of entries? I’m not sure. (But I can check.)

Also, if we only measure composites’ own time, what will we be measuring exactly? Time spent in constructor and componentWillMount? I would think that “mounting” time for composites as reported by ReactPerf should refer to the DOM work of mounting. What are your thoughts on this?

(Now I decided to do my own homework and read the docs. Apparently what you’re saying is exactly what we’ve been promising all along:)

"Exclusive" times don't include the times taken to mount the components: processing props, getInitialState, call componentWillMount and componentDidMount, etc.

So I guess this is what I’ll do then.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Feb 19, 2016

Member

In the current table view you're right that we probably don't want to show DOM components as their own rows. But if we showed it in a tree view it could make a lot more sense. Maybe the ideal is that the visualization lets you enable and disable seeing DOM components when you're actually looking at the data.

We should really include all of those lifecycle methods in the measurements. I consider it a bug that we currently don't. componentDidMount is an example of one that runs later so that could give you an opportunity to make it work for out-of-order reconciliation. Ideally we can also split out the time for each lifecycle method so you can look if you notice a slow component and want to know what piece to optimize.

Member

sophiebits commented Feb 19, 2016

In the current table view you're right that we probably don't want to show DOM components as their own rows. But if we showed it in a tree view it could make a lot more sense. Maybe the ideal is that the visualization lets you enable and disable seeing DOM components when you're actually looking at the data.

We should really include all of those lifecycle methods in the measurements. I consider it a bug that we currently don't. componentDidMount is an example of one that runs later so that could give you an opportunity to make it work for out-of-order reconciliation. Ideally we can also split out the time for each lifecycle method so you can look if you notice a slow component and want to know what piece to optimize.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 19, 2016

Member

Don't forget that the render method is the primary thing to measure for a composite. They all have one and sometimes it can be slow.

The DOM operation isn't necessarily the slowest part. Once diffed, the work that is actually touching the DOM, more often than not, is something that you have to do. So it is not very actionable.

Member

sebmarkbage commented Feb 19, 2016

Don't forget that the render method is the primary thing to measure for a composite. They all have one and sometimes it can be slow.

The DOM operation isn't necessarily the slowest part. Once diffed, the work that is actually touching the DOM, more often than not, is something that you have to do. So it is not very actionable.

Sumei1009 pushed a commit to Sumei1009/react that referenced this pull request Apr 14, 2016

Add ReactDebugInstanceMap
This PR is the first in a series of pull requests split from the new `ReactPerf` implementation in #6046.

Here, we introduce a new module called `ReactDebugInstanceMap`. It will be used in `__DEV__` and, when the `__PROFILE__` gate is added, in the `__PROFILE__` builds. It will *not* be used in the production builds.

This module acts as a mapping between “debug IDs” (a new concept) and the internal instances. Not to be confused with the existing `ReactInstanceMap` that maps internal instances to public instances.

What are the “debug IDs” and why do we need them? Both the new `ReactPerf` and other consumers of the devtool API, such as React DevTools, need access to some data from the internal instances, such as the instance type display name, current props and children, and so on. Right now we let such tools access internal instances directly but this hurts our ability to refactor their implementations and burdens React DevTools with undesired implementation details such as having to support React ART in a special way.[1]

The purpose of adding `ReactDebugInstanceMap` is to only expose “debug IDs” instead of the internal instances to any devtools. In a future RP, whenever there is an event such as mounting, updating, or unmounting a component, we will emit an event in `ReactDebugTool` with the debug ID of the instance. We will also add an introspection API that lets the consumer pass an ID and get the information about the current children, props, state, display name, and so on, without exposing the internal instances.

`ReactDebugInstanceMap` has a concept of “registering” an instance. We plan to add the hooks that register an instance as soon as it is created, and unregister it during unmounting. It will only be possible to read information about the instance while it is still registered. If we add support for reparenting, we should be able to move the (un)registration code to different places in the component lifecycle without changing this code. The currently registered instances are held in the `registeredInstancesByIDs` dictionary.

There is also a reverse lookup dictionary called `allInstancesToIDs` which maps instances back to their IDs. It is implemented as a `WeakMap` so the keys are stable and we’re not holding onto the unregistered instances. If we’re not happy with `WeakMap`, one possible alternative would be to add a new field called `_debugID` to all the internal instances, but we don’t want to do this in production. Using `WeakMap` seems like a simpler solution here (and stable IDs are a nice bonus). This, however, means that the `__DEV__` (and the future `__PROFILE__`) builds will only work in browsers that support our usage of `WeakMap`.

[1]: https://github.com/facebook/react-devtools/blob/577ec9b8d994fd26d76feb20a1993a96558b7745/backend/getData.js
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 23, 2016

Member

Rebased this on top of #6549.

Member

gaearon commented Apr 23, 2016

Rebased this on top of #6549.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 23, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 24, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 24, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 24, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 24, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 26, 2016

@gaearon updated the pull request.

ghost commented Apr 26, 2016

@gaearon updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 27, 2016

@gaearon updated the pull request.

ghost commented Apr 27, 2016

@gaearon updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 27, 2016

@gaearon updated the pull request.

ghost commented Apr 27, 2016

@gaearon updated the pull request.

+ return flushHistory;
+ }
+ },
+ onBeginFlush() {

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 28, 2016

Member

Can you explain a bit on why the flush batch is important? Does it matter if the implementation batches or just does the work whenever?

@sebmarkbage

sebmarkbage Apr 28, 2016

Member

Can you explain a bit on why the flush batch is important? Does it matter if the implementation batches or just does the work whenever?

This comment has been minimized.

@gaearon

gaearon Apr 28, 2016

Member

I think this comes down to the way printWasted() heuristic works. It says “count a render as wasted if no DOM operations below were made during the same batch”. I was trying to replicate the existing behavior so this is the main reason.

I think we may want to reconsider this as part of #6632. I’d leave it as is for now though.

@gaearon

gaearon Apr 28, 2016

Member

I think this comes down to the way printWasted() heuristic works. It says “count a render as wasted if no DOM operations below were made during the same batch”. I was trying to replicate the existing behavior so this is the main reason.

I think we may want to reconsider this as part of #6632. I’d leave it as is for now though.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 28, 2016

@gaearon updated the pull request.

ghost commented Apr 28, 2016

@gaearon updated the pull request.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Apr 28, 2016

@gaearon updated the pull request.

ghost commented Apr 28, 2016

@gaearon updated the pull request.

@gaearon gaearon changed the title from [WIP] New ReactPerf to Add new ReactPerf Apr 28, 2016

@gaearon gaearon added this to the 15.x milestone Apr 28, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 28, 2016

@gaearon updated the pull request.

@gaearon updated the pull request.

@gaearon gaearon referenced this pull request in facebook/react-native Apr 28, 2016

Closed

Update to new ReactPerf #7283

@gaearon gaearon merged commit 98a8f49 into facebook:master Apr 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gaearon gaearon deleted the gaearon:new-perf branch Apr 29, 2016

zpao added a commit that referenced this pull request May 10, 2016

Merge pull request #6046 from gaearon/new-perf
Add new ReactPerf
(cherry picked from commit 98a8f49)

@zpao zpao modified the milestones: 15.1.0, 15.y.0 May 20, 2016

@nathanmarks nathanmarks referenced this pull request in mui-org/material-ui May 21, 2016

Open

[Discussion] Benchmark components #4305

@renovate renovate bot referenced this pull request in signavio/backbone-rel Feb 2, 2018

Open

Update dependency react to v16 #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment