Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Improve performance of WP_Theme_JSON::get_merged_data method #3441

wants to merge 1 commit into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Oct 11, 2022

See:

What

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:

public static function get_merged_data( $origin = 'custom' ) {
	// ...

	$result = new WP_Theme_JSON();
	$result->merge( static::get_core_data() );
	$result->merge( static::get_block_data() );
	$result->merge( static::get_theme_data() );

	// ...
}

to:

public static function get_merged_data( $origin = 'custom' ) {
	// ...

	$result = static::get_core_data();
	$result->merge( static::get_block_data() );
	$result->merge( static::get_theme_data() );

	// ...
}

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

Method WordPress 6.0 WordPress 6.1 RC1 PR 3441 (based off trunk@54506)
total time 746ms 877ms 794ms
WP_Theme_JSON_Resolver::get_merged_data 9ms (27 calls) 146ms (98 calls) 55ms (98 calls)
WP_Theme_JSON->merge 25ms (130 calls) 114ms (512 calls) 74ms (414 calls)

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

  • Make sure automated test pass.
  • Manually test that global styles still works as expected by using the TT2 or TT3 themes and updating some block styles via the global styles sidebar.
  • Use a localized WordPress and verify that translations for colors are still applied.

@oandregal
Copy link
Member Author

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.

@oandregal
Copy link
Member Author

oandregal commented Oct 13, 2022

cc @ockham @hellofromtonya this is ready for review/merge.


There are some failures in the PHPUnit Tests / 8.2 on ubuntu-latest job. These are totally unrelated to this PR and I don't know why they fail. They say things such as:

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)

@mukeshpanchal27
Copy link
Member

Thanks @oandregal This change improve the performance in my test.

Copy link
Member

@aristath aristath left a 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.
@oandregal
Copy link
Member Author

Rebased from trunk to address the environment failures. I've seen some test have been disabled since this was reported.

@jorgefilipecosta
Copy link
Member

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.

@desrosj
Copy link
Contributor

desrosj commented Oct 14, 2022

No need to leave this open. The 6.1 branch will be copied from the latest state of trunk, and the changeset will be included.

@desrosj desrosj closed this Oct 14, 2022
@oandregal oandregal deleted the try/reduce-merge-calls branch October 17, 2022 09:10
Copy link

@mcsf mcsf left a 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. 👍

@oandregal
Copy link
Member Author

I'm proposing we revert this change at #4145 as it introduced a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants