-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store merged data in memory #5024
Store merged data in memory #5024
Conversation
The way I see it, we can make this change for 6.4 as long as we invalidate the A longer brain dump:
Not knowing this has been a source of bugs and performance issues. WordPress 6.4 is introducing two public methods for consumers to access templateParts and customTemplates, so it's unlikely that this happens again. However! I still think |
Oh! By the way, @joemcgill I'd rather make this change in Gutenberg first, and only port it to core after it has been tested there. This gives us confidence it works and makes synchronizing the two codebases easier for everyone involved. |
Thanks for taking a look and giving some feedback, @oandregal. The use case that you explained in your write up is also confirmed by failing unit tests. I can see that there are several different places where theme.json data is recalculated based on the return value of the Lifecycle issues was one of my concerns with this approach, and this confirms that is indeed a problem. That also means that any function that relies on the I'm not sure if the unit tests failures are due to missing a place where the cache should be invalidated during the test setup, or if there is a more predictable way to know when all of the different sources of data are finalized, at which point it's safe to cache the merged object. Definitely more to dig into. I'm happy to try to apply an iteration of this change to the Gutenberg version of this class first once we've got a working approach. |
} | ||
|
||
// Map origins to block cache values. | ||
$cache_map = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the advantage of declaring this map over just using the origin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. The existing calls to has_same_registered_blocks
do not use the origin
. It's clear if they do, that'd be a little nice change before this PR (or as part of it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I did this because keys used to store the $blocks_cache
values don't match the $origin
values used by WP_Theme_JSON
. If they did, then this mapping wouldn't be necessary. Not sure if we can make that update in a backwards compatible way, but it would be nice if these were stored using the same keys so it's obvious how one relates to the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's unfortunate that the names don't match, but I also think we shouldn't make a change to that at this point due to backward compatibility risk. If we want to update them, that should be in its own decoupled issue due to the additional considerations.
I see the failing test is |
@joemcgill I like where this is going, however as @oandregal mentioned above, in order to fix the outstanding issues, we would need to somehow cache the "theme data" results to also include their data This was previously attempted in #3624, which however was eventually closed. Looking back at the discussion there though, while there is some complexity to figure out, I don't think there was a clear reason it was closed, I believe we just didn't prioritize thinking about it further at the time. I do think the idea from that PR is worth picking up, particularly now as it would potentially unlock this greater performance enhancement here. Maybe we can find a better solution to avoid the problem of having to clean the theme.json caches lots of times (see https://github.com/WordPress/wordpress-develop/pull/3624/files#diff-4112a9a0bb18713bc2fc7868ceffe4403d765722f605e42b313ef2cdc218177fR354-R356)? Potentially we can come up with something along the lines of |
@oandregal I've taken another pass at this PR, and it does seem like the main thing causing the types of bugs (and unit test failures) we discussed previously was the need to recalculate the merged data if theme supports data changes. I've modified the approach so that the theme supports data gets saved to a static variable so we can test whether the value has changed, much like we test whether registered blocks have changed. This results in a big improvement in terms of reducing the number of unnecessary WP_Theme_JSON objects that get created and merged during a page lifecycle, but unfortunately doesn't seem to result in a very large performance benefit from what I can tell, but I want to do some more profiling. In the two XHProf reports below, you can see the reduction in calls to all the getter methods for various theme.json data origins, as well as a reduction in calls to Profiling: TrunkProfiling: This PRDo you think something like this is worth pursuing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joemcgill Left a bit of feedback on the logic, but overall looks great to me.
@@ -597,6 +653,8 @@ public static function get_merged_data( $origin = 'custom' ) { | |||
$result->merge( static::get_user_data() ); | |||
$result->set_spacing_sizes(); | |||
|
|||
static::$merged[ $origin ] = $result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are early returns above, so I think this needs to be added to all those clauses as well, otherwise only the custom
origin will ever have cached values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wonder if there's any value in storing anything other than the completed merged data. I'll raise that question as part of the PR to the Gutenberg repo, but this is good feedback!
if ( | ||
null !== static::$merged[ $origin ] | ||
&& static::has_same_registered_blocks( $cache_map[ $origin ] ) | ||
&& static::get_theme_supports_data() === static::$theme_support_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theme support data only matters for the theme
and custom
origins, so I think we should only apply this check if the given origin is one of those, to keep caches warm for the other two origins.
} | ||
|
||
// Map origins to block cache values. | ||
$cache_map = array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's unfortunate that the names don't match, but I also think we shouldn't make a change to that at this point due to backward compatibility risk. If we want to update them, that should be in its own decoupled issue due to the additional considerations.
@joemcgill In my benchmarking (similar approach that I used for #5267), this PR shows a small but consistent improvement. After rerunning a few times, it is always slightly faster with this PR, so I feel good about making this change. For reference:
It also makes sense to me that the benefit is slightly higher for the block theme given If my observation from #5024 (comment) is correct, I wonder whether with that fix we can get a little bit more of an improvement from that. |
Thanks @felixarntz. I'm going to go ahead and apply these changes to a Gutenberg PR to get feedback from that team and to hopefully get this tested in the plugin prior to a core change. |
@felixarntz I've applied your PR feedback and also opened a PR with these changes in the Gutenberg repo, here: WordPress/gutenberg#54742 |
Continued the conversation in the gutenberg PR WordPress/gutenberg#54742 (review) |
I'm closing this and the related Gutenberg PR given that this approach didn't provide a large enough measurable performance impact. In the future, I think this could be useful as reference if we are able to make the merged |
This is a performance optimization of the
WP_Theme_JSON_Resolver
, which stores the mergedWP_Theme_JSON
object to a static property to avoid unnecessarily recreating this object by merging from other sources. In my testing, this cuts the execution time of this method in half.Trac ticket: https://core.trac.wordpress.org/ticket/57789
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.