Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue #24247

acdlite · 2022-04-01T18:00:43Z

The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders.

The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgent render, then we can use the latest value.

acdlite · 2022-04-01T18:01:39Z

@bvaughn Do you have advice on what to do about react-debug-hooks? The internal hook layout has changed; there's only one internal hook object instead of two. Do I need to add a version check?

sizebot · 2022-04-01T18:05:36Z

Comparing: 0568c0f...e4fd270

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	131.28 kB	131.21 kB	+0.08%	41.94 kB	41.98 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	136.55 kB	136.47 kB	+0.10%	43.53 kB	43.57 kB
facebook-www/ReactDOM-prod.classic.js	=	435.21 kB	434.63 kB	+0.01%	79.86 kB	79.87 kB
facebook-www/ReactDOM-prod.modern.js	=	420.20 kB	419.63 kB	=	77.49 kB	77.50 kB
facebook-www/ReactDOMForked-prod.classic.js	=	435.21 kB	434.63 kB	+0.01%	79.86 kB	79.87 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
react-native/implementations/ReactNativeRenderer-profiling.fb.js	=	321.42 kB	320.76 kB	+0.02%	57.40 kB	57.42 kB
facebook-www/ReactART-prod.classic.js	=	279.43 kB	278.86 kB	+0.04%	49.52 kB	49.54 kB
react-native/implementations/ReactFabric-profiling.fb.js	=	319.26 kB	318.61 kB	+0.04%	57.08 kB	57.11 kB
facebook-www/ReactART-prod.modern.js	=	268.65 kB	268.08 kB	+0.04%	47.73 kB	47.76 kB
react-native/implementations/ReactNativeRenderer-profiling.js	=	305.32 kB	304.66 kB	=	54.70 kB	54.69 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js	=	264.94 kB	264.36 kB	+0.03%	47.88 kB	47.89 kB
react-native/implementations/ReactFabric-profiling.js	=	298.54 kB	297.88 kB	=	53.66 kB	53.66 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js	=	294.45 kB	293.80 kB	=	53.26 kB	53.19 kB
react-native/implementations/ReactFabric-prod.fb.js	=	292.34 kB	291.68 kB	=	52.86 kB	52.85 kB
react-native/implementations/ReactNativeRenderer-prod.js	=	286.32 kB	285.66 kB	=	51.56 kB	51.56 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js	=	250.15 kB	249.58 kB	=	45.56 kB	45.56 kB
react-native/implementations/ReactFabric-prod.js	=	279.59 kB	278.93 kB	=	50.44 kB	50.43 kB

Generated by 🚫 dangerJS against e4fd270

bvaughn · 2022-04-01T18:06:14Z

Do I need to add a version check?

Either that– or react-debug-tools just needs to make a breaking change (only support React 18.0.1+) and DevTools needs to import two versions of react-debug-tools (and switch based on version). This would align with what you, @lunaruan, and I talked about the other afternoon.

I guess this is one of those unavoidable downside of our hooks-as-a-flat-list architecture. 😄

Speaking of which, I wonder if there wouldn't be something clever we could do to the hooks structure to make packages like react-debug-tools a little more future proof to changes like this. (Like an auto-incremented ID attribute that could "batch" related primitive hooks.)

[
  // Only 1 hook with the batch ID signifies a simple hook e.g. useState, useReducer
  {
    batchID: 1,
    memoizedState: null,
    // ...
  },

  // 2 hooks sharing a batchID signifies a composite hook like useDeferredValue
  {
    batchID: 2,
    memoizedState: null,
    // ...
  },
  {
    batchID: 2,
    memoizedState: null,
    // ...
  }
]

React itself doesn't need this info of course, so maybe adding it purely in the service of developer tooling isn't desirable, but it would enable react-debug-tools to work without having to hard-code the number of inner hooks for composite hooks like deferred value.

acdlite · 2022-04-01T18:31:22Z

Like an auto-incremented ID attribute that could "batch" related primitive hooks.

We could definitely do something like that in dev/profiling at least, not sure if it's worth it for prod

bvaughn · 2022-04-01T18:32:38Z

Like an auto-incremented ID attribute that could "batch" related primitive hooks.

We could definitely do something like that in dev/profiling at least, not if it's worth it for prod

Right, that makes sense. Unfortunately it wouldn't be that helpful if we didn't also do it for production because react-debug-tools is needed for component inspection in DevTools. (It's how we detect the hooks tree and show "custom hooks").

acdlite · 2022-04-01T18:48:26Z

What if React Debug Tools rendered each of built-in hooks by itself (just once during start up, perhaps, or the first time lazily) and counted how many internal hooks it produces?

sebmarkbage · 2022-04-01T18:50:05Z

Seems kind of ok for DevTools to only support uDV in 18.0.1+. Given that it was only in one stable release.

bvaughn · 2022-04-01T18:55:16Z

What if React Debug Tools rendered each of built-in hooks by itself (just once during start up, perhaps, or the first time lazily) and counted how many internal hooks it produces?

That's an interesting idea.

Seems kind of ok for DevTools to only support uDV in 18.0.1+. Given that it was only in one stable release.

Probably okay in this specific case, but it seems likely to come up again with uSES or other composite hooks.

bvaughn · 2022-04-01T19:33:39Z

What if React Debug Tools rendered each of built-in hooks by itself (just once during start up, perhaps, or the first time lazily) and counted how many internal hooks it produces?

That's an interesting idea.

I think this still might not work though, as React Debug Tools needs to know which internal hook stores the canonical value that should be returned to the component (to avoid the component trying to access an invalid property or something).

For example, useSyncEternalStore happens to currently store its "value" in the first internal hook.

I think this happens to be the case for most "composite" hooks at the moment (e.g. useDeferredValue) but ~~what about a hook like useTransition?~~ I guess we could just return a hard-coded false and no-op function in this case. Maybe relying on the value always being stored in the first composed hook for the "composite" cases would be okay but it feels fragile.

packages/react-reconciler/src/ReactFiberHooks.new.js

packages/react-reconciler/src/ReactFiberHooks.old.js

sebmarkbage · 2022-04-11T14:13:27Z

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

+  if (workInProgressRootSpawnedLane === NoLane) {
+    workInProgressRootSpawnedLane = claimNextTransitionLane();
+  }
+  return workInProgressRootSpawnedLane;


Do they need to be the same one? Is that even desirable?

It’s unfortunate that we need another thread-local field. It’s getting to be a lot.

Yeah maybe not, I think I thought maybe they should be batched together because in the previous implementation they were, but I think all that's important is that they don't cause a waterfall which the new implementation already accounts for

Changed it to grab a new one each time

The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders. The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgen render, then we can use the latest value.

DevTools' timeline profiler warns if an update inside a layout effect results in an expensive re-render. However, it misattributes renders that are spawned from a sync render at lower priority. This affects the new implementation of useDeferredValue but it would also apply to things like Offscreen. It's not obvious to me how to fix this given how DevTools models the idea of a "nested update" so I'm disabling the warning for now to unblock the bugfix for useDeferredValue.

…sed to useDeferredValue (#24247) * Fix infinite loop if unmemoized val passed to uDV The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders. The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgen render, then we can use the latest value. * Temporarily disable "long nested update" warning DevTools' timeline profiler warns if an update inside a layout effect results in an expensive re-render. However, it misattributes renders that are spawned from a sync render at lower priority. This affects the new implementation of useDeferredValue but it would also apply to things like Offscreen. It's not obvious to me how to fix this given how DevTools models the idea of a "nested update" so I'm disabling the warning for now to unblock the bugfix for useDeferredValue.

…sed to useDeferredValue (facebook#24247) * Fix infinite loop if unmemoized val passed to uDV The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders. The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgen render, then we can use the latest value. * Temporarily disable "long nested update" warning DevTools' timeline profiler warns if an update inside a layout effect results in an expensive re-render. However, it misattributes renders that are spawned from a sync render at lower priority. This affects the new implementation of useDeferredValue but it would also apply to things like Offscreen. It's not obvious to me how to fix this given how DevTools models the idea of a "nested update" so I'm disabling the warning for now to unblock the bugfix for useDeferredValue.

…sed to useDeferredValue (#24247) * Fix infinite loop if unmemoized val passed to uDV The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders. The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgen render, then we can use the latest value. * Temporarily disable "long nested update" warning DevTools' timeline profiler warns if an update inside a layout effect results in an expensive re-render. However, it misattributes renders that are spawned from a sync render at lower priority. This affects the new implementation of useDeferredValue but it would also apply to things like Offscreen. It's not obvious to me how to fix this given how DevTools models the idea of a "nested update" so I'm disabling the warning for now to unblock the bugfix for useDeferredValue.

…e is passed to useDeferredValue (#24247)" This reverts commit f993ffc.

Summary: This sync includes the following changes: - **[bd4784c8f](facebook/react@bd4784c8f )**: Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) ([#24434](facebook/react#24434)) //<dan>// - **[6d3b6d0f4](facebook/react@6d3b6d0f4 )**: forwardRef et al shouldn't affect if props reused ([#24421](facebook/react#24421)) //<Andrew Clark>// - **[bd0813766](facebook/react@bd0813766 )**: Fix: useDeferredValue should reuse previous value ([#24413](facebook/react#24413)) //<Andrew Clark>// - **[9ae80d6a2](facebook/react@9ae80d6a2 )**: Suppress hydration warnings when a preceding sibling suspends ([#24404](facebook/react#24404)) //<Josh Story>// - **[0dc4e6663](facebook/react@0dc4e6663 )**: Land enableClientRenderFallbackOnHydrationMismatch ([#24410](facebook/react#24410)) //<Andrew Clark>// - **[354772952](facebook/react@354772952 )**: Land enableSelectiveHydration flag ([#24406](facebook/react#24406)) //<Andrew Clark>// - **[392808a1f](facebook/react@392808a1f )**: Land enableClientRenderFallbackOnTextMismatch flag ([#24405](facebook/react#24405)) //<Andrew Clark>// - **[1e748b452](facebook/react@1e748b452 )**: Land enableLazyElements flag ([#24407](facebook/react#24407)) //<Andrew Clark>// - **[4175f0593](facebook/react@4175f0593 )**: Temporarily feature flag numeric fallback for symbols ([#24401](facebook/react#24401)) //<Ricky>// - **[a6d53f346](facebook/react@a6d53f346 )**: Revert "Clean up Selective Hydration / Event Replay flag ([#24156](facebook/react#24156))" ([#24402](facebook/react#24402)) //<Ricky>// - **[ab9cdd34f](facebook/react@ab9cdd34f )**: Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted ([#24400](facebook/react#24400)) //<Andrew Clark>// - **[168da8d55](facebook/react@168da8d55 )**: Fix typo that happened during rebasing //<Andrew Clark>// - **[8bc527a4c](facebook/react@8bc527a4c )**: Bugfix: Fix race condition between interleaved and non-interleaved updates ([#24353](facebook/react#24353)) //<Andrew Clark>// - **[f7cf077cc](facebook/react@f7cf077cc )**: [Transition Tracing] Add Offscreen Queue ([#24341](facebook/react#24341)) //<Luna Ruan>// - **[4fc394bbe](facebook/react@4fc394bbe )**: Fix suspense fallback throttling ([#24253](facebook/react#24253)) //<sunderls>// - **[80170a068](facebook/react@80170a068 )**: Match bundle.name and match upper case entry points ([#24346](facebook/react#24346)) //<Sebastian Markbåge>// - **[fea6f8da6](facebook/react@fea6f8da6 )**: [Transition Tracing] Add transition to OffscreenState and pendingSuspenseBoundaries to RootState ([#24340](facebook/react#24340)) //<Luna Ruan>// - **[8e2f9b086](facebook/react@8e2f9b086 )**: move passive flag ([#24339](facebook/react#24339)) //<Luna Ruan>// - **[55a21ef7e](facebook/react@55a21ef7e )**: fix pushTransition for transition tracing ([#24338](facebook/react#24338)) //<Luna Ruan>// - **[069d23bb7](facebook/react@069d23bb7 )**: [eslint-plugin-exhaustive-deps] Fix exhaustive deps check for unstable vars ([#24343](facebook/react#24343)) //<Afzal Sayed>// - **[4997515b9](facebook/react@4997515b9 )**: Point useSubscription to useSyncExternalStore shim ([#24289](facebook/react#24289)) //<dan>// - **[01e2bff1d](facebook/react@01e2bff1d )**: Remove unnecessary check ([#24332](facebook/react#24332)) //<zhoulixiang>// - **[d9a0f9e20](facebook/react@d9a0f9e20 )**: Delete create-subscription folder ([#24288](facebook/react#24288)) //<dan>// - **[f993ffc51](facebook/react@f993ffc51 )**: Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue ([#24247](facebook/react#24247)) //<Andrew Clark>// - **[fa5800226](facebook/react@fa5800226 )**: [Fizz] Pipeable Stream Perf ([#24291](facebook/react#24291)) //<Josh Story>// - **[0568c0f8c](facebook/react@0568c0f8c )**: Replace zero with NoLanes for consistency in FiberLane ([#24327](facebook/react#24327)) //<Leo>// - **[e0160d50c](facebook/react@e0160d50c )**: add transition tracing transitions stack ([#24321](facebook/react#24321)) //<Luna Ruan>// - **[b0f13e5d3](facebook/react@b0f13e5d3 )**: add pendingPassiveTransitions ([#24320](facebook/react#24320)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 60e63b9...bd4784c jest_e2e[run_all_tests] Reviewed By: kacieb Differential Revision: D35899012 fbshipit-source-id: 86a885e336fca9f0efa80cd2b8ca040f2cb53853

acdlite requested review from sebmarkbage, gaearon and rickhanlonii April 1, 2022 18:00

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 1, 2022

gaearon mentioned this pull request Apr 1, 2022

Bug: useDeferredValue with a value that is new every render hangs browser #19925

Closed

rheh reviewed Apr 3, 2022

View reviewed changes

acdlite force-pushed the fix-infinite-udv-loop branch from dd45378 to ab2d7b6 Compare April 5, 2022 18:07

sebmarkbage approved these changes Apr 11, 2022

View reviewed changes

acdlite added 2 commits April 11, 2022 11:03

acdlite force-pushed the fix-infinite-udv-loop branch from ab2d7b6 to e4fd270 Compare April 11, 2022 15:04

acdlite merged commit f993ffc into facebook:main Apr 11, 2022

rickhanlonii added a commit that referenced this pull request Apr 20, 2022

Revert "Fix infinite update loop that happens when an unmemoized valu…

f47e471

…e is passed to useDeferredValue (#24247)" This reverts commit f993ffc.

gaearon mentioned this pull request May 27, 2022

[DevTools Bug] When inspecting, hook values after useDeferredValue are offset #24623

Closed

mondaychen mentioned this pull request Jun 16, 2022

[DevTools] fix useDeferredValue to match reconciler change #24742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue #24247

Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue #24247

acdlite commented Apr 1, 2022

acdlite commented Apr 1, 2022

sizebot commented Apr 1, 2022 •

edited

Loading

bvaughn commented Apr 1, 2022 •

edited

Loading

acdlite commented Apr 1, 2022 •

edited

Loading

bvaughn commented Apr 1, 2022

acdlite commented Apr 1, 2022

sebmarkbage commented Apr 1, 2022

bvaughn commented Apr 1, 2022

bvaughn commented Apr 1, 2022 •

edited

Loading

sebmarkbage Apr 11, 2022

acdlite Apr 11, 2022

acdlite Apr 11, 2022

Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue #24247

Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue #24247

Conversation

acdlite commented Apr 1, 2022

acdlite commented Apr 1, 2022

sizebot commented Apr 1, 2022 • edited Loading

Critical size changes

Significant size changes

bvaughn commented Apr 1, 2022 • edited Loading

acdlite commented Apr 1, 2022 • edited Loading

bvaughn commented Apr 1, 2022

acdlite commented Apr 1, 2022

sebmarkbage commented Apr 1, 2022

bvaughn commented Apr 1, 2022

bvaughn commented Apr 1, 2022 • edited Loading

sebmarkbage Apr 11, 2022

Choose a reason for hiding this comment

acdlite Apr 11, 2022

Choose a reason for hiding this comment

acdlite Apr 11, 2022

Choose a reason for hiding this comment

sizebot commented Apr 1, 2022 •

edited

Loading

bvaughn commented Apr 1, 2022 •

edited

Loading

acdlite commented Apr 1, 2022 •

edited

Loading

bvaughn commented Apr 1, 2022 •

edited

Loading