Porting ReactPerf to the new DevTools API

[gaearon]

There are a few concerns that I heard about ReactPerf, in the order of importance:

• (1) It is gated by __DEV__ but React Native wants to have a way to enable it in production builds and pass the accumulated info into systrace
• (2) It is often broken by refactoring
• (3) It appears generic but depends on specific module and method names
• (4) It has some obscure bugs that are hard to fix because of the coupling
• (5) The console.table() visualization is poor
• (6) We are working on a new DevTools API so they should align

If I’m mistaken here, please let me know!

In #5306, we started introducing a new set of APIs for first-class instrumentation. It makes sense that we transition ReactPerf to the new API, and fix some of these issues while we’re at it.

I talked to some people on the React Native about this. What I understand so far is:

ASAP

• We want to remove ReactPerf function wrapping completely (addresses 2, somewhat mitigates overhead from fixing 1)
• We want to create a devtool like ReactDOMDebugTool, e.g. ReactPerfTool (addresses 6)
• Unlike ReactDOMDebugTool, ReactPerfTool will not be gated by __DEV__ (addresses 1)
• ReactPerfTool will have startMeasurement() and endMeasurement() methods for profiling functions (addresses 1)
• It can have more granular methods for specific parts of React lifecycle if needed (addresses 2, 3, 5)
• Calls to ReactPerfTool will be gated by a runtime flag, e.g. ReactPerfTool.isActive (addresses 1)

In the Future

• Rather than reach out into the profiled functions, startMeasurement() will accept arbitrary arguments (addresses 3)
• We would separate generating user-meaningful data from its accumulation so it would be easy to test (addresses 4)
• (Bonus) We can use a noop decorator + Babel plugin to insert if (ReactPerfTool.isActive) ReactPerfTool.startMeasurement() and .endMeasurement() calls (mitigates pain and potential breakage from addressing 1)
• (Bonus) Rather than console.table() API we can provide a component that interprets that data and displays it in an overlay, both on web and native (addresses 5)

The decorator + Babel plugin part is the one I’m not sure about because it would involve complicating tooling. On the other hand, it will allow adding performance measurement to any functions in the codebase without risking having early returns, missing endMeasurement() calls, and similar breakage during refactorings.

The first actionable step, in my opinion, would be to remove the function wrapping and the __DEV__ gate from the existing ReactPerf, and replace measure() with explicit startMeasurement() and endMeasurement() calls gated by ReactPerf.isActive wherever it is used.

We would still pass the function as an argument so we don’t have to rewrite everything at once, but this would give the RN team more freedom, and unlock future refactorings. We can also combine this with introducing decorator + Babel transform if this is the way we want to go, to avoid adding manual startMeasurement() and endMeasurement() calls all over the place.

Does this make sense? What have I missed?

[sebmarkbage]

An existing issue is that AOP style measurement isn't always very convenient because when we wrap recursive function calls it is difficult to measure the "self time" of a component.

Another issue is that the numbers are intuitive but not necessarily very actionable. That would probably require some more thinking around how we can design this.

Turning measure() into if (ReactPerf.isActive) { startMeasurement(); } seems like a good first step.

However, I'd like it even more if we did something like:

var isPerfActive = ReactPerf.isActive;
function foo() {
  if (isPerfActive) { startMeasurement(); }
};

That avoids an indirection in the runtime look up at each callsite.

This becomes a constant so that the JIT or compiler can safely exclude these code paths.

The downside is that you can't turn profiling on/off while it is already running. I don't think that is a required use case though.

Another alternative is that we simply build three different builds of PROD/PROFILE/DEBUG and then the packager can choose if it wants to profile in production or not.

In that case maybe we should have:

if (__PROFILE__) { startMeasurement(); }

or something.

[sebmarkbage]

The points I care most about are:

A) ReactPerf should not reach into internal properties, event and component names etc. Everything in needs should be colocated with the code so that we can change implementation details without breaking it. One way to achieve that could be to fire events and then have the tree reconstructed by the perf tool. However, it should also not rely on execution order of those events. E.g. we will probably want to render children in a different stack frame from the parent. Therefore the inclusive/exclusive perf measurement of a parent can't depend on a child rendering within the stack of the parent rendering.

This might mean that the React code needs to have some high level knowledge about the use case rather than just spewing out events.

B) We should not make things slower because we add profiling. Not even temporarily. We need to have a way to disable it completely from day one.

[gaearon]

Turning measure() into if (ReactPerf.isActive) { startMeasurement(); } seems like a good first step.

I’m on it then. 👍

The downside is that you can't turn profiling on/off while it is already running. I don't think that is a required use case though.

@tadeuzagallo told me that this is a use case that RN team has. I don’t have the details but maybe he can provide them.

In the meantime, I can do as you suggested first. Then I can measure the perf impact from making it a lookup, and we can reconsider if the use case is compelling enough.

Another alternative is that we simply build three different builds of PROD/PROFILE/DEBUG and then the packager can choose if it wants to profile in production or not.

One of the reasons RN team wants a revamp of ReactPerf is that there is unnecessary friction in getting the trace results. Adding another build would increase the friction compared to a runtime switch. (Even a runtime switched computed at start time would amount to less friction.) Therefore I’m going with the runtime flag, and we can reconsider later if we find good reasons to do a separate build. (For example, if performance regression from having a flag will turn out worse than our expectations.)

ReactPerf should not reach into internal properties, event and component names etc. Everything in needs should be colocated with the code so that we can change implementation details without breaking it. One way to achieve that could be to fire events and then have the tree reconstructed by the perf tool.

I’ll keep in mind that this is a bigger priority than it seemed to me at first. Implementing this would require a significant revamp of ReactPerf. On the other hand it seems to require us to start emitting lifecycle events which may be useful to React DevTools Extension later. I will need to explore how much work it is to remove reliance on the implementation details in ReactPerf.

We should not make things slower because we add profiling. Not even temporarily. We need to have a way to disable it completely from day one.

We can start by adding a one-time runtime switch and measure. If we regress above noise, we can resort to making a special profiling build. Then if it is very frustrating to the RN team to switch between builds we can make PROFILE build the default one for most cases (except shipping to AppStore I guess?).

[sebmarkbage]

The thing that worries me about this is that the usage of this pattern inside React will increase plus when we have less noise and do more inside React (e.g. Layout) it will start being noticeable. This will become very hot paths.

That's why I'm not really satisfied with it being below noise today.

It also just seems wasteful if this switch is always off in production builds.

If RN wants an easy toggle, why not just always run the PROFILE build during development? No friction.

On Feb 11, 2016, at 8:24 AM, Dan Abramov notifications@github.com wrote:

Turning measure() into if (ReactPerf.isActive) { startMeasurement(); } seems like a good first step.

I’m on it then.

The downside is that you can't turn profiling on/off while it is already running. I don't think that is a required use case though.

@tadeuzagallo told me that this is a use case that RN team has. I don’t have the details but maybe he can provide them.

In the meantime, I can do as you suggested first. Then I can measure the perf impact from making it a lookup, and we can reconsider if the use case is compelling enough.

Another alternative is that we simply build three different builds of PROD/PROFILE/DEBUG and then the packager can choose if it wants to profile in production or not.

One of the reasons RN team wants a revamp of ReactPerf is that there is unnecessary friction in getting the trace results. Adding another build would increase the friction compared to a runtime switch. (Even a runtime switched computed at start time would amount to less friction.) Therefore I’m going with the runtime flag, and we can reconsider later if we find good reasons to do a separate build. (For example, if performance regression from having a flag will turn out worse than our expectations.)

ReactPerf should not reach into internal properties, event and component names etc. Everything in needs should be colocated with the code so that we can change implementation details without breaking it. One way to achieve that could be to fire events and then have the tree reconstructed by the perf tool.

I’ll keep in mind that this is a bigger priority than it seemed to me at first. Implementing this would require a significant revamp of ReactPerf. On the other hand it seems to require us to start emitting lifecycle events which may be useful to Chrome DevTools later. I will need to explore how much work it is to remove reliance on the implementation details in ReactPerf.

We should not make things slower because we add profiling. Not even temporarily. We need to have a way to disable it completely from day one.

We can start by adding a one-time runtime switch and measure. If we regress above noise, we can resort to making a special profiling build. Then if it is very frustrating to the RN team to switch between builds we can make PROFILE build the default one for most cases (except shipping to AppStore I guess?).

—
Reply to this email directly or view it on GitHub.

[gaearon]

If RN wants an easy toggle, why not just always run the PROFILE build during development? No friction.

OK!

However, it should also not rely on execution order of those events. E.g. we will probably want to render children in a different stack frame from the parent. Therefore the inclusive/exclusive perf measurement of a parent can't depend on a child rendering within the stack of the parent rendering.

After putting some logs here and there I have a better idea about how ReactPerf currently works:

So what you’re saying is that it’s best we don’t rely on _renderValidatedComponent and mountComponent happening inside as part of the parent’s mountComponent. The future plan is to allow deferring some child updates to next batches. This would mean that a child could be reconciled during the next flushBatchedUpdates() while its parent is not on the stack.

However every change would still happen inside flushBatchedUpdates() so we can keep the existing measurements result structure where every single entry corresponds to a batch. The only thing we would need to change is to count inclusive and exclusive times differently without relying on the execution order of components themselves.

Would inclusive even make sense in a world like this? If child update is deferred to another batch than the parent’s reconciliation, does that count as inclusive time towards the parent?

Is my understanding of the issue correct so far?

[gaearon]

Superseded by #6046.

[gfogle]

A) ReactPerf should not reach into internal properties, event and component names etc. Everything in needs should be colocated with the code so that we can change implementation details without breaking it.

This turned out to be fairly important when I was working on #5264 because the internal id of each composite component, which i was using as a key on the measurements object, was removed and broke the perf code. I ended up having to require keys to be on the each component which seems bad to me. Does ReactPerf attaching properties, such as reactPerfInstanceID or something very obscure to avoid future name collisions, to the components break this as well?