Initial implementation of cache cleanup #22510
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
josephsavona
changed the title
josephsavona
commented
Oct 5, 2021
josephsavona
commented
Oct 5, 2021
acdlite
reviewed
Oct 6, 2021
acdlite
reviewed
Oct 6, 2021
josephsavona
commented
Oct 7, 2021
acdlite
reviewed
Oct 8, 2021
acdlite
reviewed
Oct 8, 2021
acdlite
reviewed
Oct 8, 2021
josephsavona
commented
Oct 8, 2021
josephsavona
commented
Oct 8, 2021
josephsavona
commented
Oct 8, 2021
| @@ -1877,8 +1880,12 @@ function commitRootImpl(root, renderPriorityLevel) { | |||
| ) { | |||
| if (!rootDoesHavePassiveEffects) { | |||
| rootDoesHavePassiveEffects = true; | |||
| pendingPassiveEffectsRemainingLanes = remainingLanes; | |||
There was a problem hiding this comment.
we need to store this to know which lanes were remaining when flushPassiveEffects() runs asynchronously
josephsavona
commented
Oct 8, 2021
|
Okay @acdlite this is ready for another review pass! |
josephsavona
commented
Oct 12, 2021
packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js
Outdated
Show resolved
Hide resolved
josephsavona
changed the title
acdlite
reviewed
Oct 19, 2021
acdlite
reviewed
Oct 19, 2021
acdlite
reviewed
Oct 19, 2021
acdlite
reviewed
Oct 19, 2021
josephsavona
commented
Oct 20, 2021
|
Comparing: bfb4022...dcf04a4 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
josephsavona
force-pushed
the
cache-cleanup-2
branch
from
db7944f
to
4ba534c
Compare
acdlite
approved these changes
Oct 21, 2021
josephsavona
force-pushed
the
cache-cleanup-2
branch
from
f2a9fc7
to
6fae379
Compare
This comment has been minimized.
This comment has been minimized.
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Nov 2, 2021
Summary: This sync includes the following changes: - **[5cccacd13](facebook/react@5cccacd13 )**: Upgrade useId to alpha channel ([#22674](facebook/react#22674)) //<Andrew Clark>// - **[75f3ddebf](facebook/react@75f3ddebf )**: Remove experimental useOpaqueIdentifier API ([#22672](facebook/react#22672)) //<Andrew Clark>// - **[8c4a05b8f](facebook/react@8c4a05b8f )**: Remove flow pragma comment from module registration start/stop templates ([#22670](facebook/react#22670)) //<Brian Vaughn>// - **[ebf9ae857](facebook/react@ebf9ae857 )**: useId ([#22644](facebook/react#22644)) //<Andrew Clark>// - **[a0d991fe6](facebook/react@a0d991fe6 )**: Re-land #22292 (remove uMS from open source build) ([#22664](facebook/react#22664)) //<Andrew Clark>// - **[6bce0355c](facebook/react@6bce0355c )**: Upgrade useSyncExternalStore to alpha channel ([#22662](facebook/react#22662)) //<Andrew Clark>// - **[7034408ff](facebook/react@7034408ff )**: Follow-up improvements to error code extraction infra ([#22516](facebook/react#22516)) //<Andrew Clark>// - **[90e5d3638](facebook/react@90e5d3638 )**: chore: fix comment typo ([#22615](facebook/react#22615)) //<btea>// - **[3c4c1c470](facebook/react@3c4c1c470 )**: Remove warning for dangling passive effects ([#22609](facebook/react#22609)) //<Andrew Clark>// - **[d5b6b4b86](facebook/react@d5b6b4b86 )**: Expand act warning to cover all APIs that might schedule React work ([#22607](facebook/react#22607)) //<Andrew Clark>// - **[fa9bea0c4](facebook/react@fa9bea0c4 )**: Initial implementation of cache cleanup ([#22510](facebook/react#22510)) //<Joseph Savona>// - **[0e8a5aff3](facebook/react@0e8a5aff3 )**: Scheduling Profiler: Add marks for component effects (mount and unmount) ([#22578](facebook/react#22578)) //<Brian Vaughn>// - **[4ba20579d](facebook/react@4ba20579d )**: Scheduling Profiler: De-emphasize React internal frames ([#22588](facebook/react#22588)) //<Brian Vaughn>// - **[cdb8a1d19](facebook/react@cdb8a1d19 )**: [Fizz] Add option to inject bootstrapping script tags after the shell is injected ([#22594](facebook/react#22594)) //<Sebastian Markbåge>// - **[34e4c9756](facebook/react@34e4c9756 )**: Clear extra nodes if there's a hydration mismatch within a suspense boundary ([#22592](facebook/react#22592)) //<Sebastian Markbåge>// - **[02f411578](facebook/react@02f411578 )**: Upgrade useInsertionEffect to stable ([#22589](facebook/react#22589)) //<Andrew Clark>// - **[2af4a7933](facebook/react@2af4a7933 )**: Hydrate using SuspenseComponent as the parent ([#22582](facebook/react#22582)) //<Sebastian Markbåge>// - **[b1acff0cc](facebook/react@b1acff0cc )**: Enable cache in test renderer ([#22580](facebook/react#22580)) //<Joseph Savona>// - **[996da67b2](facebook/react@996da67b2 )**: Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` ([#22562](facebook/react#22562)) //<Andrew Clark>// - **[163e81c1f](facebook/react@163e81c1f )**: Support disabling spurious act warnings with a global environment flag ([#22561](facebook/react#22561)) //<Andrew Clark>// - **[23b7dfeff](facebook/react@23b7dfeff )**: Enable scheduling profiler for RN FB profiling builds ([#22566](facebook/react#22566)) //<Brian Vaughn>// - **[61455a25b](facebook/react@61455a25b )**: Enable experimental Cache API in www TestRenderer ([#22554](facebook/react#22554)) //<Joseph Savona>// - **[7142d110b](facebook/react@7142d110b )**: Bugfix: Nested useOpaqueIdentifier references ([#22553](facebook/react#22553)) //<Andrew Clark>// - **[1e247ff89](facebook/react@1e247ff89 )**: Enabled scheduling profiler marks for React Native FB target ([#22544](facebook/react#22544)) //<Brian Vaughn>// - **[c16b005f2](facebook/react@c16b005f2 )**: Update test and stack frame code to support newer V8 stack formats ([#22477](facebook/react#22477)) //<Brian Vaughn>// - **[55d75005b](facebook/react@55d75005b )**: duplicate value in variable ([#22390](facebook/react#22390)) //<BIKI DAS>// Changelog: [General][Changed] - React Native sync for revisions afcb9cd...3fcd81d jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D32065987 fbshipit-source-id: ef2d402835c981aab68ca40a894c66c1630864e9
KamranAsif
pushed a commit
to KamranAsif/react
that referenced
this pull request
Nov 4, 2021
This is an initial, partial implementation of a cleanup mechanism for the experimental Cache API. The idea is that consumers of the Cache API can register to be informed when a given Cache instance is no longer needed so that they can perform associated cleanup tasks to free resources stored in the cache. A canonical example would be cancelling pending network requests. An overview of the high-level changes: * Changes the `Cache` type from a Map of cache instances to be an object with the original Map of instances, a reference count (to count roughly "active references" to the cache instances - more below), and an AbortController. * Adds a new public API, `unstable_getCacheSignal(): AbortSignal`, which is callable during render. It returns an AbortSignal tied to the lifetime of the cache - developers can listen for the 'abort' event on the signal, which React now triggers when a given cache instance is no longer referenced. * Note that `AbortSignal` is a web standard that is supported by other platform APIs; for example a signal can be passed to `fetch()` to trigger cancellation of an HTTP request. * Implements the above - triggering the 'abort' event - by handling passive mount/unmount for HostRoot and CacheComponent fiber nodes. Cases handled: * Aborted transitions: we clean up a new cache created for an aborted transition * Suspense: we retain a fresh cache instance until a suspended tree resolves For follow-ups: * When a subsequent cache refresh is issued before a previous refresh completes, the refreshes are queued. Fresh cache instances for previous refreshes in the queue should be cleared, retaining only the most recent cache. I plan to address this in a follow-up PR. * If a refresh is cancelled, the fresh cache should be cleaned up.
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Nov 15, 2021
Summary: This sync includes the following changes: - **[c0c71a868](facebook/react@c0c71a868 )**: Re-enable useMutableSource in internal RN ([#22750](facebook/react#22750)) //<Ricky>// - **[cb11155c8](facebook/react@cb11155c8 )**: Add runtime type checks around module boundary code ([#22748](facebook/react#22748)) //<Brian Vaughn>// - **[a04f13d29](facebook/react@a04f13d29 )**: react-refresh@0.11.0 //<Dan Abramov>// - **[ff9897d23](facebook/react@ff9897d23 )**: [React Refresh] support typescript namespace syntax ([#22621](facebook/react#22621)) //<irinakk>// - **[0ddd69d12](facebook/react@0ddd69d12 )**: Throw on hydration mismatch and force client rendering if boundary hasn't suspended within concurrent root ([#22629](facebook/react#22629)) //<salazarm>// - **[c3f34e4be](facebook/react@c3f34e4be )**: eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>// - **[827021c4e](facebook/react@827021c4e )**: Changelog for eslint-plugin-react-hooks@4.3.0 //<Dan Abramov>// - **[8ca3f567b](facebook/react@8ca3f567b )**: Fix module-boundary wrappers ([#22688](facebook/react#22688)) //<Brian Vaughn>// - **[1bf6deb86](facebook/react@1bf6deb86 )**: Renamed packages/react-devtools-scheduling-profiler to packages/react-devtools-timeline ([#22691](facebook/react#22691)) //<Brian Vaughn>// - **[51c558aeb](facebook/react@51c558aeb )**: Rename (some) "scheduling profiler" references to "timeline" ([#22690](facebook/react#22690)) //<Brian Vaughn>// - **[00ced1e2b](facebook/react@00ced1e2b )**: Fix useId in strict mode ([#22681](facebook/react#22681)) //<Andrew Clark>// - **[5cccacd13](facebook/react@5cccacd13 )**: Upgrade useId to alpha channel ([#22674](facebook/react#22674)) //<Andrew Clark>// - **[75f3ddebf](facebook/react@75f3ddebf )**: Remove experimental useOpaqueIdentifier API ([#22672](facebook/react#22672)) //<Andrew Clark>// - **[8c4a05b8f](facebook/react@8c4a05b8f )**: Remove flow pragma comment from module registration start/stop templates ([#22670](facebook/react#22670)) //<Brian Vaughn>// - **[ebf9ae857](facebook/react@ebf9ae857 )**: useId ([#22644](facebook/react#22644)) //<Andrew Clark>// - **[a0d991fe6](facebook/react@a0d991fe6 )**: Re-land #22292 (remove uMS from open source build) ([#22664](facebook/react#22664)) //<Andrew Clark>// - **[6bce0355c](facebook/react@6bce0355c )**: Upgrade useSyncExternalStore to alpha channel ([#22662](facebook/react#22662)) //<Andrew Clark>// - **[7034408ff](facebook/react@7034408ff )**: Follow-up improvements to error code extraction infra ([#22516](facebook/react#22516)) //<Andrew Clark>// - **[90e5d3638](facebook/react@90e5d3638 )**: chore: fix comment typo ([#22615](facebook/react#22615)) //<btea>// - **[3c4c1c470](facebook/react@3c4c1c470 )**: Remove warning for dangling passive effects ([#22609](facebook/react#22609)) //<Andrew Clark>// - **[d5b6b4b86](facebook/react@d5b6b4b86 )**: Expand act warning to cover all APIs that might schedule React work ([#22607](facebook/react#22607)) //<Andrew Clark>// - **[fa9bea0c4](facebook/react@fa9bea0c4 )**: Initial implementation of cache cleanup ([#22510](facebook/react#22510)) //<Joseph Savona>// - **[0e8a5aff3](facebook/react@0e8a5aff3 )**: Scheduling Profiler: Add marks for component effects (mount and unmount) ([#22578](facebook/react#22578)) //<Brian Vaughn>// - **[4ba20579d](facebook/react@4ba20579d )**: Scheduling Profiler: De-emphasize React internal frames ([#22588](facebook/react#22588)) //<Brian Vaughn>// - **[cdb8a1d19](facebook/react@cdb8a1d19 )**: [Fizz] Add option to inject bootstrapping script tags after the shell is injected ([#22594](facebook/react#22594)) //<Sebastian Markbåge>// - **[34e4c9756](facebook/react@34e4c9756 )**: Clear extra nodes if there's a hydration mismatch within a suspense boundary ([#22592](facebook/react#22592)) //<Sebastian Markbåge>// - **[02f411578](facebook/react@02f411578 )**: Upgrade useInsertionEffect to stable ([#22589](facebook/react#22589)) //<Andrew Clark>// - **[2af4a7933](facebook/react@2af4a7933 )**: Hydrate using SuspenseComponent as the parent ([#22582](facebook/react#22582)) //<Sebastian Markbåge>// - **[b1acff0cc](facebook/react@b1acff0cc )**: Enable cache in test renderer ([#22580](facebook/react#22580)) //<Joseph Savona>// - **[996da67b2](facebook/react@996da67b2 )**: Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` ([#22562](facebook/react#22562)) //<Andrew Clark>// - **[163e81c1f](facebook/react@163e81c1f )**: Support disabling spurious act warnings with a global environment flag ([#22561](facebook/react#22561)) //<Andrew Clark>// - **[23b7dfeff](facebook/react@23b7dfeff )**: Enable scheduling profiler for RN FB profiling builds ([#22566](facebook/react#22566)) //<Brian Vaughn>// - **[61455a25b](facebook/react@61455a25b )**: Enable experimental Cache API in www TestRenderer ([#22554](facebook/react#22554)) //<Joseph Savona>// - **[7142d110b](facebook/react@7142d110b )**: Bugfix: Nested useOpaqueIdentifier references ([#22553](facebook/react#22553)) //<Andrew Clark>// - **[1e247ff89](facebook/react@1e247ff89 )**: Enabled scheduling profiler marks for React Native FB target ([#22544](facebook/react#22544)) //<Brian Vaughn>// - **[c16b005f2](facebook/react@c16b005f2 )**: Update test and stack frame code to support newer V8 stack formats ([#22477](facebook/react#22477)) //<Brian Vaughn>// - **[55d75005b](facebook/react@55d75005b )**: duplicate value in variable ([#22390](facebook/react#22390)) //<BIKI DAS>// Changelog: [General][Changed] - React Native sync for revisions afcb9cd...c0c71a8 jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D32395873 fbshipit-source-id: 3afd158f167b1eedcc244e29aba1a2c502d3c9d9
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
Apr 15, 2022
This is an initial, partial implementation of a cleanup mechanism for the experimental Cache API. The idea is that consumers of the Cache API can register to be informed when a given Cache instance is no longer needed so that they can perform associated cleanup tasks to free resources stored in the cache. A canonical example would be cancelling pending network requests. An overview of the high-level changes: * Changes the `Cache` type from a Map of cache instances to be an object with the original Map of instances, a reference count (to count roughly "active references" to the cache instances - more below), and an AbortController. * Adds a new public API, `unstable_getCacheSignal(): AbortSignal`, which is callable during render. It returns an AbortSignal tied to the lifetime of the cache - developers can listen for the 'abort' event on the signal, which React now triggers when a given cache instance is no longer referenced. * Note that `AbortSignal` is a web standard that is supported by other platform APIs; for example a signal can be passed to `fetch()` to trigger cancellation of an HTTP request. * Implements the above - triggering the 'abort' event - by handling passive mount/unmount for HostRoot and CacheComponent fiber nodes. Cases handled: * Aborted transitions: we clean up a new cache created for an aborted transition * Suspense: we retain a fresh cache instance until a suspended tree resolves For follow-ups: * When a subsequent cache refresh is issued before a previous refresh completes, the refreshes are queued. Fresh cache instances for previous refreshes in the queue should be cleared, retaining only the most recent cache. I plan to address this in a follow-up PR. * If a refresh is cancelled, the fresh cache should be cleaned up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is an initial, partial implementation of a cleanup mechanism for the experimental Cache API. The idea is that consumers of the Cache API can register to be informed when a given Cache instance is no longer needed so that they can perform associated cleanup tasks to free resources stored in the cache. A canonical example would be cancelling pending network requests.
An overview of the high-level changes:
Cachetype from a Map of cache instances to be an object with the original Map of instances, a reference count (to count roughly "active references" to the cache instances - more below), and an AbortController.unstable_getCacheSignal(): AbortSignal, which is callable during render. It returns an AbortSignal tied to the lifetime of the cache - developers can listen for the 'abort' event on the signal, which React now triggers when a given cache instance is no longer referenced.AbortSignalis a web standard that is supported by other platform APIs; for example a signal can be passed tofetch()to trigger cancellation of an HTTP request.Cases handled:
For follow-ups:
How did you test this change?
yarn test --all- a few tests are still failing and i'll need some guidance on how to resolve those (i'll post more details later).