Skip to content

Commit

Permalink
Don't show empty (no work) commits in Profiler (#17253)
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Vaughn authored Nov 1, 2019
1 parent a2e05b6 commit 36fd29f
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 2 deletions.
40 changes: 40 additions & 0 deletions packages/react-devtools-shared/src/__tests__/profilerStore-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,44 @@ describe('ProfilerStore', () => {
store.profilerStore.profilingData = fauxProfilingData;
expect(store.profilerStore.profilingData).toBe(fauxProfilingData);
});

// This test covers current broken behavior (arguably) with the synthetic event system.
it('should filter empty commits', () => {
const inputRef = React.createRef();
const ControlledInput = () => {
const [name, setName] = React.useState('foo');
const handleChange = event => setName(event.target.value);
return <input ref={inputRef} value={name} onChange={handleChange} />;
};

const container = document.createElement('div');

// This element has to be in the <body> for the event system to work.
document.body.appendChild(container);

// It's important that this test uses legacy sync mode.
// The root API does not trigger this particular failing case.
ReactDOM.render(<ControlledInput />, container);

utils.act(() => store.profilerStore.startProfiling());

// Sets a value in a way that React doesn't see,
// so that a subsequent "change" event will trigger the event handler.
const setUntrackedValue = Object.getOwnPropertyDescriptor(
HTMLInputElement.prototype,
'value',
).set;

const target = inputRef.current;
setUntrackedValue.call(target, 'bar');
target.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
expect(target.value).toBe('bar');

utils.act(() => store.profilerStore.stopProfiling());

// Only one commit should have been recorded (in response to the "change" event).
const root = store.roots[0];
const data = store.profilerStore.getDataForRoot(root);
expect(data.commitData).toHaveLength(1);
});
});
13 changes: 11 additions & 2 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1643,6 +1643,7 @@ export function attach(
}
}
}

if (shouldIncludeInTree) {
const isProfilingSupported = nextFiber.hasOwnProperty('treeBaseDuration');
if (isProfilingSupported) {
Expand Down Expand Up @@ -1742,6 +1743,14 @@ export function attach(
const current = root.current;
const alternate = current.alternate;

// Certain types of updates bail out at the root without doing any actual render work.
// React should probably not call the DevTools commit hook in this case,
// but if it does- we can detect it and filter them out from the profiler.
const didBailoutAtRoot =
alternate !== null &&
alternate.expirationTime === 0 &&
alternate.childExpirationTime === 0;

currentRootID = getFiberID(getPrimaryFiber(current));

// Before the traversals, remember to start tracking
Expand All @@ -1758,7 +1767,7 @@ export function attach(
// where some v16 renderers support profiling and others don't.
const isProfilingSupported = root.memoizedInteractions != null;

if (isProfiling && isProfilingSupported) {
if (isProfiling && isProfilingSupported && !didBailoutAtRoot) {
// If profiling is active, store commit time and duration, and the current interactions.
// The frontend may request this information after profiling has stopped.
currentCommitProfilingMetadata = {
Expand Down Expand Up @@ -1802,7 +1811,7 @@ export function attach(
mountFiberRecursively(current, null, false, false);
}

if (isProfiling && isProfilingSupported) {
if (isProfiling && isProfilingSupported && !didBailoutAtRoot) {
const commitProfilingMetadata = ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get(
currentRootID,
);
Expand Down
3 changes: 3 additions & 0 deletions packages/react-devtools-shared/src/backend/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,9 @@ export type DevToolsHook = {
onCommitFiberRoot: (
rendererID: RendererID,
fiber: Object,
// Added in v16.9 to support Profiler priority labels
commitPriority?: number,
// Added in v16.9 to support Fast Refresh
didError?: boolean,
) => void,
};

0 comments on commit 36fd29f

Please sign in to comment.