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

Decouple update queue from Fiber type #12600

Merged
merged 10 commits into from Apr 23, 2018

Conversation

Projects
None yet
8 participants
@acdlite
Member

acdlite commented Apr 10, 2018

The update queue is in need of a refactor. Recent bugfixes (#12528) have exposed some flaws in how it's modeled. Upcoming features like Suspense and [redacted] also rely on the update queue in ways that weren't anticipated in the original design.

Major changes:

  • Instead of boolean flags for isReplace and isForceUpdate, updates have a tag field (like Fiber). This lowers the cost for adding new types of updates.
  • Render phase updates are special cased. Updates scheduled during the render phase are dropped if the work-in-progress does not commit. This is used for getDerivedStateFrom{Props,Catch}.
  • callbackList has been replaced with a generic effect list. Aside from callbacks, this is also used for componentDidCatch.

TODO:

  • Class components
  • Host roots
  • Fix Flow
  • Update React DevTools dependency on hasForceUpdate (in a separate repo) I reverted this change
@@ -248,7 +248,7 @@ exports[`ReactDebugFiberPerf recovers from caught errors 1`] = `
⛔ (Committing Changes) Warning: Lifecycle hook scheduled a cascading update
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 2 Total)
⚛ (Calling Lifecycle Methods: 0 Total)
⚛ (Calling Lifecycle Methods: 1 Total)

This comment has been minimized.

@acdlite

acdlite Apr 10, 2018

Member

This is because we weren't counting componentDidCatch as a lifecycle previously

@pull-bot

This comment has been minimized.

pull-bot commented Apr 10, 2018

ReactDOM: size: 🔺+0.4%, gzip: -0.2%

Details of bundled changes.

Comparing: 999b656...e4ce363

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.3% +0.6% 609.63 KB 611.61 KB 140.53 KB 141.35 KB UMD_DEV
react-dom.production.min.js 🔺+0.4% -0.2% 100.2 KB 100.56 KB 31.97 KB 31.9 KB UMD_PROD
react-dom.development.js +0.3% +0.6% 594 KB 595.99 KB 136.37 KB 137.22 KB NODE_DEV
react-dom.production.min.js 🔺+0.3% 0.0% 98.65 KB 98.99 KB 31.1 KB 31.11 KB NODE_PROD
react-dom-test-utils.development.js -0.0% 0.0% 40.75 KB 40.75 KB 11.69 KB 11.69 KB UMD_DEV
react-dom-test-utils.development.js -0.0% -0.0% 35.61 KB 35.61 KB 10.27 KB 10.27 KB NODE_DEV
ReactDOM-dev.js +0.3% +0.6% 618.41 KB 620.4 KB 139.14 KB 139.94 KB FB_WWW_DEV
ReactDOM-prod.js -0.1% -0.4% 284.94 KB 284.78 KB 52.3 KB 52.09 KB FB_WWW_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.5% +0.9% 412.73 KB 414.71 KB 89.59 KB 90.41 KB UMD_DEV
react-art.production.min.js 🔺+0.4% -0.3% 90.48 KB 90.85 KB 27.62 KB 27.54 KB UMD_PROD
react-art.development.js +0.6% +1.1% 338.56 KB 340.55 KB 70.81 KB 71.62 KB NODE_DEV
react-art.production.min.js 🔺+0.6% -0.5% 55.04 KB 55.37 KB 16.87 KB 16.79 KB NODE_PROD
ReactART-dev.js +0.6% +1.1% 346.81 KB 348.8 KB 70.42 KB 71.18 KB FB_WWW_DEV
ReactART-prod.js -0.1% -0.7% 167.61 KB 167.48 KB 27.75 KB 27.57 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.6% +1.1% 345.69 KB 347.67 KB 72.47 KB 73.28 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.6% -0.2% 54.77 KB 55.13 KB 16.71 KB 16.68 KB UMD_PROD
react-test-renderer.development.js +0.6% +1.2% 336.51 KB 338.5 KB 69.66 KB 70.46 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.6% -0.1% 54.02 KB 54.36 KB 16.35 KB 16.33 KB NODE_PROD
ReactTestRenderer-dev.js +0.6% +1.1% 344.98 KB 346.97 KB 69.31 KB 70.09 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.6% +1.2% 316.9 KB 318.89 KB 65.12 KB 65.92 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.7% -0.5% 46.91 KB 47.24 KB 14.3 KB 14.23 KB NODE_PROD
react-reconciler-persistent.development.js +0.6% +1.2% 316.23 KB 318.22 KB 64.88 KB 65.69 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.7% 🔺+0.2% 45.83 KB 46.16 KB 14.08 KB 14.11 KB NODE_PROD
react-reconciler-reflection.development.js -0.0% 0.0% 11.06 KB 11.06 KB 3.42 KB 3.42 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.4% +0.8% 456.63 KB 458.64 KB 97.38 KB 98.15 KB RN_FB_DEV
ReactNativeRenderer-prod.js -0.3% -0.7% 218.26 KB 217.69 KB 36.66 KB 36.39 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.4% +0.8% 456.38 KB 458.39 KB 97.32 KB 98.08 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.9% -0.1% 215.01 KB 216.91 KB 36.29 KB 36.26 KB RN_OSS_PROD
ReactFabric-dev.js +0.5% +0.9% 439.04 KB 441.05 KB 92.94 KB 93.75 KB RN_FB_DEV
ReactFabric-prod.js 🔺+1.0% 🔺+0.3% 200.59 KB 202.5 KB 33.55 KB 33.65 KB RN_FB_PROD
ReactFabric-dev.js +0.5% +0.9% 439.08 KB 441.09 KB 92.96 KB 93.76 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+1.0% 🔺+0.3% 200.63 KB 202.54 KB 33.57 KB 33.67 KB RN_OSS_PROD

Generated by 🚫 dangerJS

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 10, 2018

Need to add “update DevTools” to todos. I think it might rely on isForceUpdate somewhere.

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Apr 10, 2018

Need to add “update DevTools” to todos. I think it might rely on isForceUpdate somewhere.

I don't think so? I don't see any references to isForced.

@gaearon

This comment has been minimized.

@acdlite

This comment has been minimized.

Member

acdlite commented Apr 10, 2018

Note: the bundle size changes are mostly because I haven't deleted the old update queue yet :D

@acdlite acdlite changed the title from [WIP] Decouple update queue from Fiber type to Decouple update queue from Fiber type Apr 11, 2018

@acdlite

This comment has been minimized.

Member

acdlite commented Apr 11, 2018

@sebmarkbage Ready for review

⚛ (Committing Host Effects: 1 Total)
⚛ (Calling Lifecycle Methods: 1 Total)
⚛ (Committing Host Effects: 0 Total)
⚛ (Calling Lifecycle Methods: 0 Total)

This comment has been minimized.

@acdlite

acdlite Apr 11, 2018

Member

I don't know why this wasn't already 0. There are no host effects or lifecycles in the second flush. New snapshot seems correct.

@@ -274,7 +274,7 @@ exports[`ReactDebugFiberPerf recovers from fatal errors 1`] = `
⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 1 Total)
⚛ (Calling Lifecycle Methods: 0 Total)
⚛ (Calling Lifecycle Methods: 1 Total)

This comment has been minimized.

@acdlite
@acdlite

This comment has been minimized.

Member

acdlite commented Apr 11, 2018

Needless to say, this is a risky change. I'll do the next www sync and fix any bugs that may come up.

@gaearon gaearon referenced this pull request Apr 11, 2018

Merged

[Danger] Minor fixes #12606

@NE-SmallTown

This comment has been minimized.

Contributor

NE-SmallTown commented Apr 13, 2018

Upcoming features like Suspense and [redacted] also rely on the update queue in ways that weren't anticipated in the original design.

What does [redacted] mean?

@acdlite acdlite referenced this pull request Apr 14, 2018

Merged

Suspense #12279

4 of 5 tasks complete
@@ -749,10 +684,11 @@ export default function(
): boolean {
const ctor = workInProgress.type;
const instance = workInProgress.stateNode;
resetInputPointers(workInProgress, instance);

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 18, 2018

Member

What is this change? You didn't replicate the other assignment so something is new.

Object.assign(resultState, partialState);
}
function enqueueDerivedStateFromProps(

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 18, 2018

Member

DRY for DRYness sake? Makes it harder to read this inline IMO. It also encourages reading the getDerivedStateFromProps property in multiple places if they're also needed at the callsite. Only abstract if it is chunk of work. (On the flip side, this PR successfully unabstract memoizeProps and memoizeState in places.)

if (updateQueue !== null && updateQueue.capturedValues !== null) {
workInProgress.effectTag &= ~DidCapture;
if (typeof instance.componentDidCatch === 'function') {
workInProgress.effectTag |= ErrLog;

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 18, 2018

Member

There was a reason we had to apply these effects in the complete phase. Why are we able to move them to the begin phase now?

This comment has been minimized.

@acdlite

acdlite Apr 20, 2018

Member

I don't think there was a reason. It happened to be a convenient place to do it. Now we do it while processing the update queue.

@@ -19,14 +19,14 @@ export const Update = /* */ 0b000000000100;
export const PlacementAndUpdate = /* */ 0b000000000110;
export const Deletion = /* */ 0b000000001000;
export const ContentReset = /* */ 0b000000010000;
export const Callback = /* */ 0b000000100000;
export const UpdateQueue = /* */ 0b000000100000;

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 18, 2018

Member

What are theses? They're not described in terms of side-effects. Either this needs to change, or the whole thing renamed.

const ClassUpdateQueue = 0;
const RootUpdateQueue = 1;
type UpdateShared<Payload, U> = {

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 18, 2018

Member

I really had the term "Shared". This has been sneaking into RN too.

It doesn't describe anything about the concept other than DRYness which indicates that maybe you should repeat yourself. If it is real concept, name it.

This comment has been minimized.

@acdlite

acdlite Apr 18, 2018

Member

Stop denying me muh mixins!

type RootUpdateQueueType = UpdateQueueShared<RootUpdate, ReactNodeList>;
type UpdateQueueOwner<Queue, State> = {
alternate: UpdateQueueOwner<Queue> | null,

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 18, 2018

Member

This is clearly not going to be how we describe "redacted" so seems like a too early abstraction. Even just assuming that it is a single argument is a big assumption. Just use Fiber until you have something else.

}
// Clear the list of render phase updates.
finishedQueue.firstRenderPhaseUpdate = finishedQueue.lastRenderPhaseUpdate = null;
}

This comment has been minimized.

@sebmarkbage

sebmarkbage Apr 18, 2018

Member

You are forking into a first class concept with typeOfUpdateQueue then you're picking separate code paths again in commitEffect. The only thing gain by doing that instead of specializing each function is that you can reuse the code above here. Just break this out into a reusable function and instead specialize each code path without making typeOfUpdateQueue a first class thing.

@acdlite

This comment has been minimized.

Member

acdlite commented Apr 18, 2018

TODO after chatting to @sebmarkbage:

  • Use first-class functions instead of typeOfUpdateQueue
  • Separate effect list into "own" effects and children effects
  • Rename UpdateQueue side effect type to something that makes more sense
  • Remove ForceUpdate effect and go back to queue.hasForceUpdate instead

acdlite added some commits Apr 10, 2018

Decouple update queue from Fiber type
The update queue is in need of a refactor. Recent bugfixes (#12528) have
exposed some flaws in how it's modeled. Upcoming features like Suspense
and [redacted] also rely on the update queue in ways that weren't
anticipated in the original design.

Major changes:

- Instead of boolean flags for `isReplace` and `isForceUpdate`, updates
have a `tag` field (like Fiber). This lowers the cost for adding new
types of updates.
- Render phase updates are special cased. Updates scheduled during
the render phase are dropped if the work-in-progress does not commit.
This is used for `getDerivedStateFrom{Props,Catch}`.
- `callbackList` has been replaced with a generic effect list. Aside
from callbacks, this is also used for `componentDidCatch`.
Remove first class UpdateQueue types and use closures instead
I tried to avoid this at first, since we avoid it everywhere else in the Fiber
codebase, but since updates are not in a hot path, the trade off with file size
seems worth it.
Store captured errors on a separate part of the update queue
This way they can be reused independently of updates like
getDerivedStateFromProps. This will be important for resuming.
Revert back to storing hasForceUpdate on the update queue
Instead of using the effect tag. Ideally, this would be part of the
return type of processUpdateQueue.
Rename UpdateQueue effect type back to Callback
I don't love this name either, but it's less confusing than UpdateQueue
I suppose. Conceptually, this is usually a callback: setState callbacks,
componentDidCatch. The only case that feels a bit weird is Timeouts,
which use this effect to attach a promise listener. I guess that kinda
fits, too.
Call getDerivedStateFromProps every render, even if props did not change
Rather than enqueue a new setState updater for every props change, we
can skip the update queue entirely and merge the result into state at
the end. This makes more sense, since "receiving props" is not an event
that should be observed. It's still a bit weird, since eventually we do
persist the derived state (in other words, it accumulates).
Store captured effects on separate list from "own" effects (callbacks)
For resuming, we need the ability to discard the "own" effects while
reusing the captured effects.
@@ -1421,7 +1422,7 @@ describe('ReactIncremental', () => {
]);
});
it('does not call static getDerivedStateFromProps for state-only updates', () => {
it('calls getDerivedStateFromProps even for state-only updates', () => {

This comment has been minimized.

@gaearon

gaearon Apr 21, 2018

Member

Why this change? Do I need to update my diagram? 😬

Does this fix some edge case?

This comment has been minimized.

@acdlite

acdlite Apr 21, 2018

Member

getDerivedStateFromProps is inherently weird because it treats receiving props as an event, which is not how React works. With this change, we call it on every render, not only on prop changes, and we only persist the result to the queue once there's no more work to be done. So the result still accumulates, but it's more deterministic — not completely deterministic, but it works for most cases. The implementation is also simpler, because we don't have to enqueue an update.

If you're use getDerivedStateFromProps the way it's intended to be used, then this change shouldn't affect anything. If you're using it to count the number of renders, or to accumulate prop change "events", then this is breaking. We (Brian, Sebastian, and I) decided it was important enough to put in a minor. Call it a bugfix.

This comment has been minimized.

@acdlite

acdlite Apr 21, 2018

Member

The three use cases for getDerivedStateFromProps are:

  • Memoization. We have a better model for how this should work in the future.
  • Avoiding the need to derive the same value repeatedly in multiple methods/lifecycles. (Similar to the problem that defaultProps solves.) Again, our future model handles this better.
  • Reacting to prop changes. This is the weird one, and I don't think we totally understand yet what the best practices should be.

This comment has been minimized.

@gaearon

gaearon Apr 30, 2018

Member

If we back out of this change let's also remember to back out of #12676.

This comment has been minimized.

@guillaumep

guillaumep May 30, 2018

Going from React 16.3 to 16.4 has caused two major regressions in my application because of this change. I was relying on the fact that getDerivedStateFromProps() was only called when the props changed, in the same way componentWillReceiveProps worked.

Granted, I probably did not understand the original intent of getDerivedStateFromProps. My case is this is more than just a simple bug fix.

This comment has been minimized.

@bvaughn

bvaughn May 30, 2018

Contributor

@guillaumep I'm sorry to hear that 16.4 caused troubles for you.

The point you raise is discussed in detail in #12898 if you'd like to learn more about why we made it.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 21, 2018

What’s special about updates that we decided to use first class functions instead of tagging types? Smells Stack-y and I wonder what our criteria are.

Optimize for class components
Change `process` and `callback` to match the expected payload types
for class components. I had intended for the update queue to be reusable
for both class components and a future React API, but we'll likely have
to fork anyway.
@acdlite

This comment has been minimized.

Member

acdlite commented Apr 21, 2018

@gaearon I reverted some of this in the most recent commit, to optimize for class components, but according to @sebmarkbage, some things to consider are whether we're in a hot path, and the impact on file size.

So ReactChildFiber is a big file because of all the duplication, but we live with it because reconciliation is a super hot path and the file size is worth it. Queuing and processing updates is not a hot path, so we trade off file size for some extra closures.

TejasQ added a commit to TejasQ/react that referenced this pull request Aug 26, 2018

Do not fire getDerivedStateFromProps unless props or state have chang…
…ed (facebook#12802)

Fixes an oversight from facebook#12600. getDerivedStateFromProps should fire
if either props *or* state have changed, but not if *neither* have
changed. This prevents a parent from re-rendering if a deep child
receives an update.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment