-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Optimize compute_style_properties
by reducing the number of iterations it performs
#51983
Optimize compute_style_properties
by reducing the number of iterations it performs
#51983
Conversation
cd8d56c
to
10d5c77
Compare
|
||
foreach ( $properties as $css_property => $value_path ) { | ||
$root_variable_duplicates = array(); | ||
foreach ( $properties_to_inspect as $css_property => $value_path ) { | ||
$value = static::get_property_value( $styles, $value_path, $theme_json ); | ||
|
||
if ( str_starts_with( $css_property, '--wp--style--root--' ) && ( static::ROOT_BLOCK_SELECTOR !== $selector || ! $use_root_padding ) ) { |
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.
if ( str_starts_with( $css_property, '--wp--style--root--' ) && ( static::ROOT_BLOCK_SELECTOR !== $selector || ! $use_root_padding ) ) { | |
if ( ( static::ROOT_BLOCK_SELECTOR !== $selector || ! $use_root_padding ) && str_starts_with( $css_property, '--wp--style--root--' ) ) { |
Changing this line also helps performance.
Flaky tests detected in 10d5c77. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5391880948
|
In profiling, the results are big enough that warranted an investigation:
Before: After: Note:
|
I'm unconvinced this has a real impact, given the benchmark numbers I've shared. I'll close it in a few days, unless someone else wants to review the benchmark I've done, or provide a different one that shows impact. See "How to reproduce the benchmarks locally" section in the issue description. It sounds like this is one of those cases where the observational cost of profiling reports bigger gains than it actually is. |
@spacedmonkey I presume those screenshots you shared are from a profiling tool, are they? While useful for initial investigations, we cannot count on profiler numbers to show impact on production environments. See how profiling and benchmarking differ in this PR as an example. |
cc @felixarntz This PR doesn't have any urgency. I know your benchmark setup is different from mine, so I was curious to see what your benchmark would report, when you have the time. |
Related #45171
What?
This PR improves TTFB by improving how
WP_Theme_JSON_Gutenberg::compute_style_properties
operates.Why?
The faster, the better.
TTFB using a production environment locally (data):
How?
The
compute_style_properties
method is responsible for converting a "style node" coming from atheme.json
structure into a list of CSS declarations.Input:
Output:
To do such conversion, it maintains a map of CSS properties to paths in a "style node" within a
theme.json
file: PROPERTIES_METADATA. At the time of writing this, there's 54 of them.The
compute_style_properties
method iterates over all of them for every input node. As a result, in a theme such as TwentyTwentyThree with 65 style nodes, some code paths are executed 65 * 54 = 3510 times.This PR updates how
compute_style_properties
works to iterate over the properties that exist in the input, not over all of them. In a theme such as TwentyTwentyThree this can be less than 3 properties per style node, amounting to 65 * 3 ~= 195 times.Testing Instructions
Make sure the automated tests pass.
How to reproduce the benchmarks locally:
npm install && npm run build
..wp-env.override.json
file with the following contents:npm run wp-env start
.eq 250 | xargs -Iz curl -o /dev/null -H 'Cache-Control: no-cache' -s -w "%{time_starttransfer}\n" http://localhost:8888/ | xclip -selection clipboard
.Do the above for each one of the last default themes (TwentyTwenty, TwentyTwentyOne, TwentyTwentyTwo, TwentyTwentyThree) for this branch. Then, do the same using
trunk
at 72b7d89.