Make WordPress Core

Opened 17 months ago

Closed 17 months ago

Last modified 16 months ago

#57545 closed enhancement (fixed)

Backport: WP_Theme_JSON_Resolver changes

Reported by: mamaduka's profile Mamaduka Owned by: jorgefilipecosta's profile jorgefilipecosta
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch has-unit-tests gutenberg-merge
Focuses: Cc:

Description

Backport changes and fixes merged into a Gutenberg plugin after WP 6.1.

The Gutenberg plugin bundles a copy of WP_Theme_JSON_Resolver class. The backport PRs are created based on diffing core and Gutenberg files. See https://github.com/WordPress/gutenberg/pull/46750.

Change History (10)

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


17 months ago
#1

  • Keywords has-patch has-unit-tests added

#2 @hellofromTonya
17 months ago

  • Keywords gutenberg-merge added

Adding keyword for tracking backports from Gutenberg to Core.

@hellofromTonya commented on PR #3901:


17 months ago
#3

Hey @Mamaduka, is this PR and Trac ticket still needed? I remember you were breaking up the GB PRs into smaller Trac tickets. Just checking before I dive into it.

@Mamaduka commented on PR #3901:


17 months ago
#4

@hellofromtonya, that was for WP_Theme_JSON file. I decided to keep this one since I had already done the work, and most of the line changes were for PHPUnit tests.

@Mamaduka commented on PR #3901:


17 months ago
#5

Thanks for the suggestions, @costdev!

@flixos90 commented on PR #3901:


17 months ago
#6

## Performance check ✅

I gave this some server-side performance tests for the frontend, measuring TT3 on the home page and a single post with the Gutenberg demo content, using https://gist.github.com/felixarntz/de5c697a1a16c2b892634b70216eb6c7.

### TLDR / Summary / Conclusion
Results look inconclusive. Function-specific benchmarks show a clear improvement, while overall WordPress execution time shows inconsistent results between the two tests. This suggests that the changes in this PR probably enhance performance, but overall does not have a great impact on performance. At the same time, it also means that there is no concern about merging this, so we can proceed from that perspective.

### Results

_All times below are in milliseconds._

First I benchmarked overall WP load time. Every test row below is actually based on the median of 20 runs itself. So overall this is 120 runs per scenario.

| trunk @ [55218] | plus PR #3901 | % diff

-- | -- | -- | --
Home page (test 1) | 193.99 | 206.9 | 6.65%
Gutenberg demo (test 1) | 195.65 | 193.1 | -1.30%
Home page (test 2) | 194.31 | 198.11 | 1.96%
Gutenberg demo (test 2) | 194.98 | 191.46 | -1.81%
Home page (test 3) | 198.5 | 203.47 | 2.50%
Gutenberg demo (test 3) | 193.64 | 202.12 | 4.38%

Because the results didn't show a clear trend in either direction, I wanted to see at least more specifically whether certain WP_Theme_JSON_Resolver class methods showed performance increases or regressions here.

Here are the results for WP_Theme_JSON_Resolver::get_merged_data(), again based on 20 runs per scenario (for the home page):

| trunk @ [55218] | plus PR #3901 | % diff

-- | -- | -- | --
get_merged_data() time | 14.63 | 13.99 | -4.37%
get_merged_data() calls | 50 | 50 | 0.00%
WP total time | 199.01 | 202.04 | 1.52%

Here are the results for WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles(), again based on 20 runs per scenario (for the home page):

| trunk @ [55218] | plus PR #3901 | % diff

-- | -- | -- | --
get_user_data_from_...() time | 2.5 | 1.57 | -37.20%
get_user_data_from...() calls | 2 | 2 | 0.00%
WP total time | 198.64 | 196.6 | -1.03%

Both of the above show improvements for their respective method's performance, so that's promising. We will have to see whether this has any impact in the field, but overall it certainly looks unproblematic, and if anything, there is a positive trend in WP_Theme_JSON_Resolver performance.

_Note: The get_style_variations() method was not covered as it is not invoked in the frontend and thus has no impact there._

@Mamaduka commented on PR #3901:


17 months ago
#7

Thanks for the reviews and testing, folks 🙇

It would be great to get this committed before Beta 1.

#8 @jorgefilipecosta
17 months ago

  • Owner set to jorgefilipecosta
  • Resolution set to fixed
  • Status changed from new to closed

In 55231:

Block editor: Update WP_Theme_JSON_Resolver and improve its performance.

This commit includes the latest updates WP_Theme_JSON_Resolver class made in the block editor. Some of these updates improve the performance of the class.

Props Mamaduka, hellofromTonya, flixos90, jorgefilipecosta, oandregal, spacedmonkey, audrasjb, costdev, scruffian.
Closes #57545.

This ticket was mentioned in Slack in #themereview by dd32. View the logs.


16 months ago

Note: See TracTickets for help on using tickets.