Try: Improve caching for getBlockContext #46797
Conversation
Mamaduka
added
[Type] Performance
|
Size Change: +13 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
Mamaduka
force-pushed
the
try/improve-block-context-cache
branch
2 times, most recently
from
d7a5354
to
9ed8a11
Compare
Mamaduka
force-pushed
the
try/improve-block-context-cache
branch
from
9ed8a11
to
dc28945
Compare
|
Hey, @Mamaduka FYI I'm on my annual support rotation this week, but this is on my radar and I'll review it ASAP next week when I'm back to my regular workday. |
There was a problem hiding this comment.
The patch is OK, but personally I would write the whole function differently 🙂 To avoid the ever-growing and global cache object, I would convert it to a useBlockContext hook and memoize the return value locally, in a ref. And instead of potentially expensive JSON.stringify of the entire context object, use isShallowEqual:
function useBlockContext( attributes, blockType ) {
const memoContext = useRef( undefined );
const newContext = computeContext( attributes, blockType );
if ( ! isShallowEqual( memoContext.current, newContext ) ) {
memoContext.current = newContext;
}
return memoContext.current;
}This achieves the same purpose: if the attributes object changes, but not in a way that's relevant to the context (the subset of attributes copied to the context didn't change), return an old memoized value of the context.
Also, the condition that checks if blockType.providesContext is non-empty, it can be incorporated into the computeContext code, causing context to become undefined. The callers of useBlockContext then don't have to do it themselves, as they currently do.
|
Thanks for the review, @jsnajdr 🙇 Considering that we cannot call But I like the idea of replacing the global cache with a local one and using |
You're right, although we can move the
If you want both local cache and |
I thought about this as well. But then read through the original PR, and I was under the impression that most APIs were intentionally left private. See https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-context/README.md. Introducing the new public selector probably goes against this original decision. What do you think? P.S. I'm mostly AFK this week, but I will be happy to make the requested changes when I get back. |
| expect( prevContext ).toBe( nextContext ); | ||
| } ); | ||
|
|
||
| it( 'should return cached value if attributes used in context are the same', () => { |
There was a problem hiding this comment.
Should we also add a test for the case when there's no providesContext?
There was a problem hiding this comment.
The blockType object is a required argument, so I'm not sure how helpful that test would be.
Looking at the PR and the docs, it seems to me that the motivation to keep the APIs private was to encourage block authors to use the On one hand, It's true that a new On the other hand, For these reasons, I wouldn't worry about adding a new public |
|
No matter how we decide about the |
|
@jsnajdr, I just realized that if we use block attributes in deps, we'll have the same issue with unwanted cache invalidation. This is the memoized selector structure I've in mind, but I might be missing something. export const getBlockContext = createSelector(
( state, clientId ) => {
// ...
},
( state, clientId ) => [
state.blocks.byClientId.get( clientId ),
state.blocks.attributes.get( clientId ),
]
); |
|
You'd have to write you custom memoization code that compares the However, here's an idea how to implement the correct memoization using only function useBlockContext( clientId ) {
return useSelect( ( select ) => {
const block = select( blockEditorStore ).getBlock( clientId );
const blockType = getBlockType( block.name );
return mapValues( blockType.providesContext, attr => block.attributes[ attr ] );
}, [ clientId ] );
};This returns the context object directly from |
|
That's perfect. Thank you, @jsnajdr! I will follow up with the new PR later today. |
|
I started working on the I still need to find a way to write good unit tests for it a migrate the |
|
Closed in favor of #47028. |
What?
PR tries to improve the caching logic inside the
getBlockContextmethod.Why?
Currently, every attribute update causes the
BlockContextProvidervalue to change. Providers and consumers should only care when attributes provided as context are updated.How?
I've updated the function to use
cacheKeygenerated by the context object andMapfor storing the values.Testing Instructions
Running the same test on the trunk will fail.