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.

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

Jump to bottom

[Fiber] Remove callbackList field from Fiber #8728

Merged
merged 2 commits into from
Jan 10, 2017
Merged

[Fiber] Remove callbackList field from Fiber #8728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4e971a
Remove callbackList field from Fiber
acdlite Jan 9, 2017
288a4c4
Reset invalid UpdateQueue fields when cloning from current
acdlite Jan 10, 2017
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
6 changes: 2 additions & 4 deletions src/renderers/shared/fiber/ReactFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ export type Fiber = {

// A queue of state updates and callbacks.
updateQueue: UpdateQueue | null,
// A list of callbacks that should be called during the next commit.
callbackList: UpdateQueue | null,

// The state used to create the output
memoizedState: any,

Expand Down Expand Up @@ -203,7 +202,6 @@ var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber {
pendingProps: null,
memoizedProps: null,
updateQueue: null,
callbackList: null,
memoizedState: null,

effectTag: NoEffect,
Expand Down Expand Up @@ -279,7 +277,7 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi
// pendingProps is here for symmetry but is unnecessary in practice for now.
// TODO: Pass in the new pendingProps as an argument maybe?
alt.pendingProps = fiber.pendingProps;
cloneUpdateQueue(alt, fiber);
cloneUpdateQueue(fiber, alt);
alt.pendingWorkPriority = priorityLevel;

alt.memoizedProps = fiber.memoizedProps;
Expand Down
12 changes: 6 additions & 6 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ var { commitCallbacks } = require('ReactFiberUpdateQueue');
var {
Placement,
Update,
Callback,
ContentReset,
} = require('ReactTypeOfSideEffect');

Expand Down Expand Up @@ -410,17 +411,16 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
}
}
}
const callbackList = finishedWork.callbackList;
if (callbackList) {
commitCallbacks(finishedWork, callbackList, instance);
if ((finishedWork.effectTag & Callback) && finishedWork.updateQueue) {
commitCallbacks(finishedWork, finishedWork.updateQueue, instance);
}
return;
}
case HostRoot: {
const callbackList = finishedWork.callbackList;
if (callbackList) {
const updateQueue = finishedWork.updateQueue;
if (updateQueue) {
const instance = finishedWork.child && finishedWork.child.stateNode;
commitCallbacks(finishedWork, callbackList, instance);
commitCallbacks(finishedWork, updateQueue, instance);
}
return;
}
Expand Down
1 change: 0 additions & 1 deletion src/renderers/shared/fiber/ReactFiberRoot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ exports.createFiberRoot = function(containerInfo : any) : FiberRoot {
containerInfo: containerInfo,
isScheduled: false,
nextScheduledRoot: null,
callbackList: null,
context: null,
pendingContext: null,
};
Expand Down
69 changes: 32 additions & 37 deletions src/renderers/shared/fiber/ReactFiberUpdateQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export type UpdateQueue = {
first: Update | null,
last: Update | null,
hasForceUpdate: boolean,
callbackList: null | Array<Callback>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This used to use a linked list like all the other structures. That requires dynamic reallocation as it grows. It also requires us to take on a dependency on such an implementation. This might matter if we make a WASM port.

Would it make sense to just use a simple linked list here too, just for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, though may I ask why it's ok to use arrays for context but not here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In context it is a fixed general stack. In normal use it never gets reallocated or expanded. The wasm implementation can be as easy as a fixed one time linear allocation. Where as this is dynamically allocated and growing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#8752


// Dev only
isProcessing?: boolean,
Expand Down Expand Up @@ -95,13 +96,15 @@ function ensureUpdateQueue(fiber : Fiber) : UpdateQueue {
first: null,
last: null,
hasForceUpdate: false,
callbackList: null,
isProcessing: false,
};
} else {
queue = {
first: null,
last: null,
hasForceUpdate: false,
callbackList: null,
};
}

Expand All @@ -110,19 +113,25 @@ function ensureUpdateQueue(fiber : Fiber) : UpdateQueue {
}

// Clones an update queue from a source fiber onto its alternate.
function cloneUpdateQueue(alt : Fiber, fiber : Fiber) : UpdateQueue | null {
const sourceQueue = fiber.updateQueue;
if (!sourceQueue) {
function cloneUpdateQueue(current : Fiber, workInProgress : Fiber) : UpdateQueue | null {
const currentQueue = current.updateQueue;
if (!currentQueue) {
// The source fiber does not have an update queue.
alt.updateQueue = null;
workInProgress.updateQueue = null;
return null;
}
// If the alternate already has a queue, reuse the previous object.
const altQueue = alt.updateQueue || {};
altQueue.first = sourceQueue.first;
altQueue.last = sourceQueue.last;
altQueue.hasForceUpdate = sourceQueue.hasForceUpdate;
alt.updateQueue = altQueue;
const altQueue = workInProgress.updateQueue || {};
altQueue.first = currentQueue.first;
altQueue.last = currentQueue.last;

// These fields are invalid by the time we clone from current. Reset them.
altQueue.hasForceUpdate = false;
altQueue.callbackList = null;
altQueue.isProcessing = false;

workInProgress.updateQueue = altQueue;

return altQueue;
}
exports.cloneUpdateQueue = cloneUpdateQueue;
Expand Down Expand Up @@ -438,29 +447,20 @@ function beginUpdateQueue(
// Second condition ignores top-level unmount callbacks if they are not the
// last update in the queue, since a subsequent update will cause a remount.
if (update.callback && !(update.isTopLevelUnmount && update.next)) {
const callbackUpdate = cloneUpdate(update);
if (callbackList && callbackList.last) {
callbackList.last.next = callbackUpdate;
callbackList.last = callbackUpdate;
} else {
callbackList = {
first: callbackUpdate,
last: callbackUpdate,
hasForceUpdate: false,
};
}
callbackList = callbackList || [];
callbackList.push(update.callback);
workInProgress.effectTag |= CallbackEffect;
}
update = update.next;
}

if (!queue.first && !queue.hasForceUpdate) {
// Queue is now empty
queue.callbackList = callbackList;

if (!queue.first && !callbackList && !queue.hasForceUpdate) {
// The queue is empty and there are no callbacks. We can reset it.
workInProgress.updateQueue = null;
}

workInProgress.callbackList = callbackList;

if (__DEV__) {
queue.isProcessing = false;
}
Expand All @@ -469,19 +469,14 @@ function beginUpdateQueue(
}
exports.beginUpdateQueue = beginUpdateQueue;

function commitCallbacks(finishedWork : Fiber, callbackList : UpdateQueue, context : mixed) {
const stopAfter = callbackList.last;
let update = callbackList.first;
while (update) {
const callback = update.callback;
if (typeof callback === 'function') {
callback.call(context);
}
if (update === stopAfter) {
break;
}
update = update.next;
function commitCallbacks(finishedWork : Fiber, queue : UpdateQueue, context : mixed) {
const callbackList = queue.callbackList;
if (!callbackList) {
return;
}
for (let i = 0; i < callbackList.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this check unnecessary? Are we validating early now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Brian added validateCallback checks whenever a callback is enqueued

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also the type is stricter now. Previously we iterated through a list of Updates, whose callback type is Function | null, but now we're iterating through a list of functions.

const callback = callbackList[i];
callback.call(context);
}
finishedWork.callbackList = null;
}
exports.commitCallbacks = commitCallbacks;