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

Fix Overlapping "message" Bug in Performance Track #31528

Merged
merged 4 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/react-art/src/ReactFiberConfigART.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,8 @@ export function resolveUpdatePriority(): EventPriority {
return currentUpdatePriority || DefaultEventPriority;
}

export function trackSchedulerEvent(): void {}

export function resolveEventType(): null | string {
return null;
}
Expand Down
9 changes: 7 additions & 2 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -606,14 +606,19 @@ export function shouldAttemptEagerTransition(): boolean {
return false;
}

let schedulerEvent: void | Event = undefined;
export function trackSchedulerEvent(): void {
schedulerEvent = window.event;
}

export function resolveEventType(): null | string {
const event = window.event;
return event ? event.type : null;
return event && event !== schedulerEvent ? event.type : null;
}

export function resolveEventTimeStamp(): number {
const event = window.event;
return event ? event.timeStamp : -1.1;
return event && event !== schedulerEvent ? event.timeStamp : -1.1;
}

export const isPrimaryRenderer = true;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-native-renderer/src/ReactFiberConfigFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,8 @@ export function resolveUpdatePriority(): EventPriority {
return DefaultEventPriority;
}

export function trackSchedulerEvent(): void {}

export function resolveEventType(): null | string {
return null;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/react-native-renderer/src/ReactFiberConfigNative.js
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,8 @@ export function resolveUpdatePriority(): EventPriority {
return DefaultEventPriority;
}

export function trackSchedulerEvent(): void {}

export function resolveEventType(): null | string {
return null;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
return currentEventPriority;
},

trackSchedulerEvent(): void {},

resolveEventType(): null | string {
return null;
},
Expand Down
6 changes: 4 additions & 2 deletions packages/react-reconciler/src/ReactFiberPerformanceTrack.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ export function logBlockingStart(
updateTime: number,
eventTime: number,
eventType: null | string,
eventIsRepeat: boolean,
renderStartTime: number,
): void {
if (supportsUserTiming) {
Expand All @@ -127,7 +128,7 @@ export function logBlockingStart(
reusableLaneDevToolDetails.color = 'secondary-dark';
reusableLaneOptions.start = eventTime;
reusableLaneOptions.end = updateTime > 0 ? updateTime : renderStartTime;
performance.measure(eventType, reusableLaneOptions);
performance.measure(eventIsRepeat ? '' : eventType, reusableLaneOptions);
}
if (updateTime > 0) {
// Log the time from when we called setState until we started rendering.
Expand All @@ -144,6 +145,7 @@ export function logTransitionStart(
updateTime: number,
eventTime: number,
eventType: null | string,
eventIsRepeat: boolean,
renderStartTime: number,
): void {
if (supportsUserTiming) {
Expand All @@ -158,7 +160,7 @@ export function logTransitionStart(
: updateTime > 0
? updateTime
: renderStartTime;
performance.measure(eventType, reusableLaneOptions);
performance.measure(eventIsRepeat ? '' : eventType, reusableLaneOptions);
}
if (startTime > 0) {
// Log the time from when we started an async transition until we called setState or started rendering.
Expand Down
14 changes: 14 additions & 0 deletions packages/react-reconciler/src/ReactFiberRootScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
disableSchedulerTimeoutInWorkLoop,
enableProfilerTimer,
enableProfilerNestedUpdatePhase,
enableComponentPerformanceTrack,
enableSiblingPrerendering,
} from 'shared/ReactFeatureFlags';
import {
Expand Down Expand Up @@ -64,6 +65,7 @@ import {
supportsMicrotasks,
scheduleMicrotask,
shouldAttemptEagerTransition,
trackSchedulerEvent,
} from './ReactFiberConfig';

import ReactSharedInternals from 'shared/ReactSharedInternals';
Expand Down Expand Up @@ -225,6 +227,12 @@ function flushSyncWorkAcrossRoots_impl(
}

function processRootScheduleInMicrotask() {
if (enableProfilerTimer && enableComponentPerformanceTrack) {
// Track the currently executing event if there is one so we can ignore this
// event when logging events.
trackSchedulerEvent();
}

// This function is always called inside a microtask. It should never be
// called synchronously.
didScheduleMicrotask = false;
Expand Down Expand Up @@ -428,6 +436,12 @@ function performWorkOnRootViaSchedulerTask(
resetNestedUpdateFlag();
}

if (enableProfilerTimer && enableComponentPerformanceTrack) {
// Track the currently executing event if there is one so we can ignore this
// event when logging events.
trackSchedulerEvent();
}

// Flush any pending passive effects before deciding which lanes to work on,
// in case they schedule additional work.
const originalCallbackNode = root.callbackNode;
Expand Down
38 changes: 33 additions & 5 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ import {
setCurrentUpdatePriority,
getCurrentUpdatePriority,
resolveUpdatePriority,
trackSchedulerEvent,
} from './ReactFiberConfig';

import {createWorkInProgress, resetWorkInProgress} from './ReactFiber';
Expand Down Expand Up @@ -229,13 +230,17 @@ import {
} from './ReactFiberConcurrentUpdates';

import {
blockingClampTime,
blockingUpdateTime,
blockingEventTime,
blockingEventType,
blockingEventIsRepeat,
transitionClampTime,
transitionStartTime,
transitionUpdateTime,
transitionEventTime,
transitionEventType,
transitionEventIsRepeat,
clearBlockingTimers,
clearTransitionTimers,
clampBlockingTimers,
Expand Down Expand Up @@ -938,6 +943,9 @@ export function performWorkOnRoot(
}
break;
} else if (exitStatus === RootDidNotComplete) {
if (enableProfilerTimer && enableComponentPerformanceTrack) {
finalizeRender(lanes, now());
}
// The render unwound without completing the tree. This happens in special
// cases where need to exit the current render without producing a
// consistent tree or committing.
Expand Down Expand Up @@ -1130,6 +1138,9 @@ function finishConcurrentRender(
// This is a transition, so we should exit without committing a
// placeholder and without scheduling a timeout. Delay indefinitely
// until we receive more data.
if (enableProfilerTimer && enableComponentPerformanceTrack) {
finalizeRender(lanes, now());
}
const didAttemptEntireTree =
!workInProgressRootDidSkipSuspendedSiblings;
markRootSuspended(
Expand Down Expand Up @@ -1655,19 +1666,31 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {

if (includesSyncLane(lanes) || includesBlockingLane(lanes)) {
logBlockingStart(
blockingUpdateTime,
blockingEventTime,
blockingUpdateTime >= 0 && blockingUpdateTime < blockingClampTime
? blockingClampTime
: blockingUpdateTime,
blockingEventTime >= 0 && blockingEventTime < blockingClampTime
? blockingClampTime
: blockingEventTime,
blockingEventType,
blockingEventIsRepeat,
renderStartTime,
);
clearBlockingTimers();
}
if (includesTransitionLane(lanes)) {
logTransitionStart(
transitionStartTime,
transitionUpdateTime,
transitionEventTime,
transitionStartTime >= 0 && transitionStartTime < transitionClampTime
? transitionClampTime
: transitionStartTime,
transitionUpdateTime >= 0 && transitionUpdateTime < transitionClampTime
? transitionClampTime
: transitionUpdateTime,
transitionEventTime >= 0 && transitionEventTime < transitionClampTime
? transitionClampTime
: transitionEventTime,
transitionEventType,
transitionEventIsRepeat,
renderStartTime,
);
clearTransitionTimers();
Expand Down Expand Up @@ -3139,6 +3162,11 @@ function commitRootImpl(
// with setTimeout
pendingPassiveTransitions = transitions;
scheduleCallback(NormalSchedulerPriority, () => {
if (enableProfilerTimer && enableComponentPerformanceTrack) {
// Track the currently executing event if there is one so we can ignore this
// event when logging events.
trackSchedulerEvent();
}
flushPassiveEffects(true);
// This render triggered passive effects: release the root cache pool
// *after* passive effects fire to avoid freeing a cache pool that may
Expand Down
48 changes: 27 additions & 21 deletions packages/react-reconciler/src/ReactProfilerTimer.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,18 @@ export let componentEffectDuration: number = -0;
export let componentEffectStartTime: number = -1.1;
export let componentEffectEndTime: number = -1.1;

export let blockingClampTime: number = -0;
export let blockingUpdateTime: number = -1.1; // First sync setState scheduled.
export let blockingEventTime: number = -1.1; // Event timeStamp of the first setState.
export let blockingEventType: null | string = null; // Event type of the first setState.
export let blockingEventIsRepeat: boolean = false;
// TODO: This should really be one per Transition lane.
export let transitionClampTime: number = -0;
export let transitionStartTime: number = -1.1; // First startTransition call before setState.
export let transitionUpdateTime: number = -1.1; // First transition setState scheduled.
export let transitionEventTime: number = -1.1; // Event timeStamp of the first transition.
export let transitionEventType: null | string = null; // Event type of the first transition.
export let transitionEventIsRepeat: boolean = false;

export function startUpdateTimerByLane(lane: Lane): void {
if (!enableProfilerTimer || !enableComponentPerformanceTrack) {
Expand All @@ -52,15 +56,25 @@ export function startUpdateTimerByLane(lane: Lane): void {
if (isSyncLane(lane) || isBlockingLane(lane)) {
if (blockingUpdateTime < 0) {
blockingUpdateTime = now();
blockingEventTime = resolveEventTimeStamp();
blockingEventType = resolveEventType();
const newEventTime = resolveEventTimeStamp();
const newEventType = resolveEventType();
blockingEventIsRepeat =
newEventTime === blockingEventTime &&
newEventType === blockingEventType;
blockingEventTime = newEventTime;
blockingEventType = newEventType;
}
} else if (isTransitionLane(lane)) {
if (transitionUpdateTime < 0) {
transitionUpdateTime = now();
if (transitionStartTime < 0) {
transitionEventTime = resolveEventTimeStamp();
transitionEventType = resolveEventType();
const newEventTime = resolveEventTimeStamp();
const newEventType = resolveEventType();
transitionEventIsRepeat =
newEventTime === transitionEventTime &&
newEventType === transitionEventType;
transitionEventTime = newEventTime;
transitionEventType = newEventType;
}
}
}
Expand All @@ -76,8 +90,13 @@ export function startAsyncTransitionTimer(): void {
}
if (transitionStartTime < 0 && transitionUpdateTime < 0) {
transitionStartTime = now();
transitionEventTime = resolveEventTimeStamp();
transitionEventType = resolveEventType();
const newEventTime = resolveEventTimeStamp();
const newEventType = resolveEventType();
transitionEventIsRepeat =
newEventTime === transitionEventTime &&
newEventType === transitionEventType;
transitionEventTime = newEventTime;
transitionEventType = newEventType;
}
}

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

export function clampTransitionTimers(finalTime: number): void {
Expand All @@ -130,15 +144,7 @@ export function clampTransitionTimers(finalTime: number): void {
// If we had new updates come in while we were still rendering or committing, we don't want
// those update times to create overlapping tracks in the performance timeline so we clamp
// them to the end of the commit phase.
if (transitionStartTime >= 0 && transitionStartTime < finalTime) {
transitionStartTime = finalTime;
}
if (transitionUpdateTime >= 0 && transitionUpdateTime < finalTime) {
transitionUpdateTime = finalTime;
}
if (transitionEventTime >= 0 && transitionEventTime < finalTime) {
transitionEventTime = finalTime;
}
transitionClampTime = finalTime;
}

export function pushNestedEffectDurations(): number {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ describe('ReactFiberHostContext', () => {
}
return DefaultEventPriority;
},
trackSchedulerEvent: function () {},
resolveEventType: function () {
return null;
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export const getInstanceFromScope = $$$config.getInstanceFromScope;
export const setCurrentUpdatePriority = $$$config.setCurrentUpdatePriority;
export const getCurrentUpdatePriority = $$$config.getCurrentUpdatePriority;
export const resolveUpdatePriority = $$$config.resolveUpdatePriority;
export const trackSchedulerEvent = $$$config.trackSchedulerEvent;
export const resolveEventType = $$$config.resolveEventType;
export const resolveEventTimeStamp = $$$config.resolveEventTimeStamp;
export const shouldAttemptEagerTransition =
Expand Down
3 changes: 2 additions & 1 deletion packages/react-test-renderer/src/ReactFiberConfigTestHost.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,11 @@ export function resolveUpdatePriority(): EventPriority {
}
return DefaultEventPriority;
}

export function trackSchedulerEvent(): void {}
export function resolveEventType(): null | string {
return null;
}

export function resolveEventTimeStamp(): number {
return -1.1;
}
Expand Down
Loading