Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 22f7663

Browse filesBrowse files
author
Brian Vaughn
authored
Profiler: Don't count timed out (hidden) subtrees in base duration (#18966)
1 parent 64f50c6 commit 22f7663
Copy full SHA for 22f7663

File tree

Expand file treeCollapse file tree

5 files changed

+120
-30
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+120
-30
lines changed

‎packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Copy file name to clipboardExpand all lines: packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+13-1Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1700,7 +1700,7 @@ function resetChildLanes(completedWork: Fiber) {
17001700
// In profiling mode, resetChildExpirationTime is also used to reset
17011701
// profiler durations.
17021702
let actualDuration = completedWork.actualDuration;
1703-
let treeBaseDuration = completedWork.selfBaseDuration;
1703+
let treeBaseDuration = ((completedWork.selfBaseDuration: any): number);
17041704

17051705
// When a fiber is cloned, its actualDuration is reset to 0. This value will
17061706
// only be updated if work is done on the fiber (i.e. it doesn't bailout).
@@ -1725,6 +1725,18 @@ function resetChildLanes(completedWork: Fiber) {
17251725
treeBaseDuration += child.treeBaseDuration;
17261726
child = child.sibling;
17271727
}
1728+
1729+
const isTimedOutSuspense =
1730+
completedWork.tag === SuspenseComponent &&
1731+
completedWork.memoizedState !== null;
1732+
if (isTimedOutSuspense) {
1733+
// Don't count time spent in a timed out Suspense subtree as part of the base duration.
1734+
const primaryChildFragment = completedWork.child;
1735+
if (primaryChildFragment !== null) {
1736+
treeBaseDuration -= ((primaryChildFragment.treeBaseDuration: any): number);
1737+
}
1738+
}
1739+
17281740
completedWork.actualDuration = actualDuration;
17291741
completedWork.treeBaseDuration = treeBaseDuration;
17301742
} else {

‎packages/react-reconciler/src/ReactFiberWorkLoop.old.js

Copy file name to clipboardExpand all lines: packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+13-1Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1775,7 +1775,7 @@ function resetChildExpirationTime(completedWork: Fiber) {
17751775
// In profiling mode, resetChildExpirationTime is also used to reset
17761776
// profiler durations.
17771777
let actualDuration = completedWork.actualDuration;
1778-
let treeBaseDuration = completedWork.selfBaseDuration;
1778+
let treeBaseDuration = ((completedWork.selfBaseDuration: any): number);
17791779

17801780
// When a fiber is cloned, its actualDuration is reset to 0. This value will
17811781
// only be updated if work is done on the fiber (i.e. it doesn't bailout).
@@ -1804,6 +1804,18 @@ function resetChildExpirationTime(completedWork: Fiber) {
18041804
treeBaseDuration += child.treeBaseDuration;
18051805
child = child.sibling;
18061806
}
1807+
1808+
const isTimedOutSuspense =
1809+
completedWork.tag === SuspenseComponent &&
1810+
completedWork.memoizedState !== null;
1811+
if (isTimedOutSuspense) {
1812+
// Don't count time spent in a timed out Suspense subtree as part of the base duration.
1813+
const primaryChildFragment = completedWork.child;
1814+
if (primaryChildFragment !== null) {
1815+
treeBaseDuration -= ((primaryChildFragment.treeBaseDuration: any): number);
1816+
}
1817+
}
1818+
18071819
completedWork.actualDuration = actualDuration;
18081820
completedWork.treeBaseDuration = treeBaseDuration;
18091821
} else {

‎packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js

Copy file name to clipboardExpand all lines: packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js
+1-7Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2709,13 +2709,7 @@ describe('ReactSuspenseList', () => {
27092709
// actualDuration
27102710
expect(onRender.mock.calls[2][2]).toBe((1 + 4 + 5 + 3) * 2 + 3);
27112711
// treeBaseDuration
2712-
expect(onRender.mock.calls[2][3]).toBe(
2713-
1 +
2714-
4 +
2715-
3 +
2716-
3 +
2717-
/* Resuspending a boundary also includes the content in base duration but it shouldn't */ 5,
2718-
);
2712+
expect(onRender.mock.calls[2][3]).toBe(1 + 4 + 3 + 3);
27192713

27202714
await C.resolve();
27212715

‎packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js

Copy file name to clipboardExpand all lines: packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js
+7-21Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -405,19 +405,9 @@ describe('ReactSuspensePlaceholder', () => {
405405
// The suspense update should only show the "Loading..." Fallback.
406406
// The actual duration should include 10ms spent rendering Fallback,
407407
// plus the 8ms render all of the hidden, suspended subtree.
408-
// Note from Andrew to Brian: I don't fully understand why this one
409-
// diverges, but I checked and it matches the times we get when
410-
// we run this same test in Concurrent Mode.
411-
if (gate(flags => flags.new)) {
412-
// But the tree base duration should only include 10ms spent rendering Fallback,
413-
// plus the 5ms rendering the previously committed version of the hidden tree.
414-
expect(onRender.mock.calls[1][2]).toBe(18);
415-
expect(onRender.mock.calls[1][3]).toBe(15);
416-
} else {
417-
// Old behavior includes the time spent on the primary tree.
418-
expect(onRender.mock.calls[1][2]).toBe(18);
419-
expect(onRender.mock.calls[1][3]).toBe(18);
420-
}
408+
// But the tree base duration should only include 10ms spent rendering Fallback,
409+
expect(onRender.mock.calls[1][2]).toBe(18);
410+
expect(onRender.mock.calls[1][3]).toBe(10);
421411

422412
ReactNoop.renderLegacySyncRoot(
423413
<App shouldSuspend={true} text="New" textRenderDuration={6} />,
@@ -432,18 +422,15 @@ describe('ReactSuspensePlaceholder', () => {
432422
expect(ReactNoop).toMatchRenderedOutput('Loading...');
433423
expect(onRender).toHaveBeenCalledTimes(3);
434424

435-
// Note from Andrew to Brian: I don't fully understand why this one
436-
// diverges, but I checked and it matches the times we get when
437-
// we run this same test in Concurrent Mode.
438425
if (gate(flags => flags.new)) {
439426
expect(onRender.mock.calls[1][2]).toBe(18);
440-
expect(onRender.mock.calls[1][3]).toBe(15);
427+
expect(onRender.mock.calls[1][3]).toBe(10);
441428
} else {
442429
// If we force another update while still timed out,
443430
// but this time the Text component took 1ms longer to render.
444431
// This should impact both actualDuration and treeBaseDuration.
445432
expect(onRender.mock.calls[2][2]).toBe(19);
446-
expect(onRender.mock.calls[2][3]).toBe(19);
433+
expect(onRender.mock.calls[2][3]).toBe(10);
447434
}
448435

449436
jest.advanceTimersByTime(1000);
@@ -500,10 +487,9 @@ describe('ReactSuspensePlaceholder', () => {
500487
// The suspense update should only show the "Loading..." Fallback.
501488
// The actual duration should include 10ms spent rendering Fallback,
502489
// plus the 8ms render all of the hidden, suspended subtree.
503-
// But the tree base duration should only include 10ms spent rendering Fallback,
504-
// plus the 5ms rendering the previously committed version of the hidden tree.
490+
// But the tree base duration should only include 10ms spent rendering Fallback.
505491
expect(onRender.mock.calls[1][2]).toBe(18);
506-
expect(onRender.mock.calls[1][3]).toBe(15);
492+
expect(onRender.mock.calls[1][3]).toBe(10);
507493

508494
// Update again while timed out.
509495
// Since this test was originally written we added an optimization to avoid

‎packages/react/src/__tests__/ReactProfiler-test.internal.js

Copy file name to clipboardExpand all lines: packages/react/src/__tests__/ReactProfiler-test.internal.js
+86Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4145,6 +4145,92 @@ describe('Profiler', () => {
41454145
expect(call[0]).toEqual('test-profiler');
41464146
expect(call[4]).toMatchInteractions([]);
41474147
});
4148+
4149+
it('should properly report base duration wrt suspended subtrees', async () => {
4150+
loadModulesForTracing({useNoopRenderer: true});
4151+
4152+
const onRender = jest.fn();
4153+
4154+
let resolve = null;
4155+
const promise = new Promise(_resolve => {
4156+
resolve = _resolve;
4157+
});
4158+
4159+
function Other() {
4160+
Scheduler.unstable_advanceTime(1);
4161+
Scheduler.unstable_yieldValue('Other');
4162+
return <div>Other</div>;
4163+
}
4164+
4165+
function Fallback() {
4166+
Scheduler.unstable_advanceTime(8);
4167+
Scheduler.unstable_yieldValue('Fallback');
4168+
return <div>Fallback</div>;
4169+
}
4170+
4171+
let shouldSuspend = false;
4172+
function Suspender() {
4173+
Scheduler.unstable_advanceTime(15);
4174+
if (shouldSuspend) {
4175+
Scheduler.unstable_yieldValue('Suspender!');
4176+
throw promise;
4177+
}
4178+
Scheduler.unstable_yieldValue('Suspender');
4179+
return <div>Suspender</div>;
4180+
}
4181+
4182+
function App() {
4183+
return (
4184+
<React.Profiler id="root" onRender={onRender}>
4185+
<Other />
4186+
<React.Suspense fallback={<Fallback />}>
4187+
<Suspender />
4188+
</React.Suspense>
4189+
</React.Profiler>
4190+
);
4191+
}
4192+
4193+
ReactNoop.render(<App />);
4194+
expect(Scheduler).toFlushAndYield(['Other', 'Suspender']);
4195+
expect(ReactNoop).toMatchRenderedOutput(
4196+
<>
4197+
<div>Other</div>
4198+
<div>Suspender</div>
4199+
</>,
4200+
);
4201+
expect(onRender).toHaveBeenCalledTimes(1);
4202+
expect(onRender.mock.calls[0][2]).toBe(1 + 15); // actual
4203+
expect(onRender.mock.calls[0][3]).toBe(1 + 15); // base
4204+
4205+
shouldSuspend = true;
4206+
ReactNoop.render(<App />);
4207+
expect(Scheduler).toFlushAndYield(['Other', 'Suspender!', 'Fallback']);
4208+
await awaitableAdvanceTimers(20000);
4209+
expect(ReactNoop).toMatchRenderedOutput(
4210+
<>
4211+
<div>Other</div>
4212+
<div hidden={true}>Suspender</div>
4213+
<div>Fallback</div>
4214+
</>,
4215+
);
4216+
expect(onRender).toHaveBeenCalledTimes(2);
4217+
expect(onRender.mock.calls[1][2]).toBe(1 + 15 + 8); // actual
4218+
expect(onRender.mock.calls[1][3]).toBe(1 + 8); // base
4219+
4220+
shouldSuspend = false;
4221+
resolve();
4222+
await promise;
4223+
expect(Scheduler).toFlushAndYield(['Suspender']);
4224+
expect(ReactNoop).toMatchRenderedOutput(
4225+
<>
4226+
<div>Other</div>
4227+
<div>Suspender</div>
4228+
</>,
4229+
);
4230+
expect(onRender).toHaveBeenCalledTimes(3);
4231+
expect(onRender.mock.calls[2][2]).toBe(15); // actual
4232+
expect(onRender.mock.calls[2][3]).toBe(1 + 15); // base
4233+
});
41484234
});
41494235
});
41504236
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.