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
11 changes: 10 additions & 1 deletion packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
} from 'shared/ReactTypeOfWork';
import getComponentName from 'shared/getComponentName';

import {isDevToolsPresent} from './ReactFiberDevToolsHook';
import {NoWork} from './ReactFiberExpirationTime';
import {NoContext, AsyncMode, ProfileMode, StrictMode} from './ReactTypeOfMode';
import {
Expand Down Expand Up @@ -345,7 +346,15 @@ export function createWorkInProgress(
}

export function createHostRootFiber(isAsync: boolean): Fiber {
const mode = isAsync ? AsyncMode | StrictMode : NoContext;
let mode = isAsync ? AsyncMode | StrictMode : NoContext;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it only affects profiling builds.

// This enables DevTools to start capturing timing at any point–
// Without some nodes in the tree having empty base times.
mode |= ProfileMode;
}

return createFiber(HostRoot, null, null, mode);
}

Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberDevToolsHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ function catchErrors(fn) {
};
}

export const isDevToolsPresent =
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ !== 'undefined';

export function injectInternals(internals: Object): boolean {
if (typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ === 'undefined') {
// No DevTools
Expand Down
11 changes: 11 additions & 0 deletions packages/react-test-renderer/src/ReactTestRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
Profiler,
} from 'shared/ReactTypeOfWork';
import invariant from 'shared/invariant';
import ReactVersion from 'shared/ReactVersion';

import * as ReactTestHostConfig from './ReactTestHostConfig';
import * as TestRendererScheduling from './ReactTestRendererScheduling';
Expand Down Expand Up @@ -510,4 +511,14 @@ const ReactTestRendererFiber = {
unstable_setNowImplementation: TestRendererScheduling.setNowImplementation,
};

// Enable ReactTestRenderer to be used to test DevTools integration.
TestRenderer.injectIntoDevTools({
findFiberByHostInstance: (() => {
throw new Error('TestRenderer does not support findFiberByHostInstance()');
}: any),
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
rendererPackageName: 'react-test-renderer',
});

export default ReactTestRendererFiber;
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

describe('ReactProfiler DevTools integration', () => {
let React;
let ReactFeatureFlags;
let ReactTestRenderer;
let AdvanceTime;
let advanceTimeBy;
let hook;
let mockNow;

const mockNowForTests = () => {
let currentTime = 0;

mockNow = jest.fn().mockImplementation(() => currentTime);

ReactTestRenderer.unstable_setNowImplementation(mockNow);
advanceTimeBy = amount => {
currentTime += amount;
};
};

beforeEach(() => {
global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook = {
inject: () => {},
onCommitFiberRoot: jest.fn((rendererId, root) => {}),
onCommitFiberUnmount: () => {},
supportsFiber: true,
};

jest.resetModules();

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableProfilerTimer = true;
React = require('react');
ReactTestRenderer = require('react-test-renderer');

mockNowForTests();

AdvanceTime = class extends React.Component {
static defaultProps = {
byAmount: 10,
shouldComponentUpdate: true,
};
shouldComponentUpdate(nextProps) {
return nextProps.shouldComponentUpdate;
}
render() {
// Simulate time passing when this component is rendered
advanceTimeBy(this.props.byAmount);
return this.props.children || null;
}
};
});

it('should auto-Profile all fibers if the DevTools hook is detected', () => {
const App = ({multiplier}) => {
advanceTimeBy(2);
return (
<React.unstable_Profiler id="Profiler" onRender={onRender}>
<AdvanceTime byAmount={3 * multiplier} shouldComponentUpdate={true} />
<AdvanceTime
byAmount={7 * multiplier}
shouldComponentUpdate={false}
/>
</React.unstable_Profiler>
);
};

const onRender = jest.fn(() => {});
const rendered = ReactTestRenderer.create(<App multiplier={1} />);

expect(hook.onCommitFiberRoot).toHaveBeenCalledTimes(1);

// 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 accounted for in the offset times.
expect(onRender).toHaveBeenCalledTimes(1);
expect(onRender).toHaveBeenCalledWith('Profiler', 'mount', 10, 10, 2, 12);
onRender.mockClear();

// Measure unobservable timing required by the DevTools profiler.
// At this point, the base time should include both:
// The time 2ms in the App component itself, and
// The 10ms spend in the Profiler sub-tree beneath.
expect(rendered.root.findByType(App)._currentFiber().treeBaseTime).toBe(12);

rendered.update(<App multiplier={2} />);

// 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 accounted for in the offset times.
expect(onRender).toHaveBeenCalledTimes(1);
expect(onRender).toHaveBeenCalledWith('Profiler', 'update', 6, 13, 14, 20);

// Measure unobservable timing required by the DevTools profiler.
// 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
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
});