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

Renamed *BaseTime Fiber attributes to *baseDuration #13156

Merged
merged 1 commit into from
Jul 6, 2018
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
16 changes: 8 additions & 8 deletions packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,12 @@ export type Fiber = {|
// Duration of the most recent render time for this Fiber.
// This value is not updated when we bailout for memoization purposes.
// This field is only set when the enableProfilerTimer flag is enabled.
selfBaseTime?: number,
selfBaseDuration?: number,

// Sum of base times for all descedents of this Fiber.
// This value bubbles up during the "complete" phase.
// This field is only set when the enableProfilerTimer flag is enabled.
treeBaseTime?: number,
treeBaseDuration?: number,

// Conceptual aliases
// workInProgress : Fiber -> alternate The alternate used for reuse happens
Expand Down Expand Up @@ -230,8 +230,8 @@ function FiberNode(
if (enableProfilerTimer) {
this.actualDuration = 0;
this.actualStartTime = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this one (actualStartTime) intentionally still called "time"? Or would it be more consistent to rename it too?

If it's intentionally still "time", would you say "time" now always refers to timestamps and "duration" always means length? Or is it still different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Start time and commit time are accurate. They both contain performance.now() values. Actual and base durations contain time deltas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So yes, "time" now always means a point in time and "duration" always means a length of time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's almost like this was the plan all along! 😁

this.selfBaseTime = 0;
this.treeBaseTime = 0;
this.selfBaseDuration = 0;
this.treeBaseDuration = 0;
}

if (__DEV__) {
Expand Down Expand Up @@ -338,8 +338,8 @@ export function createWorkInProgress(
workInProgress.ref = current.ref;

if (enableProfilerTimer) {
workInProgress.selfBaseTime = current.selfBaseTime;
workInProgress.treeBaseTime = current.treeBaseTime;
workInProgress.selfBaseDuration = current.selfBaseDuration;
workInProgress.treeBaseDuration = current.treeBaseDuration;
}

return workInProgress;
Expand Down Expand Up @@ -569,8 +569,8 @@ export function assignFiberPropertiesInDEV(
if (enableProfilerTimer) {
target.actualDuration = source.actualDuration;
target.actualStartTime = source.actualStartTime;
target.selfBaseTime = source.selfBaseTime;
target.treeBaseTime = source.treeBaseTime;
target.selfBaseDuration = source.selfBaseDuration;
target.treeBaseDuration = source.treeBaseDuration;
}
target._debugID = source._debugID;
target._debugSource = source._debugSource;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
finishedWork.memoizedProps.id,
current === null ? 'mount' : 'update',
finishedWork.actualDuration,
finishedWork.treeBaseTime,
finishedWork.treeBaseDuration,
finishedWork.actualStartTime,
getCommitTime(),
);
Expand Down
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -736,10 +736,10 @@ function resetExpirationTime(
// Bubble up the earliest expiration time.
// (And "base" render timers if that feature flag is enabled)
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
let treeBaseTime = workInProgress.selfBaseTime;
let treeBaseDuration = workInProgress.selfBaseDuration;
let child = workInProgress.child;
while (child !== null) {
treeBaseTime += child.treeBaseTime;
treeBaseDuration += child.treeBaseDuration;
if (
child.expirationTime !== NoWork &&
(newExpirationTime === NoWork ||
Expand All @@ -749,7 +749,7 @@ function resetExpirationTime(
}
child = child.sibling;
}
workInProgress.treeBaseTime = treeBaseTime;
workInProgress.treeBaseDuration = treeBaseDuration;
} else {
let child = workInProgress.child;
while (child !== null) {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactProfilerTimer.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ function recordElapsedBaseRenderTimeIfRunning(fiber: Fiber): void {
return;
}
if (baseStartTime !== -1) {
fiber.selfBaseTime = now() - baseStartTime;
fiber.selfBaseDuration = now() - baseStartTime;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ describe('ReactProfiler DevTools integration', () => {
// At this point, the base time should include both:
// The time 2ms in the App component itself, and
// The 10ms spend in the Profiler sub-tree beneath.
expect(rendered.root.findByType(App)._currentFiber().treeBaseTime).toBe(12);
expect(rendered.root.findByType(App)._currentFiber().treeBaseDuration).toBe(
12,
);

rendered.update(<App multiplier={2} />);

Expand All @@ -106,7 +108,9 @@ describe('ReactProfiler DevTools integration', () => {
// At this point, the base time should include both:
// The initial 9ms for the components that do not re-render, and
// The updated 6ms for the component that does.
expect(rendered.root.findByType(App)._currentFiber().treeBaseTime).toBe(15);
expect(rendered.root.findByType(App)._currentFiber().treeBaseDuration).toBe(
15,
);
});

it('should reset the fiber stack correctly after an error when profiling host roots', () => {
Expand Down Expand Up @@ -141,8 +145,8 @@ describe('ReactProfiler DevTools integration', () => {
// At this point, the base time should include only the most recent (not failed) render.
// It should not include time spent on the initial render,
// Or time that elapsed between any of the above renders.
expect(rendered.root.findByType('div')._currentFiber().treeBaseTime).toBe(
7,
);
expect(
rendered.root.findByType('div')._currentFiber().treeBaseDuration,
).toBe(7);
});
});