Profiler should measure commit phase durations

[bvaughn]

View formatted RFC

Implementation: facebook/react#17910

[bvaughn]

Should this RFC drop passive effect timing?

As I dig into actually implementing this, I've realized there are a few challenges presented by passive effects. The biggest is that we can't call onPostCommit during the commit phase tree traversal because:

1. Passive effects won't have been processed by this time (at least not for the render work we're currently committing). Duration values will represent the previous round of passive effects.
2. We could call the previous callback at this time, although that presents additional problems:
   1. The memoizedInteractions set corresponds to the current batch of render work, so it would be incorrect.
   2. The memoizedProps.id of the Profiler also corresponds to the current batch of render work. (It may have changed since the render our passive effects are part of.)
   3. This would make resetting the passive effect durations a little more complicated as well. I think we should do this during the begin phase, so DevTools has a chance to read these values, but if we did that we'd erase the previous passive effect durations too.

I think this means that we would need to do the following:

1. Add a traversal pass to flushPassiveEffectsImpl that:
   1. Bubbles passive effect durations up through all ancestor Profilers
   2. Calls onPostCommit for any Profilers with this prop registered
2. Add a new DevTools hook (so the Profiler actually has a way to read these transient values if it wants to report them in the Profiler somehow). Normally DevTools reads values in its commit hook, but this won't work for passive effect durations.

Adding a new DevTools hook is probably fine. I'm less sure about adding the extra traversal passes. (Do passive effect durations actually provide enough value to offset this?) I'll go ahead and do it for now, but I assume we may want to re-evaluate the cost/benefit during the PR stage.

[bvaughn]

Should we measure cleanup functions?

This is tricky in a similar way as passive effects, since cleanup functions don't get run until an update (or unmount)- after we've already reported durations.

We have a couple of options here. I'm going to list them from least ideal to most ideal:

1. ☆☆☆ Delay reporting commit durations until the next commit (after cleanup functions have been run) so the durations are inclusive of both. This has similar complexity downsides as mentioned above for passive effects though.
2. ★☆☆ Add new callbacks for cleanup durations (e.g. onCommitCleanup? onCommitDestroy?) and report them separately. This would provide the most insight into where slowness comes from, but I also think this might be overkill.
3. ★★☆ Don't measure these durations. Typically cleanup functions are small (e.g. removing an event listener). This would be a continued blindspot in the Profiler API though.
4. ★★★ Measure cleanup functions, but include their durations as part of the current commit. An argument could be made for this being the best way to think of them, since their durations impact the current commit (not the previous one), although it might be confusing since e.g. interactions are for the current commit.

My current vote is for option 4 above (include their durations as part of the current commit).

Should we also measure "before mutation" code?

Currently this only includes the class component getSnapshotBeforeUpdate lifecycle, but we will probably add a before-mutation hook at some point too.

Given the currently proposed callbacks (onRender, onCommit, onPostCommit) it might make sense to add a new Profiler hook (onPreCommit?) for "before mutation" lifecycles. Could be done as part of this RFC, or as part of a follow up.

I am currently thinking that it might be better to track this with a separate RFC. Pre-mutation effects aren't very common, so it seems a little less urgent.

[bvaughn]

I've updated the RFC to incorporate the above comments.

[bvaughn]

Implementation PR has been accepted and merged, so I'm going to merge the RFC.