[Fiber] Remove callbackList field from Fiber #8728
Conversation
acdlite
force-pushed
the
fibernocallbacklist
branch
from
fbec958 to
9dbb39e
Compare
| altQueue.last = sourceQueue.last; | ||
| altQueue.hasForceUpdate = sourceQueue.hasForceUpdate; | ||
| altQueue.callbackList = sourceQueue.callbackList; | ||
| altQueue.isProcessing = sourceQueue.isProcessing; |
There was a problem hiding this comment.
Why are we setting isProcessing here, but didn’t set before? Is this fixing a bug?
There was a problem hiding this comment.
No bug, just noticed it wasn't being copied and I added it for consistency's sake. Although actually, callbackList, isProcessing, and hasForceUpdate can be reset when cloning. I'll do that instead. #8728 (comment)
| // Queue is now empty | ||
| queue.callbackList = callbackList; | ||
|
|
||
| if (!queue.first && !queue.callbackList && !queue.hasForceUpdate) { |
There was a problem hiding this comment.
Minor nit: do you need to dereference queue.callbackList here? Can you use callbackList directly?
| let update = callbackList.first; | ||
| while (update) { | ||
| const callback = update.callback; | ||
| if (typeof callback === 'function') { |
There was a problem hiding this comment.
Was this check unnecessary? Are we validating early now?
There was a problem hiding this comment.
Brian added validateCallback checks whenever a callback is enqueued
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Why do we not need to reset it now?
There was a problem hiding this comment.
Didn't need it before either, really. The field gets reset whenever we beginWork.
There was a problem hiding this comment.
This reminds me though that in cloneUpdateQueue, some of those fields should just be reset since by the time we clone from current, they're no longer valid. Not an observable change but it's what cloneFiber does so better to be consistent.
gaearon
left a comment
gaearon
left a comment
There was a problem hiding this comment.
Looks good, left a few comments for parts I don't understand. Feel free to merge if there are good answers. :-)
Moves it to UpdateQueue instead so that we don't waste memory for components that don't have update queues.
Similar to what cloneFiber does. No change in behavior, but it's more consistent this way.
acdlite
force-pushed
the
fibernocallbacklist
branch
from
9dbb39e to
288a4c4
Compare
| first: Update | null, | ||
| last: Update | null, | ||
| hasForceUpdate: boolean, | ||
| callbackList: null | Array<Callback>, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, though may I ask why it's ok to use arrays for context but not here?
There was a problem hiding this comment.
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.
4 participants
Moves it to UpdateQueue instead so that we don't waste memory for components that don't have update queues.
Unobservable, so no tests.