Skip to content

Commit c13986d

Browse files
authored
Fix Overlapping "message" Bug in Performance Track (#31528)
When you schedule a microtask from render or effect and then call setState (or ping) from there, the "event" is the event that React scheduled (which will be a postMessage). The event time of this new render will be before the last render finished. We usually clamp these but in this scenario the update doesn't happen while a render is happening. Causing overlapping events. Before: <img width="1229" alt="Screenshot 2024-11-12 at 11 01 30 PM" src="https://github.com/user-attachments/assets/9652cf3b-b358-453c-b295-1239cbb15952"> Therefore when we finalize a render we need to store the end of the last render so when we a new update comes in later with an event time earlier than that, we know to clamp it. There's also a special case here where when we enter the `RootDidNotComplete` or `RootSuspendedWithDelay` case we neither leave the root as in progress nor commit it. Those needs to finalize too. Really this should be modeled as a suspended track that we haven't added yet. That's the gap between "Blocked" and "message" below. After: <img width="1471" alt="Screenshot 2024-11-13 at 12 31 34 AM" src="https://github.com/user-attachments/assets/b24f994e-9055-4b10-ad29-ad9b36302ffc"> I also fixed an issue where we may log the same event name multiple times if we're rendering more than once in the same event. In this case I just leave a blank trace between the last commit and the next update. I also adding ignoring of the "message" event at all in these cases when the event is from React's scheduling itself.
1 parent 4686872 commit c13986d

File tree

12 files changed

+97
-31
lines changed

12 files changed

+97
-31
lines changed

packages/react-art/src/ReactFiberConfigART.js

+2
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,8 @@ export function resolveUpdatePriority(): EventPriority {
363363
return currentUpdatePriority || DefaultEventPriority;
364364
}
365365

366+
export function trackSchedulerEvent(): void {}
367+
366368
export function resolveEventType(): null | string {
367369
return null;
368370
}

packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js

+7-2
Original file line numberDiff line numberDiff line change
@@ -606,14 +606,19 @@ export function shouldAttemptEagerTransition(): boolean {
606606
return false;
607607
}
608608

609+
let schedulerEvent: void | Event = undefined;
610+
export function trackSchedulerEvent(): void {
611+
schedulerEvent = window.event;
612+
}
613+
609614
export function resolveEventType(): null | string {
610615
const event = window.event;
611-
return event ? event.type : null;
616+
return event && event !== schedulerEvent ? event.type : null;
612617
}
613618

614619
export function resolveEventTimeStamp(): number {
615620
const event = window.event;
616-
return event ? event.timeStamp : -1.1;
621+
return event && event !== schedulerEvent ? event.timeStamp : -1.1;
617622
}
618623

619624
export const isPrimaryRenderer = true;

packages/react-native-renderer/src/ReactFiberConfigFabric.js

+2
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,8 @@ export function resolveUpdatePriority(): EventPriority {
372372
return DefaultEventPriority;
373373
}
374374

375+
export function trackSchedulerEvent(): void {}
376+
375377
export function resolveEventType(): null | string {
376378
return null;
377379
}

packages/react-native-renderer/src/ReactFiberConfigNative.js

+2
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,8 @@ export function resolveUpdatePriority(): EventPriority {
288288
return DefaultEventPriority;
289289
}
290290

291+
export function trackSchedulerEvent(): void {}
292+
291293
export function resolveEventType(): null | string {
292294
return null;
293295
}

packages/react-noop-renderer/src/createReactNoop.js

+2
Original file line numberDiff line numberDiff line change
@@ -531,6 +531,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
531531
return currentEventPriority;
532532
},
533533

534+
trackSchedulerEvent(): void {},
535+
534536
resolveEventType(): null | string {
535537
return null;
536538
},

packages/react-reconciler/src/ReactFiberPerformanceTrack.js

+4-2
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ export function logBlockingStart(
118118
updateTime: number,
119119
eventTime: number,
120120
eventType: null | string,
121+
eventIsRepeat: boolean,
121122
renderStartTime: number,
122123
): void {
123124
if (supportsUserTiming) {
@@ -127,7 +128,7 @@ export function logBlockingStart(
127128
reusableLaneDevToolDetails.color = 'secondary-dark';
128129
reusableLaneOptions.start = eventTime;
129130
reusableLaneOptions.end = updateTime > 0 ? updateTime : renderStartTime;
130-
performance.measure(eventType, reusableLaneOptions);
131+
performance.measure(eventIsRepeat ? '' : eventType, reusableLaneOptions);
131132
}
132133
if (updateTime > 0) {
133134
// Log the time from when we called setState until we started rendering.
@@ -144,6 +145,7 @@ export function logTransitionStart(
144145
updateTime: number,
145146
eventTime: number,
146147
eventType: null | string,
148+
eventIsRepeat: boolean,
147149
renderStartTime: number,
148150
): void {
149151
if (supportsUserTiming) {
@@ -158,7 +160,7 @@ export function logTransitionStart(
158160
: updateTime > 0
159161
? updateTime
160162
: renderStartTime;
161-
performance.measure(eventType, reusableLaneOptions);
163+
performance.measure(eventIsRepeat ? '' : eventType, reusableLaneOptions);
162164
}
163165
if (startTime > 0) {
164166
// Log the time from when we started an async transition until we called setState or started rendering.

packages/react-reconciler/src/ReactFiberRootScheduler.js

+14
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
disableSchedulerTimeoutInWorkLoop,
1919
enableProfilerTimer,
2020
enableProfilerNestedUpdatePhase,
21+
enableComponentPerformanceTrack,
2122
enableSiblingPrerendering,
2223
} from 'shared/ReactFeatureFlags';
2324
import {
@@ -64,6 +65,7 @@ import {
6465
supportsMicrotasks,
6566
scheduleMicrotask,
6667
shouldAttemptEagerTransition,
68+
trackSchedulerEvent,
6769
} from './ReactFiberConfig';
6870

6971
import ReactSharedInternals from 'shared/ReactSharedInternals';
@@ -225,6 +227,12 @@ function flushSyncWorkAcrossRoots_impl(
225227
}
226228

227229
function processRootScheduleInMicrotask() {
230+
if (enableProfilerTimer && enableComponentPerformanceTrack) {
231+
// Track the currently executing event if there is one so we can ignore this
232+
// event when logging events.
233+
trackSchedulerEvent();
234+
}
235+
228236
// This function is always called inside a microtask. It should never be
229237
// called synchronously.
230238
didScheduleMicrotask = false;
@@ -428,6 +436,12 @@ function performWorkOnRootViaSchedulerTask(
428436
resetNestedUpdateFlag();
429437
}
430438

439+
if (enableProfilerTimer && enableComponentPerformanceTrack) {
440+
// Track the currently executing event if there is one so we can ignore this
441+
// event when logging events.
442+
trackSchedulerEvent();
443+
}
444+
431445
// Flush any pending passive effects before deciding which lanes to work on,
432446
// in case they schedule additional work.
433447
const originalCallbackNode = root.callbackNode;

packages/react-reconciler/src/ReactFiberWorkLoop.js

+33-5
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ import {
9090
setCurrentUpdatePriority,
9191
getCurrentUpdatePriority,
9292
resolveUpdatePriority,
93+
trackSchedulerEvent,
9394
} from './ReactFiberConfig';
9495

9596
import {createWorkInProgress, resetWorkInProgress} from './ReactFiber';
@@ -229,13 +230,17 @@ import {
229230
} from './ReactFiberConcurrentUpdates';
230231

231232
import {
233+
blockingClampTime,
232234
blockingUpdateTime,
233235
blockingEventTime,
234236
blockingEventType,
237+
blockingEventIsRepeat,
238+
transitionClampTime,
235239
transitionStartTime,
236240
transitionUpdateTime,
237241
transitionEventTime,
238242
transitionEventType,
243+
transitionEventIsRepeat,
239244
clearBlockingTimers,
240245
clearTransitionTimers,
241246
clampBlockingTimers,
@@ -938,6 +943,9 @@ export function performWorkOnRoot(
938943
}
939944
break;
940945
} else if (exitStatus === RootDidNotComplete) {
946+
if (enableProfilerTimer && enableComponentPerformanceTrack) {
947+
finalizeRender(lanes, now());
948+
}
941949
// The render unwound without completing the tree. This happens in special
942950
// cases where need to exit the current render without producing a
943951
// consistent tree or committing.
@@ -1130,6 +1138,9 @@ function finishConcurrentRender(
11301138
// This is a transition, so we should exit without committing a
11311139
// placeholder and without scheduling a timeout. Delay indefinitely
11321140
// until we receive more data.
1141+
if (enableProfilerTimer && enableComponentPerformanceTrack) {
1142+
finalizeRender(lanes, now());
1143+
}
11331144
const didAttemptEntireTree =
11341145
!workInProgressRootDidSkipSuspendedSiblings;
11351146
markRootSuspended(
@@ -1655,19 +1666,31 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {
16551666

16561667
if (includesSyncLane(lanes) || includesBlockingLane(lanes)) {
16571668
logBlockingStart(
1658-
blockingUpdateTime,
1659-
blockingEventTime,
1669+
blockingUpdateTime >= 0 && blockingUpdateTime < blockingClampTime
1670+
? blockingClampTime
1671+
: blockingUpdateTime,
1672+
blockingEventTime >= 0 && blockingEventTime < blockingClampTime
1673+
? blockingClampTime
1674+
: blockingEventTime,
16601675
blockingEventType,
1676+
blockingEventIsRepeat,
16611677
renderStartTime,
16621678
);
16631679
clearBlockingTimers();
16641680
}
16651681
if (includesTransitionLane(lanes)) {
16661682
logTransitionStart(
1667-
transitionStartTime,
1668-
transitionUpdateTime,
1669-
transitionEventTime,
1683+
transitionStartTime >= 0 && transitionStartTime < transitionClampTime
1684+
? transitionClampTime
1685+
: transitionStartTime,
1686+
transitionUpdateTime >= 0 && transitionUpdateTime < transitionClampTime
1687+
? transitionClampTime
1688+
: transitionUpdateTime,
1689+
transitionEventTime >= 0 && transitionEventTime < transitionClampTime
1690+
? transitionClampTime
1691+
: transitionEventTime,
16701692
transitionEventType,
1693+
transitionEventIsRepeat,
16711694
renderStartTime,
16721695
);
16731696
clearTransitionTimers();
@@ -3139,6 +3162,11 @@ function commitRootImpl(
31393162
// with setTimeout
31403163
pendingPassiveTransitions = transitions;
31413164
scheduleCallback(NormalSchedulerPriority, () => {
3165+
if (enableProfilerTimer && enableComponentPerformanceTrack) {
3166+
// Track the currently executing event if there is one so we can ignore this
3167+
// event when logging events.
3168+
trackSchedulerEvent();
3169+
}
31423170
flushPassiveEffects(true);
31433171
// This render triggered passive effects: release the root cache pool
31443172
// *after* passive effects fire to avoid freeing a cache pool that may

packages/react-reconciler/src/ReactProfilerTimer.js

+27-21
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,18 @@ export let componentEffectDuration: number = -0;
3636
export let componentEffectStartTime: number = -1.1;
3737
export let componentEffectEndTime: number = -1.1;
3838

39+
export let blockingClampTime: number = -0;
3940
export let blockingUpdateTime: number = -1.1; // First sync setState scheduled.
4041
export let blockingEventTime: number = -1.1; // Event timeStamp of the first setState.
4142
export let blockingEventType: null | string = null; // Event type of the first setState.
43+
export let blockingEventIsRepeat: boolean = false;
4244
// TODO: This should really be one per Transition lane.
45+
export let transitionClampTime: number = -0;
4346
export let transitionStartTime: number = -1.1; // First startTransition call before setState.
4447
export let transitionUpdateTime: number = -1.1; // First transition setState scheduled.
4548
export let transitionEventTime: number = -1.1; // Event timeStamp of the first transition.
4649
export let transitionEventType: null | string = null; // Event type of the first transition.
50+
export let transitionEventIsRepeat: boolean = false;
4751

4852
export function startUpdateTimerByLane(lane: Lane): void {
4953
if (!enableProfilerTimer || !enableComponentPerformanceTrack) {
@@ -52,15 +56,25 @@ export function startUpdateTimerByLane(lane: Lane): void {
5256
if (isSyncLane(lane) || isBlockingLane(lane)) {
5357
if (blockingUpdateTime < 0) {
5458
blockingUpdateTime = now();
55-
blockingEventTime = resolveEventTimeStamp();
56-
blockingEventType = resolveEventType();
59+
const newEventTime = resolveEventTimeStamp();
60+
const newEventType = resolveEventType();
61+
blockingEventIsRepeat =
62+
newEventTime === blockingEventTime &&
63+
newEventType === blockingEventType;
64+
blockingEventTime = newEventTime;
65+
blockingEventType = newEventType;
5766
}
5867
} else if (isTransitionLane(lane)) {
5968
if (transitionUpdateTime < 0) {
6069
transitionUpdateTime = now();
6170
if (transitionStartTime < 0) {
62-
transitionEventTime = resolveEventTimeStamp();
63-
transitionEventType = resolveEventType();
71+
const newEventTime = resolveEventTimeStamp();
72+
const newEventType = resolveEventType();
73+
transitionEventIsRepeat =
74+
newEventTime === transitionEventTime &&
75+
newEventType === transitionEventType;
76+
transitionEventTime = newEventTime;
77+
transitionEventType = newEventType;
6478
}
6579
}
6680
}
@@ -76,8 +90,13 @@ export function startAsyncTransitionTimer(): void {
7690
}
7791
if (transitionStartTime < 0 && transitionUpdateTime < 0) {
7892
transitionStartTime = now();
79-
transitionEventTime = resolveEventTimeStamp();
80-
transitionEventType = resolveEventType();
93+
const newEventTime = resolveEventTimeStamp();
94+
const newEventType = resolveEventType();
95+
transitionEventIsRepeat =
96+
newEventTime === transitionEventTime &&
97+
newEventType === transitionEventType;
98+
transitionEventTime = newEventTime;
99+
transitionEventType = newEventType;
81100
}
82101
}
83102

@@ -115,12 +134,7 @@ export function clampBlockingTimers(finalTime: number): void {
115134
// If we had new updates come in while we were still rendering or committing, we don't want
116135
// those update times to create overlapping tracks in the performance timeline so we clamp
117136
// them to the end of the commit phase.
118-
if (blockingUpdateTime >= 0 && blockingUpdateTime < finalTime) {
119-
blockingUpdateTime = finalTime;
120-
}
121-
if (blockingEventTime >= 0 && blockingEventTime < finalTime) {
122-
blockingEventTime = finalTime;
123-
}
137+
blockingClampTime = finalTime;
124138
}
125139

126140
export function clampTransitionTimers(finalTime: number): void {
@@ -130,15 +144,7 @@ export function clampTransitionTimers(finalTime: number): void {
130144
// If we had new updates come in while we were still rendering or committing, we don't want
131145
// those update times to create overlapping tracks in the performance timeline so we clamp
132146
// them to the end of the commit phase.
133-
if (transitionStartTime >= 0 && transitionStartTime < finalTime) {
134-
transitionStartTime = finalTime;
135-
}
136-
if (transitionUpdateTime >= 0 && transitionUpdateTime < finalTime) {
137-
transitionUpdateTime = finalTime;
138-
}
139-
if (transitionEventTime >= 0 && transitionEventTime < finalTime) {
140-
transitionEventTime = finalTime;
141-
}
147+
transitionClampTime = finalTime;
142148
}
143149

144150
export function pushNestedEffectDurations(): number {

packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js

+1
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ describe('ReactFiberHostContext', () => {
8383
}
8484
return DefaultEventPriority;
8585
},
86+
trackSchedulerEvent: function () {},
8687
resolveEventType: function () {
8788
return null;
8889
},

packages/react-reconciler/src/forks/ReactFiberConfig.custom.js

+1
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ export const getInstanceFromScope = $$$config.getInstanceFromScope;
7373
export const setCurrentUpdatePriority = $$$config.setCurrentUpdatePriority;
7474
export const getCurrentUpdatePriority = $$$config.getCurrentUpdatePriority;
7575
export const resolveUpdatePriority = $$$config.resolveUpdatePriority;
76+
export const trackSchedulerEvent = $$$config.trackSchedulerEvent;
7677
export const resolveEventType = $$$config.resolveEventType;
7778
export const resolveEventTimeStamp = $$$config.resolveEventTimeStamp;
7879
export const shouldAttemptEagerTransition =

packages/react-test-renderer/src/ReactFiberConfigTestHost.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -224,10 +224,11 @@ export function resolveUpdatePriority(): EventPriority {
224224
}
225225
return DefaultEventPriority;
226226
}
227+
228+
export function trackSchedulerEvent(): void {}
227229
export function resolveEventType(): null | string {
228230
return null;
229231
}
230-
231232
export function resolveEventTimeStamp(): number {
232233
return -1.1;
233234
}

0 commit comments

Comments
 (0)