Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically Profile roots when DevTools is present #13058

Merged
merged 13 commits into from Jun 20, 2018

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jun 16, 2018

I'm working on a new DevTools plug-in that uses the new profiler timings to create charts about an application's rendering performance. One of these charts will be a flame graph that shows all fibers in an application. The width of each fiber will be based on its treeBaseTime (the time it last took to render) and the color will be based on how much time it took relative to the longest running parts of the commit. Here's a small demo:
react-devtools-profiling

Profiling when DevTools are present

My original idea was for DevTools to walk each root and turn ProfileMode on for all fibers when the user started profiling. (PR #12910 was created in order to support this functionality.) I made an oversight though. React does not collect timing information for fibers that aren't in ProfileMode, and so selfBaseTime and treeBaseTime values are all set to 0. This means that when a user starts profiling, base times are all 0. These times get updated when a fiber next renders– but not if it bails out. This means that in some cases, parts of the tree will not have base times and the resulting graph will be inaccurate.

There are a couple of options to address this:

  1. Profiling builds of React always collection timing information if DevTools has been detected. This way the base time information is already present when a user starts profiling. (This has an added benefit of not requiring DevTools to crawl each tree and modify the mode when recording starts.)
  2. DevTools somehow forces a deep re-render (that ignores shouldComponentUpdate) when profiling is started so that all fiber are guaranteed to have a base time value.

This PR adopts the first option, but I'm happy to discuss the second one if there are strong preferences or concerns.

Testing DevTools profiler integration

This PR also updates the test renderer to inject itself into DevTools if the global hook is present. This was done so that I could add a test for the otherwise unobservable profiling behavior. (It may also be useful for other tests.)

I made a few judgement calls here that I don't feel strongly about and would be happy to discuss changing:

  1. Test renderer does not support the findFiberByHostInstance method because I didn't want to add the overhead of mapping instances to fibers until/unless we had a need for that feature.
  2. Test renderer DevTools injection is based on a new supportDevToolsIfPresent feature flag, which is only enabled for the tests that depend on it.
  3. I updated the other DevTools-capable renderers to also check this feature flag because it seemed a bit weird not to. (I particularly don't feel strongly about this change.)
@pull-bot
Copy link

@pull-bot pull-bot commented Jun 16, 2018

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 8e87c13...854ef7e

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 623.3 KB 623.65 KB 145.64 KB 145.76 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 95.18 KB 95.18 KB 30.74 KB 30.73 KB UMD_PROD
react-dom.development.js +0.1% +0.1% 619.43 KB 619.78 KB 144.46 KB 144.58 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 95.17 KB 95.17 KB 30.29 KB 30.29 KB NODE_PROD
ReactDOM-dev.js +0.1% +0.1% 626.68 KB 627.04 KB 143.09 KB 143.21 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 271.76 KB 271.92 KB 51.22 KB 51.25 KB FB_WWW_PROD
react-dom.profiling.min.js +0.1% +0.1% 96.07 KB 96.14 KB 30.59 KB 30.61 KB NODE_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.2% 407.58 KB 407.93 KB 91.21 KB 91.35 KB UMD_DEV
react-art.development.js +0.1% +0.2% 339.55 KB 339.91 KB 74.1 KB 74.24 KB NODE_DEV
ReactART-dev.js +0.1% +0.2% 329.62 KB 329.97 KB 69.19 KB 69.33 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.1% 🔺+0.3% 142.8 KB 143 KB 24.55 KB 24.63 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +1.1% +1.5% 341.82 KB 345.61 KB 74.02 KB 75.11 KB UMD_DEV
react-test-renderer.production.min.js 🔺+2.5% 🔺+2.5% 47.29 KB 48.46 KB 14.51 KB 14.87 KB UMD_PROD
react-test-renderer.development.js +1.1% +1.5% 337.94 KB 341.73 KB 73.05 KB 74.13 KB NODE_DEV
react-test-renderer.production.min.js 🔺+2.5% 🔺+2.6% 46.99 KB 48.17 KB 14.32 KB 14.68 KB NODE_PROD
ReactTestRenderer-dev.js +1.1% +1.5% 341.13 KB 345 KB 71.83 KB 72.89 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.2% 330.08 KB 330.43 KB 70.58 KB 70.72 KB NODE_DEV
react-reconciler.production.min.js 0.0% -0.0% 47.09 KB 47.09 KB 14.09 KB 14.09 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% +0.2% 328.69 KB 329.04 KB 70.01 KB 70.15 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% -0.0% 47.11 KB 47.11 KB 14.1 KB 14.09 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 463.82 KB 464.17 KB 102 KB 102.12 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.1% 206.8 KB 206.96 KB 36.42 KB 36.47 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.1% +0.1% 463.53 KB 463.88 KB 101.94 KB 102.06 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 199.36 KB 199.36 KB 34.96 KB 34.96 KB RN_OSS_PROD
ReactFabric-dev.js +0.1% +0.1% 454.07 KB 454.42 KB 99.61 KB 99.72 KB RN_FB_DEV
ReactFabric-dev.js +0.1% +0.1% 454.11 KB 454.46 KB 99.63 KB 99.74 KB RN_OSS_DEV
ReactNativeRenderer-profiling.js +0.1% +0.1% 201.9 KB 202.03 KB 35.51 KB 35.56 KB RN_OSS_PROFILING
ReactFabric-profiling.js +0.1% +0.1% 193.87 KB 194.01 KB 34.03 KB 34.07 KB RN_OSS_PROFILING

Generated by 🚫 dangerJS

@bvaughn bvaughn requested a review from sebmarkbage Jun 16, 2018
@@ -39,6 +39,9 @@ export const warnAboutLegacyContextAPI = false;
// Gather advanced timing metrics for Profiler subtrees.
export const enableProfilerTimer = __PROFILE__;

// If the DevTools hook is present, inject into it.
export const supportDevToolsIfPresent = false;
Copy link
Member

@gaearon gaearon Jun 16, 2018

Shouldn't this be true? I think this caused devtools code to get DCE'd in open source builds.

@@ -279,6 +281,10 @@ class ReactTestInstance {
return this._currentFiber().memoizedProps;
}

get treeBaseTime(): number {
Copy link
Member

@sebmarkbage sebmarkbage Jun 16, 2018

The Devtools are going to use a private api to access this data in a way that we support only because we can push new versions of it so mitigates maintainance burden.

You can use the private api to test this behavior since the private api is the “internally public” api, the one actually used.

Even if we wanted to expose this in the future I don’t think we are ready to do that yet.

Copy link
Contributor Author

@bvaughn bvaughn Jun 16, 2018

Sure, that makes sense.

'font-weight:bold',
);
if (__DEV__) {
if (!foundDevTools && canUseDOM && window.top === window.self) {
Copy link
Member

@sebmarkbage sebmarkbage Jun 16, 2018

I didn’t realize we limit it to the top most frame. Seems hacky and unfortunate limitation.

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jun 19, 2018

Ping 😄

@@ -21,6 +21,7 @@ export const warnAboutDeprecatedLifecycles = false;
export const warnAboutLegacyContextAPI = false;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const enableProfilerTimer = false;
export const supportDevToolsIfPresent = false;
Copy link
Member

@gaearon gaearon Jun 19, 2018

Why not?

@bvaughn bvaughn force-pushed the Profiler-DevTools-integration-test branch from 111a4f0 to 3e22e59 Jun 19, 2018
@gaearon
Copy link
Member

@gaearon gaearon commented Jun 20, 2018

What is the utility in making supportDevToolsIfPresent conditional? Can we remove the flag, and always inject?

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jun 20, 2018

We definitely could. I don't feel strongly about this. It just seemed a little odd for the OSS react-test-renderer to have DevTools injection logic, when it doesn't really support or interact with DevTools in a meaningful way.

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 20, 2018

Does it hurt to have it though? I imagine one might build some interactive React tutorial that embeds the test renderer. Would be cool if DevTools worked with it.

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jun 20, 2018

No, not really.

As the PR description mentions, I made a few judgement calls here that I don't feel strongly about 😄 This is one of them. I'll back it out.

let mode = isAsync ? AsyncMode | StrictMode : NoContext;

if (enableProfilerTimer && isDevToolsPresent) {
// Always collect profile timings when DevTools are present.
Copy link
Member

@gaearon gaearon Jun 20, 2018

This only affects the profiling bundle, right?

At first I was a bit worried we'd do this in production mode too, but now I realized that we seemingly don't. Then it should be fine.

In general we try to avoid any extra overhead based on DevTools presence (i.e. until we actually connect). But a bit of overhead in profiling bundle specifically (and not in production) seems okay to me.

Copy link
Contributor Author

@bvaughn bvaughn Jun 20, 2018

Yes, it only affects profiling builds.

Copy link
Member

@gaearon gaearon left a comment

LGTM assuming my understanding is correct, and any overhead only affects profiling mode bundles.

I'd probably remove supportDevToolsIfPresent. I don't think it gives us something valuable, and each flag adds some maintenance cost and a chance of getting it wrong (as happened in this PR twice 😛 ).

There is some weird artifact with ReactTestRendererComponentTree.js showing up in diff view with no changes. Not sure what that's about.


// Measure observable timing using the Profiler component.
// The time spent in App (above the Profiler) won't be included in the durations,
// But needs to be acocunted for in the offset times.
Copy link
Member

@gaearon gaearon Jun 20, 2018

Typo

Copy link
Contributor Author

@bvaughn bvaughn Jun 20, 2018

Had to read 3x to spot it


// Measure observable timing using the Profiler component.
// The time spent in App (above the Profiler) won't be included in the durations,
// But needs to be acocunted for in the offset times.
Copy link
Member

@gaearon gaearon Jun 20, 2018

Typo

// At this point, the base time should include both:
// The initial 9ms for the components that do not re-render, and
// The updated 6ms for the component that does.
expect(rendered.root.findByType(App)._currentFiber().treeBaseTime).toBe(15);
Copy link
Member

@gaearon gaearon Jun 20, 2018

Can you explain more about why this is completely unobservable? Is it because it's only used for the UI, and not reported by the Profiler component? Should it be reported by Profiler component somehow?

Copy link
Contributor Author

@bvaughn bvaughn Jun 20, 2018

Maybe "unobservable" isn't the right word to use, but this time value won't be tracked at all unless the DevTools are detected– because App isn't inside of a Profiler root (which is intentional or this test).

Copy link
Member

@gaearon gaearon Jun 20, 2018

My main concern is with using _currentFiber() here which isn't a public API. Is there any way we could write these tests without reaching into the fiber data structure? If not (and if that's how DevTools works anyways) then it's okay.

Copy link
Contributor Author

@bvaughn bvaughn Jun 20, 2018

I don't think so. Also, this is kind of doing what Sebastian suggested (assuming I understood his comment correctly).

Copy link
Member

@gaearon gaearon Jun 20, 2018

DevTools wouldn't be using _currentFiber() but yeah, this seems like the easiest way without bringing the backend itself into this repo. I assume you're reading treeBaseTime from the Fiber in DevTools—can't check because I'm not sure where the code is.

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Jun 20, 2018

Thanks for the review, Dan 😄

@bvaughn bvaughn merged commit b0f6089 into facebook:master Jun 20, 2018
1 check passed
@bvaughn bvaughn deleted the Profiler-DevTools-integration-test branch Jun 20, 2018
bvaughn added a commit to bvaughn/react that referenced this issue Jul 6, 2018
…n/treeBaseDuration

This is an unobservable change to all but the (under development) DevTools Profiler plugin. It is being done so that the plugin can safely feature detect a version of React that supports it. The profiler API has existed since the 16.4.0 release, but it did not support the DevTools plugin prior to PR facebook#13058.

Side note: I am not a big fan of the term "base duration". Both it and "actual duration" are kind of awkward and vague. If anyone has suggestions for better names– this is the best time to bikeshed about them.
bvaughn added a commit that referenced this issue Jul 6, 2018
…n/treeBaseDuration (#13156)

This is an unobservable change to all but the (under development) DevTools Profiler plugin. It is being done so that the plugin can safely feature detect a version of React that supports it. The profiler API has existed since the 16.4.0 release, but it did not support the DevTools plugin prior to PR #13058.

Side note: I am not a big fan of the term "base duration". Both it and "actual duration" are kind of awkward and vague. If anyone has suggestions for better names– this is the best time to bikeshed about them.
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* react-test-renderer injects itself into DevTools if present
* Fibers are always opted into ProfileMode if DevTools is present
* Added simple test for DevTools + always profiling behavior
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
…n/treeBaseDuration (facebook#13156)

This is an unobservable change to all but the (under development) DevTools Profiler plugin. It is being done so that the plugin can safely feature detect a version of React that supports it. The profiler API has existed since the 16.4.0 release, but it did not support the DevTools plugin prior to PR facebook#13058.

Side note: I am not a big fan of the term "base duration". Both it and "actual duration" are kind of awkward and vague. If anyone has suggestions for better names– this is the best time to bikeshed about them.
@gaearon gaearon mentioned this pull request Sep 5, 2018
NMinhNguyen referenced this issue in enzymejs/react-shallow-renderer Jan 29, 2020
* react-test-renderer injects itself into DevTools if present
* Fibers are always opted into ProfileMode if DevTools is present
* Added simple test for DevTools + always profiling behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants