-
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
Improve performance of WP_Theme_JSON::get_merged_data
method
#3441
Conversation
I've rebased this PR to be based on trunk@54506 and recalculated data for WordPress 6.0, WordPress 6.1 RC1 and this PR. |
cc @ockham @hellofromtonya this is ready for review/merge. There are some failures in the For automated test runs, this test is only run on trunk
This test requires the index to be truncated.
Rendering PDFs is not supported on this system.
The image editor engine WP_Image_Editor_Imagick is not supported on this system. I'd appreciate if someone could re-run these or provide help on what's happening to unblock this PR. (Edit: https://core.trac.wordpress.org/ticket/56817 investigates this issue) |
Thanks @oandregal This change improve the performance in my test. |
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.
Nice! LGTM 👍
get_merged_data is now a hotpath that is called many times. By improving small things that are repeated multiple times we get more performance wins. Instead of starting with an empty WP_Theme_JSON and call the merge operation we start by using the existing WP_Theme_JSON from core. This reduces from 402 to 326 the number of calls of the low-level WP_Theme_JSON->merge method.
Rebased from |
Committed to trunk in cb98544. We should leave the PR open until there is a 6.1 branch and we confirm the commit is there (because the branch was created after the commit) or the commit is not there, but we do the backport. |
No need to leave this open. The 6.1 branch will be copied from the latest state of |
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.
Tested, works well. A worthwhile improvement for the RC. 👍
I'm proposing we revert this change at #4145 as it introduced a bug. |
See:
theme.json
processing gutenberg#44772What
Follow up to #3418
Why
We want to squeeze as many ms as possible.
How
In the WordPress 6.1 cycle,
WP_Theme_JSON_Resolver::get_merged_data
method has become a hot path that is called many times. By improving small things that are repeated multiple times, we get more performance wins. This PR changes this code:to:
As a result, this reduces the number of calls of the low-level
WP_Theme_JSON->merge
method, with the corresponding performance improvements.Performance improvements
WP_Theme_JSON_Resolver::get_merged_data
WP_Theme_JSON->merge
See #3418 for instruction on how to reproduce these numbers yourself. Alternatively, you can import the following cachegrind files to visualize in the tool of choice (kcachegrind, qcachegrind, webgrind, etc.):
Test