Skip to content

Track event times per lane on the root #19387

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.

Sign up for GitHub

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

Merged
acdlite merged 2 commits into from
Jul 17, 2020
Merged

Track event times per lane on the root #19387

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99f7728
Pass event time to markRootUpdated
acdlite Jul 16, 2020
6a2703c
Track event times per lane on the root
acdlite Jul 16, 2020
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
Prev Previous commit
Track event times per lane on the root 
Previous strategy was to store the event time on the update object
and accumulate the most recent one during the render phase.

Among other advantages, by tracking them on the root, we can read the
event time before the render phase has finished.

I haven't removed the `eventTime` field from the update object yet,
because it's still used to compute the timeout. Tracking the timeout
on the root is my next step.
  • Loading branch information
@acdlite
acdlite committed Jul 17, 2020
commit 6a2703c593171e9e2583638717f63584eb840f01
38 changes: 36 additions & 2 deletions packages/react-reconciler/src/ReactFiberLane.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,25 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
return nextLanes;
}

export function getMostRecentEventTime(root: FiberRoot, lanes: Lanes): number {
const eventTimes = root.eventTimes;

let mostRecentEventTime = NoTimestamp;
while (lanes > 0) {
const index = pickArbitraryLaneIndex(lanes);
const lane = 1 << index;

const eventTime = eventTimes[index];
if (eventTime > mostRecentEventTime) {
mostRecentEventTime = eventTime;
}

lanes &= ~lane;
}

return mostRecentEventTime;
}

function computeExpirationTime(lane: Lane, currentTime: number) {
// TODO: Expiration heuristic is constant per lane, so could use a map.
getHighestPriorityLanes(lane);
Expand Down Expand Up @@ -606,10 +625,14 @@ export function pickArbitraryLane(lanes: Lanes): Lane {
return getHighestPriorityLane(lanes);
}

function pickArbitraryLaneIndex(lanes: Lane | Lanes) {
function pickArbitraryLaneIndex(lanes: Lanes) {
return 31 - clz32(lanes);
}

function laneToIndex(lane: Lane) {
return pickArbitraryLaneIndex(lane);
}

export function includesSomeLane(a: Lanes | Lane, b: Lanes | Lane) {
return (a & b) !== NoLanes;
}
Expand Down Expand Up @@ -670,6 +693,12 @@ export function markRootUpdated(

root.suspendedLanes &= higherPriorityLanes;
root.pingedLanes &= higherPriorityLanes;

const eventTimes = root.eventTimes;
const index = laneToIndex(updateLane);
// We can always overwrite an existing timestamp because we prefer the most
// recent event, and we assume time is monotonically increasing.
eventTimes[index] = eventTime;
}

export function markRootSuspended(root: FiberRoot, suspendedLanes: Lanes) {
Expand Down Expand Up @@ -727,13 +756,18 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) {

root.entangledLanes &= remainingLanes;

const entanglements = root.entanglements;
const eventTimes = root.eventTimes;
const expirationTimes = root.expirationTimes;

// Clear the lanes that no longer have pending work
let lanes = noLongerPendingLanes;
while (lanes > 0) {
const index = pickArbitraryLaneIndex(lanes);
const lane = 1 << index;

// Clear the expiration time
entanglements[index] = NoLanes;
Copy link
Collaborator Author

@acdlite acdlite Jul 17, 2020

Choose a reason for hiding this comment

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

This line was an oversight from when I added the entanglement feature. It's theoretically observable but if I write a test it'll be super convoluted, since we currently only use entanglement for useMutableSource, and only in a particular de-opt case.

eventTimes[index] = NoTimestamp;
expirationTimes[index] = NoTimestamp;

lanes &= ~lane;
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberRoot.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function FiberRootNode(containerInfo, tag, hydrate) {
this.callbackNode = null;
this.callbackId = NoLanes;
this.callbackPriority = NoLanePriority;
this.eventTimes = createLaneMap(NoLanes);
this.expirationTimes = createLaneMap(NoTimestamp);

this.pendingLanes = NoLanes;
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberRoot.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function FiberRootNode(containerInfo, tag, hydrate) {
this.callbackNode = null;
this.callbackId = NoLanes;
this.callbackPriority = NoLanePriority;
this.eventTimes = createLaneMap(NoLanes);
this.expirationTimes = createLaneMap(NoTimestamp);

this.pendingLanes = NoLanes;
Expand Down
19 changes: 7 additions & 12 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ import {
getCurrentUpdateLanePriority,
markStarvedLanesAsExpired,
getLanesToRetrySynchronouslyOnError,
getMostRecentEventTime,
markRootUpdated,
markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
markRootPinged,
Expand Down Expand Up @@ -294,8 +295,6 @@ const subtreeRenderLanesCursor: StackCursor<Lanes> = createCursor(NoLanes);
let workInProgressRootExitStatus: RootExitStatus = RootIncomplete;
// A fatal error, if one is thrown
let workInProgressRootFatalError: mixed = null;
// Most recent event time among processed updates during this render.
let workInProgressRootLatestProcessedEventTime: number = NoTimestamp;
let workInProgressRootLatestSuspenseTimeout: number = NoTimestamp;
let workInProgressRootCanSuspendUsingConfig: null | SuspenseConfig = null;
// "Included" lanes refer to lanes that were worked on during this render. It's
Expand Down Expand Up @@ -938,20 +937,21 @@ function finishConcurrentRender(root, finishedWork, exitStatus, lanes) {
break;
}

const mostRecentEventTime = getMostRecentEventTime(root, lanes);
let msUntilTimeout;
if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) {
// We have processed a suspense config whose expiration time we
// can use as the timeout.
msUntilTimeout = workInProgressRootLatestSuspenseTimeout - now();
} else if (workInProgressRootLatestProcessedEventTime === NoTimestamp) {
} else if (mostRecentEventTime === NoTimestamp) {
// This should never normally happen because only new updates
// cause delayed states, so we should have processed something.
// However, this could also happen in an offscreen tree.
msUntilTimeout = 0;
} else {
// If we didn't process a suspense config, compute a JND based on
// the amount of time elapsed since the most recent event time.
const eventTimeMs = workInProgressRootLatestProcessedEventTime;
const eventTimeMs = mostRecentEventTime;
const timeElapsedMs = now() - eventTimeMs;
msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
}
Expand All @@ -974,18 +974,19 @@ function finishConcurrentRender(root, finishedWork, exitStatus, lanes) {
}
case RootCompleted: {
// The work completed. Ready to commit.
const mostRecentEventTime = getMostRecentEventTime(root, lanes);
if (
// do not delay if we're inside an act() scope
!shouldForceFlushFallbacksInDEV() &&
workInProgressRootLatestProcessedEventTime !== NoTimestamp &&
mostRecentEventTime !== NoTimestamp &&
workInProgressRootCanSuspendUsingConfig !== null
) {
// If we have exceeded the minimum loading delay, which probably
// means we have shown a spinner already, we might have to suspend
// a bit longer to ensure that the spinner is shown for
// enough time.
const msUntilTimeout = computeMsUntilSuspenseLoadingDelay(
workInProgressRootLatestProcessedEventTime,
mostRecentEventTime,
workInProgressRootCanSuspendUsingConfig,
);
if (msUntilTimeout > 10) {
Expand Down Expand Up @@ -1323,7 +1324,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootRenderLanes = subtreeRenderLanes = workInProgressRootIncludedLanes = lanes;
workInProgressRootExitStatus = RootIncomplete;
workInProgressRootFatalError = null;
workInProgressRootLatestProcessedEventTime = NoTimestamp;
workInProgressRootLatestSuspenseTimeout = NoTimestamp;
workInProgressRootCanSuspendUsingConfig = null;
workInProgressRootSkippedLanes = NoLanes;
Expand Down Expand Up @@ -1441,11 +1441,6 @@ export function markRenderEventTimeAndConfig(
eventTime: number,
suspenseConfig: null | SuspenseConfig,
): void {
// Track the most recent event time of all updates processed in this batch.
if (workInProgressRootLatestProcessedEventTime < eventTime) {
workInProgressRootLatestProcessedEventTime = eventTime;
}

// Track the largest/latest timeout deadline in this batch.
// TODO: If there are two transitions in the same batch, shouldn't we
// choose the smaller one? Maybe this is because when an intermediate
Expand Down
19 changes: 7 additions & 12 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ import {
getCurrentUpdateLanePriority,
markStarvedLanesAsExpired,
getLanesToRetrySynchronouslyOnError,
getMostRecentEventTime,
markRootUpdated,
markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
markRootPinged,
Expand Down Expand Up @@ -286,8 +287,6 @@ const subtreeRenderLanesCursor: StackCursor<Lanes> = createCursor(NoLanes);
let workInProgressRootExitStatus: RootExitStatus = RootIncomplete;
// A fatal error, if one is thrown
let workInProgressRootFatalError: mixed = null;
// Most recent event time among processed updates during this render.
let workInProgressRootLatestProcessedEventTime: number = NoTimestamp;
let workInProgressRootLatestSuspenseTimeout: number = NoTimestamp;
let workInProgressRootCanSuspendUsingConfig: null | SuspenseConfig = null;
// "Included" lanes refer to lanes that were worked on during this render. It's
Expand Down Expand Up @@ -931,20 +930,21 @@ function finishConcurrentRender(root, finishedWork, exitStatus, lanes) {
break;
}

const mostRecentEventTime = getMostRecentEventTime(root, lanes);
let msUntilTimeout;
if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) {
// We have processed a suspense config whose expiration time we
// can use as the timeout.
msUntilTimeout = workInProgressRootLatestSuspenseTimeout - now();
} else if (workInProgressRootLatestProcessedEventTime === NoTimestamp) {
} else if (mostRecentEventTime === NoTimestamp) {
// This should never normally happen because only new updates
// cause delayed states, so we should have processed something.
// However, this could also happen in an offscreen tree.
msUntilTimeout = 0;
} else {
// If we didn't process a suspense config, compute a JND based on
// the amount of time elapsed since the most recent event time.
const eventTimeMs = workInProgressRootLatestProcessedEventTime;
const eventTimeMs = mostRecentEventTime;
const timeElapsedMs = now() - eventTimeMs;
msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
}
Expand All @@ -967,18 +967,19 @@ function finishConcurrentRender(root, finishedWork, exitStatus, lanes) {
}
case RootCompleted: {
// The work completed. Ready to commit.
const mostRecentEventTime = getMostRecentEventTime(root, lanes);
if (
// do not delay if we're inside an act() scope
!shouldForceFlushFallbacksInDEV() &&
workInProgressRootLatestProcessedEventTime !== NoTimestamp &&
mostRecentEventTime !== NoTimestamp &&
workInProgressRootCanSuspendUsingConfig !== null
) {
// If we have exceeded the minimum loading delay, which probably
// means we have shown a spinner already, we might have to suspend
// a bit longer to ensure that the spinner is shown for
// enough time.
const msUntilTimeout = computeMsUntilSuspenseLoadingDelay(
workInProgressRootLatestProcessedEventTime,
mostRecentEventTime,
workInProgressRootCanSuspendUsingConfig,
);
if (msUntilTimeout > 10) {
Expand Down Expand Up @@ -1316,7 +1317,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootRenderLanes = subtreeRenderLanes = workInProgressRootIncludedLanes = lanes;
workInProgressRootExitStatus = RootIncomplete;
workInProgressRootFatalError = null;
workInProgressRootLatestProcessedEventTime = NoTimestamp;
workInProgressRootLatestSuspenseTimeout = NoTimestamp;
workInProgressRootCanSuspendUsingConfig = null;
workInProgressRootSkippedLanes = NoLanes;
Expand Down Expand Up @@ -1434,11 +1434,6 @@ export function markRenderEventTimeAndConfig(
eventTime: number,
suspenseConfig: null | SuspenseConfig,
): void {
// Track the most recent event time of all updates processed in this batch.
if (workInProgressRootLatestProcessedEventTime < eventTime) {
workInProgressRootLatestProcessedEventTime = eventTime;
}

// Track the largest/latest timeout deadline in this batch.
// TODO: If there are two transitions in the same batch, shouldn't we
// choose the smaller one? Maybe this is because when an intermediate
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactInternalTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ type BaseFiberRootProperties = {|
// if it's already working.
callbackId: Lanes,
callbackPriority: LanePriority,
eventTimes: LaneMap<number>,
expirationTimes: LaneMap<number>,

pendingLanes: Lanes,
Expand Down