Add useSettings hook for reading multiple settings at once #55337
Conversation
jsnajdr
added
[Type] Performance
There was a problem hiding this comment.
For this particular function, my goal was to actually move its call to BlockEdit or something and add it to the context of a block (probably as a private thing), that way it's only called once per block (as opposed to one per hook like today).
|
Size Change: +198 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 64b12aee38f1f1fcb77f9015a8c1c91f66dfdeb6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6569868365
|
jsnajdr
force-pushed
the
optimize/use-setting
branch
from
c029303 to
21134f9
Compare
|
I removed the commit with debugging code to make the CI check pass. You can manually apply it with |
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Nice work 🥇
Thanks for diving deeper into the large-post typing performance issues. 👍
If useSettings is preferred for multiple settings, I think we should ensure to educate folks about it (adding some additional explanation in docs) and maybe introduce an ESLint rule that prevents sibling useSetting() calls?
| */ | ||
| export const getValueFromObjectPath = ( object, path, defaultValue ) => { | ||
| const normalizedPath = Array.isArray( path ) ? path : path.split( '.' ); | ||
| const normalizedPath = Array.isArray( path ) ? path : stringToPath( path ); |
There was a problem hiding this comment.
This can totally be shipped as a separate PR regardless of how we decide to move forward.
| * ```js | ||
| * const isEnabled = useSetting( 'typography.dropCap' ); | ||
| * ``` | ||
| * @param {string[]} paths The path to the setting. |
There was a problem hiding this comment.
| * @param {string[]} paths The path to the setting. | |
| * @param {string[]} paths The paths to the settings. |
| } | ||
|
|
||
| /** | ||
| * Hook that retrieves the given setting for the block instance in use. |
There was a problem hiding this comment.
| * Hook that retrieves the given setting for the block instance in use. | |
| * Hook that retrieves the given settings for the block instance in use. |
| * Hook that retrieves the given setting for the block instance in use. | ||
| * | ||
| * It looks up the settings first in the block instance hierarchy. | ||
| * If none is found, it'll look it up in the block editor store. |
There was a problem hiding this comment.
| * If none is found, it'll look it up in the block editor store. | |
| * If none are found, it'll look them up in the block editor store. |
There was a problem hiding this comment.
Pushed a commit that fixes all the typos in the docblocks.
| */ | ||
| export default function useSetting( path ) { | ||
| const { name: blockName, clientId } = useBlockEditContext(); | ||
| export function useSettings( paths ) { |
There was a problem hiding this comment.
Does it make sense to move useSettings() to a separate file? Perhaps under ../use-settings/index.js? Right now it's a bit unfortunate that use-setting/index.js exports useSettings().
There was a problem hiding this comment.
The hooks are very closely related, it's not justified to have them in separate files.
Most consumers should import them from the parent components module, not using the /use-setting subpath.
There was a problem hiding this comment.
I disagree because I still find it confusing and unexpected, but I'll agree to disagree and commit.
packages/block-editor/README.md
Outdated
| Hook that retrieves the given setting for the block instance in use. | ||
|
|
||
| It looks up the settings first in the block instance hierarchy. If none is found, it'll look it up in the block editor store. | ||
|
|
||
| _Parameters_ | ||
|
|
||
| - _paths_ `string[]`: The path to the setting. | ||
|
|
||
| _Returns_ | ||
|
|
||
| - `any[]`: Returns the values defined for the settings. |
There was a problem hiding this comment.
| Hook that retrieves the given setting for the block instance in use. | |
| It looks up the settings first in the block instance hierarchy. If none is found, it'll look it up in the block editor store. | |
| _Parameters_ | |
| - _paths_ `string[]`: The path to the setting. | |
| _Returns_ | |
| - `any[]`: Returns the values defined for the settings. | |
| Hook that retrieves the given settings for the block instance in use. | |
| It looks up the settings first in the block instance hierarchy. If none are found, it'll look them up in the block editor store. | |
| _Parameters_ | |
| - _paths_ `string[]`: The paths to the settings. | |
| _Returns_ | |
| - `any[]`: Returns the values defined for the settings. |
| function FontSizePicker( props ) { | ||
| const fontSizes = useSetting( 'typography.fontSizes' ); | ||
| const disableCustomFontSizes = ! useSetting( 'typography.customFontSize' ); | ||
| const [ fontSizes, customFontSizes ] = useSettings( [ |
There was a problem hiding this comment.
Maybe we should keep the same name as the subsetting name:
| const [ fontSizes, customFontSizes ] = useSettings( [ | |
| const [ fontSizes, customFontSize ] = useSettings( [ |
There was a problem hiding this comment.
Here I chose the variable name based on the name of the setting: customFontSize.
There was a problem hiding this comment.
I may be missing something, but that's exactly why I added the suggestion - in the PR we're using customFontSizes and not customFontSize.
There was a problem hiding this comment.
Ah, I see 🙂 Changed as suggested.
| { ...props } | ||
| fontSizes={ fontSizes } | ||
| disableCustomFontSizes={ disableCustomFontSizes } | ||
| disableCustomFontSizes={ ! customFontSizes } |
There was a problem hiding this comment.
| disableCustomFontSizes={ ! customFontSizes } | |
| disableCustomFontSizes={ ! customFontSize } |
| isTextEnabled, | ||
| isHeadingEnabled, | ||
| isButtonEnabled, | ||
| ] = useSettings( [ |
There was a problem hiding this comment.
I think it could be nice to consider building an ESLint rule that prevents folks from having multiple useSetting calls at the same level, recommending that we use useSettings() instead.
Not for this PR, obviously, but something that we discussed earlier and I wanted to make sure it doesn't fall through the cracks.
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
very nice update here, and one that is probably overdue. thanks for addressing this - the reduction in hook count is great.
| export function useSettings( paths ) { | ||
| const { name: blockName, clientId = null } = useBlockEditContext(); | ||
|
|
||
| paths = useMemo( () => paths, paths ); |
There was a problem hiding this comment.
what's the purpose of this useMemo? am I mistaken in that it's doing nothing since all the internal function does is return its stored value?
There was a problem hiding this comment.
On each useSettings call, the paths array will be different, although it contains the same values. This useMemo is to ensure that paths is a constant in that case, and to ensure that useSelect can memoize its return value. useSelect itself does something similar internally.
There was a problem hiding this comment.
aha! the second value is the dependencies array, which in this case is paths itself. in my head I was reading [paths]
| mergeCache.set( value, result ); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
it would be nice to have a docblock-style comment explaining the purpose of this function, and I wonder if the name could be clearer since mergeOrigins doesn't outwardly communicate that there's a cache involved.
I'm also confused on the value we're storing here, or why we need flatMap(). In the old code it's not clear to me either, so maybe this is why 🤷♂️ but it looks like it might be using .concat() with a default of [] to avoid needing a conditional for the case where result is missing the necessary key. I guess it's convenient for when keys are undefined and then it automatically returns an array with only those values.
but the old code was directly returning these things too while here we're storing them. since these are retrieved from value is there a need to split them out at this point? did you find that extracting these three properties was showing up in the hot path?
I'm so confused on all of this, really. is it worth adding a separate function and cache vs. directly building the array every time? maybe it was all confused by the nature of the original use of reduce()? I'm not trying to argue that the cache isn't worth it; I'm only asking if we have reason to believe it is.
const values = [];
[ 'default', 'theme', 'custom' ].forEach(
( key ) => {
if ( value[ key ] ) {
values.push( value[ key ] );
}
}
)
return values;There was a problem hiding this comment.
Some settings, like color.palette, have a "raw" value where settings from different "origins" are in separate arrays:
{
default: [ ... ],
theme: [ ... ],
custom: [ ... ],
}The mergeOrigins function merges the three arrays into one, concatenating them together. I added memoization so that two calls to useSelect with the same state and arguments return the same value. This was a little pre-existing bug.
I added a docblock to the function that contains this explanation.
| */ | ||
| export default function useSetting( path ) { | ||
| const { name: blockName, clientId } = useBlockEditContext(); | ||
| export function useSettings( paths ) { |
There was a problem hiding this comment.
did you consider using variable argument counts here? it seems like most cases we'd be calling this we'll have literal values vs. dynamically-created arrays and I wonder if that's a convenience that would be nice.
const [ … ] = useSettings( 'setting-a', 'setting-b', 'color.setting.thing' );There was a problem hiding this comment.
Yes, that's a good idea. Variable arguments are better than an array argument. This is quite related to @tyxla's feedback about the module name:
Does it make sense to move
useSettings()to a separate file? Perhaps under../use-settings/index.js? Right now it's a bit unfortunate thatuse-setting/index.jsexportsuseSettings().
To solve both problems, we could do this:
- Rewrite
useSettingsto use variable arguments. - Migrate all remaining usages of
useSettingtouseSettings. The entire difference is the return value as array:const [ a ] = useSettings( 'a' )vsconst a = useSetting( 'a' ). - This makes
useSettingpractically deprecated: there's no good reason any more to use it instead ofuseSettings. - The actual source file can then be renamed to
use-settings/index.js. The main thing it exports isuseSettings, and then there's also the littleuseSettingback-compat wrapper. I think that solves the naming concerns very well.
Thoughts?
There was a problem hiding this comment.
I really like that idea 👍 one fewer new API while still supporting all original use cases. Any good reason not to go that way?
There was a problem hiding this comment.
Implemented! All usages of useSetting are refactored to useSettings, and the source folder name is now use-settings.
jsnajdr
force-pushed
the
optimize/use-setting
branch
from
c79b9f3 to
f337392
Compare
jsnajdr
requested review from
ajitbohra,
artemiomorales,
dcalhoun,
fluiddot and
michalczaplinski
as code owners
There was a problem hiding this comment.
@tellthemachines This is a change that could be a separate PR. I found that the useSetting( 'layout' ) call can be completely removed, because the usedLayout value is no longer used. Only its .type field is used. And the type can be calculated without knowing defaultLayout. That value always comes from the layout object.
usedLayout was used until #47477 before usages of __experimentalLayout, but today it's redundant.
There was a problem hiding this comment.
Ah well spotted! Yes, this can be removed. Not too fussed about whether it's done here or in a separate PR, your choice 😄
|
@jsnajdr a couple questions on deprecation, you may have addressed and I may have missed.
I wasn't sure if any imports of that being said, if we want to move away from |
I wasn't sure if I want to really formally deprecate
I think by now the names of the folders and files are sufficiently fine. The main thing that
This is just the internal layout of the |
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
This looks great 🚀 Nice optimization!
I've left a few suggestions, but it's minor stuff and nothing really feels blocking.
I'm not particularly convinced that we should deprecate useSetting(), I'd rather expect that we discourage sibling useSetting() calls and encourage using useSettings() in those cases, but this is something we could explore separately.
packages/block-editor/CHANGELOG.md
Outdated
There was a problem hiding this comment.
| - Deprecated the `useSetting` function in favor of new `useSettings` one that can retrieve multiple settings at once. | |
| - Deprecated the `useSetting` function in favor of new `useSettings` one that can retrieve multiple settings at once ([#55337](https://github.com/WordPress/gutenberg/pull/55337)). |
packages/block-editor/README.md
Outdated
There was a problem hiding this comment.
Should we add a note right here that it's deprecated in favor of useSettings()?
There was a problem hiding this comment.
Should we keep the EMPTY_OBJECT fallback here?
There was a problem hiding this comment.
That's not needed because defaultLayout is used only with an object spread operator, ...defaultLayout, which treats undefined or null as an empty object already.
There was a problem hiding this comment.
Would be nice to avoid the mutation here.
There was a problem hiding this comment.
I think that local mutation is fine, it's not worse in any way than creating a new array and concatenating existing arrays into it with the spread operator.
There was a problem hiding this comment.
There's no need to introduce a bulk settings filter right?
There was a problem hiding this comment.
Certainly not, it would be counterproductive. If you want to modify a setting with a plugin, you want to register only one filter, not two of them, both useSetting and useSettings.
There was a problem hiding this comment.
I'd keep a test for useSetting() too, even if it's just a single one.
There was a problem hiding this comment.
Added a test for useSetting 👍
There was a problem hiding this comment.
Not from this PR, but this appears to be so repetitive! I wonder if we should abstract it to a separate hook with these default args.
There was a problem hiding this comment.
Yes! The default units change from case to case, but the rest is the same.
There was a problem hiding this comment.
Should we get layout.wideSize directly here since that's the only one we need?
There was a problem hiding this comment.
Maybe, but I'll keep the existing 'layout' retrieval. The codebase consistently uses 'layout', never its subfields.
There was a problem hiding this comment.
Would be nice if we could avoid mutation.
There was a problem hiding this comment.
At the same time, I'm not convinced useSetting() really needs to be deprecated. It uses useSettings() under the hood, so what difference does it make if we continue using it when we want to retrieve a single setting?
There was a problem hiding this comment.
Retrieving a single setting is very ergonomic even with useSettings. Having also useSetting doesn't add any interesting improvement. Therefore, it's a good idea to support only one canonical API, and clearly mark the other one as "inferior".
jsnajdr
force-pushed
the
optimize/use-setting
branch
from
64b12ae to
8a312be
Compare
|
This PR delivered a significant improvement in performance tests, namely for the "Type" and "Inserter Hovering Items" stats. Immediately visible in the charts on codevitals.run: 
|
|
This should have been called out in the Misc Dev note for 6.4 as it deprecates a stabile hook |
|
I don't remember this being cherry-picked for 6.4. Based on the Gutenberg version this hasn't shipped with latest WP. |
youknowriad
added
the
Needs Dev Note
|
Just added the dev note label to avoid forgetting about it for 6.5 |
|
I just downloaded WordPress 6.4.1 and inspected the |
|
Yes, 6.4 shipped with Gutenberg up to 16.7, so anything that's in 16.8 or newer and wasn't specifically cherry-picked didn't make it into the release. |
|
Thanks all for the confirmation and sorry for jumping to conclusions too early ❤️ Appreciate you all! |
|
Should we update the version number in the deprecation just for greater clarity? I created a PR for it here: #56377 |
| return newObject; | ||
| } | ||
|
|
||
| const stringToPath = memoize( ( path ) => path.split( '.' ) ); |
There was a problem hiding this comment.
This has the drawback of taking up memory that is never released. Maybe harmless here because it's small strings that probably get reused, but still worth nothing. I guess this is a weird case where a max size doesn't make sense. See also https://github.com/WordPress/gutenberg/pull/53406/files#r1286726012
There was a problem hiding this comment.
Removing in #56711. I remember when profiling, the memoization didn't have any impact on performance anyway.
| _Usage_ | ||
|
|
||
| ```js | ||
| const [ fixed, sticky ] = useSettings( 'position.fixed', 'position.sticky' ); |
There was a problem hiding this comment.
I'm a bit confused here because the PR description says
const [ allowFixed, allowSticky ] = useSettings( [ 'position.fixed', 'position.sticky' ] );Can both be used? It would be nice to be consistent so we don't need to normalise.
There was a problem hiding this comment.
Only the useSettings( 'a', 'b' ) form can be used. The PR originally started with an array argument, useSettings( settings: string[] ), but then was changed to useSettings( ...settings: string[] ) during review. That's why the PR description is out of date.
The main reason for that was that when reading a single setting, useSetting( 'a' ) and useSettings( 'a' ) are the same, except that for useSettings the return value is always an array (with one element).
Dev noteNew
|
|
Makes me wonder if we should just expose the |
Yes, of course, let's do that! After all, |
This is an attempt to improve the performance situation discussed in #54819, to make typing faster. I expected better results, but here it is anyway.
The problem is that 1000 blocks create 80k subscriptions to the
core/block-editorstore, and every action dispatch leads to calling 80k selectors and checking if their return value has changed. Of course, in a vast majority of cases the return value hasn't changed.useSelecthas async mode that's active on almost all of the 1000 blocks, but even adding an item to the priority queue 80k times takes some effort.This PR addresses one hook that is to blame for many (20k out of 80k) of the store subscriptions:
useSetting.The first few commits perform optimizations on the existing
useSettinghook, making it in total 40% faster. Don't create new strings, memoize splittinga.b.cpaths into['a', 'b', 'c']arrays (lodash.getalways did this, too), etc.The second part of the PR is creating a new hook,
useSettings. Instead of reading settings individually:we merge them into one hook call:
The point is that each
useSettinghook has oneuseSelectinside, leading to one store subscription per hook. My merging the setting reads together, we can save a lot of subscriptions.Migrating most usages of
useSettingtouseSettingsreduced the number ofblock-editorsubscription, on a large post with 1000 blocks, from 80k to 60k.The improvements are nice but still very incremental, they don't really solve the problem. We'll need to continue to search for other solutions.
The last commit contains some performance-measuring debug code I've been using. There are:
console.logstatements that report the number ofblock-editorsubscriptionsconsole.logstatements that report on how the async mode priority queue is being processed in a series ofrequestIdleCallbacks. Gives a good picture on how many store listeners we can process per second.I'll remove this debugging code before merging (hopefully 🙂)
This PR is best tested with a 4x CPU throttling enabled in the Performance tab in Chrome devtools.