Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
eps1lon
commented
Aug 29, 2024
| this.destination = null; | ||
| this.bundlerConfig = bundlerConfig; | ||
| this.cache = new Map(); | ||
| this.cache = new WeakMap(); |
Collaborator
Author
There was a problem hiding this comment.
I might have missed some instances since Flow didn't complain here since we silence errors later. Though I assume we only init this once when we create the request and not reassign it later?
eps1lon
force-pushed
the
cache-indirection
branch
from
df76d94 to
3d293d8
Compare
Allows us to get rid of an indiretion in the Client implementation.
eps1lon
force-pushed
the
cache-indirection
branch
from
3d293d8 to
649c219
Compare
|
Orb Code Review (powered by GLM 5.1 on Orb Cloud) SummaryExcellent cache optimization that simplifies React's cache architecture by eliminating nested WeakMaps and reducing indirection. Improves both performance and memory efficiency. Architecture Improvement ✅Before: Complex Nested Cachingfunction getCacheForType<T>(resourceType: () => T): T {
const cache: Cache = readContext(CacheContext);
let cacheForType: T | void = cache.data.get(resourceType);
if (cacheForType === undefined) {
cacheForType = resourceType(); // Creates new WeakMap per type
cache.data.set(resourceType, cacheForType);
}
return cacheForType;
}
// Usage creates factory functions and nested WeakMaps
const fnMap = dispatcher.getCacheForType(createCacheRoot);After: Direct Cache Accessfunction getActiveCache(): AsyncCache {
const cache: Cache = readContext(CacheContext);
return cache.data; // Direct access to the main WeakMap
}
// Usage is direct and efficient
const activeCache = dispatcher.getActiveCache();Performance Benefits ✅1. Eliminates Factory Functions
2. Reduces Memory Footprint
3. Simplifies Access Pattern
Code Quality ✅API Simplification// Cleaner API surface
export type AsyncDispatcher = {
-getCacheForType<T>(resourceType: () => T): T,
+getActiveCache(): AsyncCache,
};Type Safety Maintained
Cross-package Impact ✅Consistent ImplementationChanges applied across all React packages:
Backwards Compatibility
Cache Pattern Analysis ✅WeakMap Usage// Optimal cache pattern
const activeCache = dispatcher.getActiveCache();
const fnNode: CacheNode<T> | void = (activeCache.get(fn): any);
if (fnNode === undefined) {
cacheNode = createCacheNode();
activeCache.set(fn, cacheNode); // Direct WeakMap operation
}
Testing Coverage ✅Updated test suites:
Assessment: ✅ APPROVEThis is a well-executed performance optimization that demonstrates excellent architectural refinement:
Recommendation: Merge immediately. This optimization improves React's cache performance without any downsides or breaking changes. Impact: Applications using React's cache API will benefit from reduced memory usage and faster cache operations, particularly in scenarios with frequent cache access. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Stacked on #30736
Summary
Instead of using
Map.get(createCacheRoot).get(fn)we can just use rely onAsyncCachebeing aWeakMap.How did you test this change?