Make WordPress Core

Opened 9 months ago

Last modified 8 months ago

#60787 new enhancement

get_registered_meta_keys() should allow for late registration to improve performance

Reported by: sc0ttkclark's profile sc0ttkclark Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch
Focuses: performance Cc:

Description

For a site with many custom post types, custom taxonomies, and tons of custom fields -- the burden of register_meta() can become heavy especially when using a plugin like Advanced Custom Fields, Pods, and others.

But we shouldn't have to register every single field on every single page load regardless of whether it's needed. That just drains performance on a given site and requires needless DB/transient/cache requests.

The introduction of things like the Block Bindings API rely on fields registered through the REST API.

You can register fields with the REST API during the rest_api_init action (which yeah, you pretty much have to register everything all at once) but at least it's not every page load.

With the Block Bindings API rendering on the front of the site, this requires you having run your fields through register_meta() on any given page that might have that field bound.

Related dev tutorial: https://developer.wordpress.org/news/2024/02/20/introducing-block-bindings-part-1-connecting-custom-fields/

Proposed solution:

Run an action at the top of get_registered_meta_keys() so a plugin can run register_meta() ONLY at the time of that data being needed at the code level.

Change History (12)

This ticket was mentioned in PR #6282 on WordPress/wordpress-develop by @sc0ttkclark.


9 months ago
#1

  • Keywords has-patch added

These changes introduce a new action that plugins can hook into to register their meta fields only when they are called instead of on every page load.

#2 @sc0ttkclark
9 months ago

The work in https://github.com/WordPress/wordpress-develop/pull/6282 introduces the new action.

As a result of this new action, I saw that there was one other function that is not accounted for which is the default value filter.

That function filter_default_metadata() was manually interacting with $wp_meta_keys in a way that prevented the action from being effective for default value handling. I adjusted that function to make use of the get_registered_meta_keys() function without changing the overall logic of that function beyond what it did before.

#3 @swissspidy
9 months ago

  • Component changed from General to Posts, Post Types
  • Keywords reporter-feedback added

Thanks for opening this ticket.

You mention performance considerations with this enhancement, but it's not clear how simply registering meta keys causes "needless DB/transient/cache requests". Could you perhaps elaborate on that, maybe with some actual numbers?

If there are in fact some performance issues with the way meta registration works, it would be great if we could resolve those holistically rather than requiring developers to "opt in" by using this hook to register meta keys as late as possible.

#4 @sc0ttkclark
9 months ago

  • Keywords reporter-feedback removed

With the action as proposed, developers don't need to change their code except for the action they hook into to register their meta. It's a win-win because there's not a brand new API or set of functions. Just a better time to register them.

Use case would be something like this for the many plugins that cover custom fields in WP which want to enable their custom fields to be covered by the only currently working Block Bindings source on the front AND editor in WP 6.5: Post Meta.

Current:

  • Plugin hooks on init on every page load
  • During their action, they fetch ALL meta fields for ALL content types that need to be registered -- EVERY PAGE LOAD: Multiple DB requests to fetch configs, maybe saved to transient/cache for next page load
  • Run register_meta() for each of the fields
  • ❌ End result: Visitor goes to any page on the front of the site and the meta is registered regardless of it being needed by the current context -- But if you use the Block Binding API for Custom Fields as shipping in 6.5 then you have no other choice because it must be registered by render time

Proposed:

  • Plugin hooks on pre_get_registered_meta_keys which only gets called in the context of get_registered_meta_keys() calls
  • During their action, they conditionally fetch ALL meta fields for the object type/subtype being requested -- AS NEEDED: Multiple DB requests to fetch configs, maybe saved to transient/cache for next page load
  • Run register_meta() for each of the fields
  • ✅ End result: Visitor goes to any page on the front of the site and the meta is not registered unless it is needed by the current context -- If you use the Block Binding API for Custom Fields as shipping in 6.5, your meta can be registered in time for this context without any other custom code

This may be a one-line change for most developers (depending on their implementation of course) with a huge benefit overall of reducing DB requests, object cache requests, memory usage, or anything else I may be forgetting that's involved typically.

Other areas beyond just Block Bindings API that would be useful to increase adoption of register_meta() by Custom Fields plugins would be Revisions and for showing those fields in the REST API in general.

I was involved in the original implementation of register_meta() and we didn't have the wisdom that this would be a problem later, but I definitely wish we would have established that action and documented it as the official example of how to register meta fields at the time.

#5 @sc0ttkclark
9 months ago

  • Component changed from Posts, Post Types to Options, Meta APIs

#6 @sc0ttkclark
9 months ago

I should note that the developer will probably need more than just changing the action name, they will want to also include logic to check if their action has already been run to prevent it being called again when multiple blocks have post meta binding on and in other use-cases which have get_registered_meta_keys() called on the same page load.

#7 @jorbin
9 months ago

Are there any tests showing the performance impact of this proposed change?

EVERY PAGE LOAD: Multiple DB requests to fetch configs, maybe saved to transient/cache for next page load

This feels like a good reason not to store configs in the DB but in code where they can also be version-controlled.

#8 @sc0ttkclark
9 months ago

@jorbin I don't have any tests to show the impact at scale for this yet.

For storing configs in DB vs File, there's only a smaller segment of people who use these plugins that would be aware of that kind of thing. For example, ACF itself has 2+ million active installs, but people must pay for a separate ACF Pro plugin to get access to a local JSON feature.

Many Custom Field plugins I've seen have not attempted to utilize register_meta() because of the impact on every page load but that adoption will increase with the Block Bindings API for Post Meta in WP 6.5.

ACF only recently included register_meta() usage but not for every custom field -- only for a unique ACF field that for handling custom field revisions (a WP 6.4 feature) outside of core logic.

@jorbin I can look into how to test the performance differences between a theoretical ACF customization that attempted to do that logic in WP 6.5 vs the new way from my PR. Any suggestions on how to gather that info or would just comparing stats from Query Monitor be enough?

Last edited 9 months ago by sc0ttkclark (previous) (diff)

#9 @jorbin
9 months ago

Two types of performance data that I think would be helpful 1) how it affects the number of queries and 2) Seeing the comparison of before/after for the numbers in the performance tests

#10 @gaambo
9 months ago

With more and more things in WordPress moving to register_* APIs (which are great 🙏) instead of just "structurless" filters/actions, there's more "initializing" work that needs to be done on each request.
In the past it has just been register_post_type and register_taxonomy (and maybe registering scripts/styles if you want to count them), but now we have many other functions: register_block_type, register_block_pattern, register_block_pattern_category, register_block_style, register_meta (+ variants like register_post_meta), register_setting, register_rest_route, register_rest_field.

I think in general it would make sense to think for each one of those, if they need to run in any context or not.

For example, post type and taxonomy registering need to run on every request. Block types possible as well. Registering rest routes/fields are already limited to rest_api_init. I think the performance team is looking into loading patterns only in the editor or at least caching them (because of the expensive file look-ups). Registering block variations was also deemed to be expensive therefore being moved into a callback which is only called in the editor.
Registering settings may be required on each call (so you get default values are handled).

Now I don't have any numbers myself, and I see that it would really make sense to get and compare them, before doing any premature optimizations. And maybe registering all of those things is really cheap, because these are just arrays in the memory.
But I think many plugins need to do some database calls before/while registering those things - for example only registering a post type, if a feature is enabled or only registering meta fields to that post type if the feature is enabled. I've seen large plugins registering about 50 different styles/scripts each with file system calls (for filemtime) and checking if a specific feature is enabled and the script is needed. That's not even touching custom field plugins, which store the whole configuration in the db.

So while the core register functions may be cheap, there may be a lot of work "behind" it. Lazy "initializing" these things may be good. But again, I can't argue with any numbers, that's just what seems logical to me - maybe it really is overkill.

While I think discussing "why" and "if" makes more sense before jumping into implementation, I just want to note that in core most hooks prefixed with pre_ are there to shortcircuit the rest of the function. So if this is developed further, I think the hook should be called something else (like register_meta_keys). But that's just a small nit.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


8 months ago

Note: See TracTickets for help on using tickets.