Make WordPress Core

Opened 14 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#59595 closed defect (bug) (fixed)

Improve performance of WP_Theme_JSON::compute_style_properties

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.7 Priority: normal
Severity: normal Version: 5.8
Component: Editor Keywords: has-patch has-unit-tests
Focuses: performance Cc:

Description

When profiling WP 6.4 ( other releases ), it was not that the method WP_Theme_JSON::compute_style_properties can result in around 6-11% of server time spent to generate a page. Research should be conducted to see if this method and child function / method calls, could be improved, to improve performance of this method.

Attachments (4)

Screenshot 2023-10-24 at 22.37.16.png (1.2 MB) - added by pereirinha 14 months ago.
Request overview before
Screenshot 2023-10-24 at 22.38.16.png (1.1 MB) - added by pereirinha 14 months ago.
Function details before
Screenshot 2023-10-24 at 22.37.29.png (1.2 MB) - added by pereirinha 14 months ago.
Request overview after
Screenshot 2023-10-24 at 22.38.19.png (1.1 MB) - added by pereirinha 14 months ago.
Function details after

Change History (106)

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

This ticket was mentioned in Slack in #core-performance by pereirinha. View the logs.


14 months ago

This ticket was mentioned in Slack in #core-performance by pereirinha. View the logs.


14 months ago

@pereirinha
14 months ago

Request overview before

@pereirinha
14 months ago

Function details before

@pereirinha
14 months ago

Request overview after

@pereirinha
14 months ago

Function details after

#4 @pereirinha
14 months ago

I tested using WordPress 6.5-alpha-56966-src with the TTF theme while visiting an archive page with 10 posts. I can replicate similar results with other versions and themes and am happy to share the results if found relevant. For profiling, I used XHProf after an unsuccessful attempt to use Blackfire.

These are the results of the tests I made and the findings.

Though the compute_style_properties method is static, I didn't find a case where the arguments would be the same to benefit the runtime.

In my testing, the number of calls for this method averaged 85 calls per request.

The description of the method in its docblock states that:

Given a styles array, it extracts the style properties and adds them to the $declarations array…

In a single request, the cumulative size of its arguments is:

  • Styles: 23.725 KB,
  • Settings: 803.350 KB,
  • Properties: 202.456 KB,
  • Theme Json: 1.748 MB,
  • Selector: 2.634 KB
  • Iterations: 85

Responsible for about 50% of the wall time for this method is the child function str_starts_with, followed by WP_Theme_JSON::get_property_value with about 33.5%.

Most of the time is spent with PHP's natural functions, which pull data out of large datasets, so it can't be optimized much across the board. However, I gave it a go with object caching, and the benefits are considerable for those who can leverage this feature.

Attached are the print screens from the tests conducted. The before is as core, and the after runs a warm cache.

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


14 months ago
#5

  • Keywords has-patch added

Add object caching to improve the performance of WP_Theme_JSON::compute_style_properties when supported.

Trac ticket: 59595

This ticket was mentioned in Slack in #core-performance by pereirinha. View the logs.


14 months ago

#7 @mukesh27
14 months ago

  • Milestone changed from Future Release to 6.5
  • Owner set to pereirinha
  • Status changed from new to assigned

Assign to @pereirinha as he is actively working on it.

@mukesh27 commented on PR #5567:


14 months ago
#8

@pereirinha I did some benchmarking for TT4 theme home page using XHProf but i can't i can't find any improvement. Did i miss anything?

cc. @felixarntz @joemcgill

@pereirinha commented on PR #5567:


14 months ago
#9

@pereirinha I did some benchmarking for TT4 theme home page using XHProf but i can't i can't find any improvement. Did i miss anything?

cc. @felixarntz @joemcgill

Thanks, @mukeshpanchal27, for picking it up. Can the reason why you don't see improvements might be that you aren't using Memcached?

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


14 months ago

@mukesh27 commented on PR #5567:


14 months ago
#11

Thanks, @mukeshpanchal27, for picking it up. Can the reason why you don't see improvements might be that you aren't using Memcached?

I just enabled the Memcached through ENV and now i got the improvement after the changes.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/10103365/94cc90c8-cca2-40c8-9b59-b0969ca87301

This ticket was mentioned in Slack in #core-performance by pereirinha. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


11 months ago

@pereirinha commented on PR #5567:


11 months ago
#14

When and how is this cache invalidated? Adding caching is great, but if not correctly cache invalidated when file is change, this will make more problems than it solves.

@spacedmonkey, the cache is created based on the requested arguments. A single page load will use several different cache entries, and we are using these arguments to create a md5 hash. Now we will also include the wp_get_global_settings(), so that any changes the users make will force a new cache entry.

The cleanup will happen daily so that unused data gets purged. The caveat is that, even if there are no changes, the cache will have to be regenerated once a day.

@pereirinha commented on PR #5567:


11 months ago
#15

@felixarntz @mukeshpanchal27 this is ready for you to circle back.

I appreciate your patience here.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


11 months ago

@isabel_brison commented on PR #5567:


11 months ago
#17

Thanks for the ping! I can't see any obvious issues with this approach and changing global styles still works well in my local dev environment. Perhaps @andrewserong or @ramonjd might have additional thoughts as iirc they worked on previous caching-related bugs.

@andrewserong commented on PR #5567:


11 months ago
#18

Thanks for the ping! I can't see any obvious issues with it, either, but I'm a little cautious about caching in general for these classes just because it can be really hard to predict what sorts of things might go wrong. For example, I see there's no guard against development mode. Will that make it hard for theme and or core developers working locally who are experimenting with making changes in the processing logic (outside of just changes within their theme)?

I agree with this comment about reducing the cache expiry time down to 1 hour: https://github.com/WordPress/wordpress-develop/pull/5567#discussion_r1445408359 that feels safer to me in case there are any issues for folks during a WP version upgrade.

Actually, that'd probably be my main question: would this cache get automatically flushed when a site is upgraded? It's likely that processing logic will change between releases, for example the fluid typography rules can be tweaked, or there could be additions to this function in the future.

Very cool that adding caching here can improve performance! At first I was wondering if it might introduce a performance hit with the extra calls to wp_get_global_settings() and wp_json_encode... interesting that it's cheaper to do both of them to generate a key than to process compute_style_properties with the provided values 🤔

@ramonopoly commented on PR #5567:


11 months ago
#19

Also thanks for the ping!

Love caching stuff when we can, especially huge buckets of style gloop.

I'd lean on @oandregal's experience and opinion here. I believe he worked on caching theme.json output before. E.g., https://github.com/WordPress/gutenberg/pull/45543

#20 @joemcgill
11 months ago

  • Keywords changes-requested added

@pereirinha it's looking like the current approach won't work, based on @oandregal's feedback in the PR.

I think we could either look at setting these caches in wp_add_global_styles_for_blocks(), or as a new property in WP_Theme_JSON_Resolver that produces the WP_Theme_JSON object used in that function via WP_Theme_JSON::get_styles_for_block().

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


10 months ago

#22 @joemcgill
10 months ago

  • Milestone changed from 6.5 to 6.6

This still needs to be updated based on the latest feedback on the PR. Punting to 6.6 ahead of the 6.5 beta 1 date tomorrow.

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


8 months ago

#24 @joemcgill
8 months ago

  • Owner changed from pereirinha to thekt12

Reassigning to @thekt12, who volunteered to pick this up for the 6.6 milestone. Please make sure to review the feedback on @pereirinha's previous PR.

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


8 months ago
#25

Trac ticket: 59595

Incorporating feedback from - https://github.com/WordPress/wordpress-develop/pull/5567

#26 @thekt12
8 months ago

I have added a new PR#6392 based on feedback from @pereirinha initial PR#5567

In the current run, I am not able to record the same improvement that @pereirinha had recorded.

I also see the PR may be improved for sites with persistent cache but the way we are creating cache key it has a -ve effect (almost 6% downgrade) in performance for sites with non-persistant cache.

Version 0, edited 8 months ago by thekt12 (next)

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


8 months ago

#31 @joemcgill
8 months ago

Following up after chatting with @thekt12 about the circular dependency issue noted in my PR review. To avoid this, we could consider creating a new method on the WP_Theme_JSON_Resolver class that will get computed styles for the merged JSON object, though a better approach is likely to move caching of this data to the wp_add_global_styles_for_blocks() helper which ends up calling WP_Theme_JSON::compute_style_properties() via WP_Theme_JSON::get_styles_for_block() for each registered block.

In doing so, we may need break up caching so that we are only caching computed style properties for core blocks, so we can invalidate when the WP version changes. Otherwise, we will need to invalidate this cache whenever the registered blocks have changed, which is challenging because we don't have a concept of block versions, so an update to an existing block could introduce changes in the styles that would need to be accounted for.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-performance by clarkeemily. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


7 months ago

#35 @thekt12
7 months ago

@joemcgill I have updated the PR with new changes. I can see 10% improvement with the latest changes.

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


7 months ago

@joemcgill commented on PR #6392:


7 months ago
#38

@kt-12 I spent some time looking over the unit tests and add a couple additional tests that I think could be useful in d0b243c. I could not think of a good way of ensuring that the cache was actually used in order to avoid unnecessary calls to get_styles_for_block() since we're not able to modify the WP_Theme_JSON_Resolver class hard coded in wp_get_global_styles_custom_css() nor the WP_Theme_JSON object that is returned by WP_Theme_JSON_Resolver::get_merged_data().

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


7 months ago

@joemcgill commented on PR #6392:


7 months ago
#40

This is looking good. Just doing some testing of the PR and I see a large improvement in the wp_add_global_styles_for_blocks() function. Some of the improvement is offset by the fact that the transient requires another DB query, but in persistent caches, those will also be avoided.

Trunk

https://github.com/WordPress/wordpress-develop/assets/801097/ff5152cd-8ec4-4359-a7c3-df6dc84ab5d3

This PR
https://github.com/WordPress/wordpress-develop/assets/801097/34dc8ee7-4c0c-4807-8036-dfd0532cec58

Some of the Unit test failures seem legitimate, so let's figure out those issues and this should be almost ready to go in.

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


6 months ago
#41

Minor changes have been made to class-wp-theme-json.php to better cache the theme files. This includes adding an 'update_cache' functionality that sets 'str_start_with' and 'str_start_with_cache', and removing redundant checks for empty or non-numerical values. These improvements aim to optimize performance by updating cache when necessary.

Trac ticket: https://core.trac.wordpress.org/ticket/59595

#42 @spacedmonkey
6 months ago

@thekt12 @joemcgill @pereirinha I put together my PR for this. It add some performance improvements and logic improves. I am seeing some improves. It reduced the calls to str_starts_with from 9k down to 3k.

Can one of you run some profiles and see if this is a real improvement.

This ticket was mentioned in Slack in #core-performance by spacedmonkey. View the logs.


6 months ago

#44 @adamsilverstein
6 months ago

@spacedmonkey is your PR intended as a replacement for the one @thekt12 has been working on?

@joemcgill commented on PR #6392:


6 months ago
#45

@kt-12 I went ahead and resolved conflicts and updated this branch with the fixes proposed in my other PR so will close that now. Looks like this has fixed the test failures we were seeing on this branch. Let's do some final checks, but I think this is about ready to commit.

@joemcgill commented on PR #6392:


6 months ago
#46

With the latest updates this is still profiling really well and showing a large improvement for wp_add_global_styles_for_blocks().

Trunk
https://github.com/WordPress/wordpress-develop/assets/801097/14bf909a-6a20-4d1a-9dcd-f02ddd4dc9fb

PR
https://github.com/WordPress/wordpress-develop/assets/801097/ea22be7b-0774-49ce-b461-19be6f287457

That said, those improvements are not currently reflected in the benchmarks performed by the Performance Workflow. I suspect it's because the extra query being performed to get the transient is offsetting the savings introduced in the function, but that's just a guess. Given the size of the potential improvement, it still may be worth committing so we can continue to do further testing during the beta period.

@joemcgill commented on PR #6707:


6 months ago
#47

Thanks @spacedmonkey. I did some profiling of this change against trunk and it does reduce the number of str_starts_with calls quite drastically.

Trunk
https://github.com/WordPress/wordpress-develop/assets/801097/aa82a8b0-7b42-45bf-a602-8c704331704d

This PR
https://github.com/WordPress/wordpress-develop/assets/801097/eeac66c2-5fca-4cad-86a8-b9124b373d66

That said, the PR we've been working on has a bigger impact by reducing the number of calls to this method altogether. It may be worth combining both approaches to maximize the benefit, but I'd like to prioritize getting #6392 into trunk first as it has the biggest potential impact.

#48 @oglekler
6 months ago

  • Milestone changed from 6.6 to 6.7

We have 3 hours until the commit freeze before Beta 1 release, and it looks like this ticket will not be in Trunk in time. I am rescheduling it to the next milestone. If you are hard at work and about to merge, return it when ready.

#49 @joemcgill
6 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58334:

Editor: Cache global styles for blocks.

This caches the generated CSS from block nodes in merged Theme JSON data to avoid repeated costly operations required to compute style properties for blocks. The generated CSS is saved to a transient that expires every hour.

Props thekt12, spacedmonkey, pereirinha, mukesh27, isabel_brison, oandregal, andrewserong, ramonjd.
Fixes #59595.

@joemcgill commented on PR #6392:


6 months ago
#50

Merged in 58334.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


6 months ago

#52 @joemcgill
6 months ago

  • Milestone changed from 6.7 to 6.6

#53 @spacedmonkey
6 months ago

@joemcgill @thekt12 I have created a PR for the gutenberg repo. Would love some eyes on this.

@joemcgill commented on PR #6707:


6 months ago
#54

@spacedmonkey are ok to close this in favor of https://github.com/WordPress/gutenberg/pull/62522?

#55 @joemcgill
6 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[58334] uses a transient with a 1 hour TTL to cache global styles for blocks to avoid the costly calculation of styles for registered blocks. However, the transient expiration time results in an extra DB query being made on each request since transients with expiration do not get autoloaded, which mostly negates the performance benefit of the cache. I'm reopening to see if we can avoid this extra query.

The approach already uses a hash of the global settings and block nodes as part of the cache key, so it's possible that we could just use that hash for cache invalidation and bust cache whenever the hash misses rather than needing a TTL.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


6 months ago

@ellatrix commented on PR #6707:


6 months ago
#58

@joemcgill This is the backport PR? We'll need both?

@joemcgill commented on PR #6707:


6 months ago
#59

@ellatrix and @spacedmonkey, it's fine to use this PR as the backport from https://github.com/WordPress/gutenberg/pull/62522. At present, it's not the same code though, so this will need to be updated once the GB PR is finalized.

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


6 months ago
#60

This uses a consistent cache key with the cache busting hash saved to the cache value rather than as part of the key. This allows us to remove the expiry time for the cache and only invalidate when the hash changes.

This also swaps the use of a site transient to the use of a regular transient so that the option will be autoloaded since there is no expiration time.

Trac ticket: https://core.trac.wordpress.org/ticket/59595

#61 @joemcgill
6 months ago

After spending some time reviewing the caching strategy implemented in [58334], in light of my previous comment, I have worked on a proof of concept PR that tries to avoid the extra DB query introduced here, but I have a few concerns.

To avoid the extra DB query, we can move the hash to inside the transient, but we're currently using the hash key as a way to differentiate different caches on the same network, using site transients. However, site transients never get autoloaded, even if they never expire.

I've moved the hash to a value stored inside the transient value, as a way to validate the cache and rebuild if needed. Even so, the addition of wp_prime_option_caches() to set_transient() (et al.) in [58134] is causing an extra query to prime the timeout value, even if no expiration is present and the value has been autoloaded. That is probably worth a separate ticket altogether, but worth mentioning here.

I'm not seeing a big performance difference with avoiding the extra query, so I don't know that any additional changes are needed here for 6.6, unless additional bugs are reported.

@thekt12, @spacedmonkey, and @peterwilsoncc, I'd appreciate your thoughts here — both on the proposed changes in PR #6879 as well as the broader concerns about priming timeouts behavior.

@joemcgill commented on PR #6707:


6 months ago
#62

@spacedmonkey, I've approved the related GB PR. Once this PR is updated to reflect the latest changes, I'm happy to give another review.

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


6 months ago
#63

The method compute_style_properties() in class-wp-theme-json.php has been refactored for more efficient theme style computation. The flow has been improved by reordering conditions and breaking down complex ones into simpler bits. Now the code is easier to read, and the handling of protected properties and root styles are clearer.

Backport from https://github.com/WordPress/wordpress-develop/pull/6707

Trac ticket: https://core.trac.wordpress.org/ticket/59595

This ticket was mentioned in Slack in #core by nhrrob. View the logs.


6 months ago

#66 @nhrrob
6 months ago

  • Milestone changed from 6.6 to 6.7

#67 @joemcgill
6 months ago

  • Milestone changed from 6.7 to 6.6
  • Resolution set to fixed
  • Status changed from reopened to closed

This shouldn't be punted to 6.7, because it was reopened to try to improve the original fix. I'm going to close as fixed and we can reopen if there are any reports during RC. Any follow-ups can be in new tickets.

This ticket was mentioned in Slack in #core-performance by clarkeemily. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


6 months ago

@spacedmonkey commented on PR #6879:


6 months ago
#70

@joemcgill On a side now, get_site_transient only result in one database query now. See

https://github.com/WordPress/wordpress-develop/assets/237508/b383b4f1-0b38-4707-ba24-6d979ef33351

Same with get_transient

https://github.com/WordPress/wordpress-develop/assets/237508/cded09ba-4bf4-4e87-940c-438b61da4114

#71 @andrewserong
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi folks, it looks like the updated caching strategy here has resulted in a regression in WP 6.6 (I'm testing in RC 3), where styles that a user sets in global styles for a block within the site editor become stuck on the site frontend.

To reproduce:

  1. Open the site editor on a WP 6.6 RC3 site running Twenty Twenty Four theme
  2. Click the editor preview to go into edit mode
  3. Open the Global Styles sidebar panel on the right hand side
  4. Click Blocks
  5. Select the Button block
  6. Give the Button block a style, e.g. Background color set to a gradient
  7. Save the global styles
  8. Load the site frontend — notice that the styling on the Button is applied
  9. Go back to global styles and give the Button block another style, e.g. large font size
  10. Save the global styles
  11. Load the site frontend and notice that the Button block's styling has not changed — it'll be cached with the previous styles that were originally saved

It sounds like the culprit is the caching strategy used in wp_add_global_styles_for_blocks.

Can this be looked at, or reverted for 6.6?

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


5 months ago
#72

This PR is an initial pass at preventing stale global block styles from being rendered on the frontend in 6.6

#### Problem

Caching was recently added to the global block styles in wp_add_global_styles_for_blocks, this was keyed only on the block name or metadata meaning the cached styles were returned even if the styles had recently changed.

#### Proposed Solution

Cache the global block styles using a hash of the style data as well.

#### Steps to replicate issue on trunk

Copied from #71

  1. Open the site editor on a WP 6.6 RC3 site running Twenty Twenty Four theme
  2. Click the editor preview to go into edit mode
  3. Open the Global Styles sidebar panel on the right hand side
  4. Click Blocks
  5. Select the Button block
  6. Give the Button block a style, e.g. Background color set to a gradient
  7. Save the global styles
  8. Load the site frontend — notice that the styling on the Button is applied
  9. Go back to global styles and give the Button block another style, e.g. large font size
  10. Save the global styles
  11. Load the site frontend and notice that the Button block's styling has not changed — it'll be cached with the previous styles that were originally saved

#### Test Instructions

  1. Checkout this PR and repeat the steps above. This time the styles should update appropriately
  2. Confirm that the cache is still used when styles haven't changed.

Trac ticket: https://core.trac.wordpress.org/ticket/59595

@aaronrobertshaw commented on PR #7017:


5 months ago
#73

I'll be AFK for a couple of days. So please feel free to push changes, rework, or discard this PR. It's only meant to help find a possible solution for 6.6.

@isabel_brison commented on PR #7017:


5 months ago
#74

I gave this a quick test run on my local and this does seem to fix the issue!

#75 @joemcgill
5 months ago

I can also confirm the issue reported by @andrewserong. PR #7017 fixes the issue by including a hash of the block styles data with each block node, which will invalidate the cached styles for each block individually if that data changes.

Rather than splitting the responsibility for cache invalidation to both the hash stored as part of the transient name and also a hash saved as part of the transient data, I propose we consider the approach in PR #6897, which I've just updated so it's ready for review.

The major changes in my alternate approach are:

  1. Uses a hash of the entire raw Theme JSON data for cache invalidation.
  2. Saves the hash as part of the saved data rather than as part of the transient name, to make manual clearing of the transient easier (e.g., wp transient delete wp_styles_for_blocks.
  3. Changes cache storage from using {get|set}_site_transient() to {get|set}_transient, so that each site on a network has an individual cache, rather than storing all caches at the network level when running as a multisite. This addresses a separate concern that I had noticed while reviewing the performance of the current approach (comment:61), but would be good to fix at the same time.

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


5 months ago
#76

  • Keywords has-unit-tests added

This reverts https://core.trac.wordpress.org/changeset/58334, to fix this bug which was discovered late in the 6.6 RC cycle.

Trac ticket: https://core.trac.wordpress.org/ticket/59595

#77 @joemcgill
5 months ago

PR #7026 Is a clean revert of [58334] for 6.6, and we can reconsider the performance fix for 6.6.1.

For context, see the related conversation in Slack.

CC: @audrasjb, @hellofromtonya, and @spacedmonkey for review of the revert.

#78 @hellofromTonya
5 months ago

  • Keywords commit added

Tested using the steps @andrewserong provided comment:71.

  • Can reproduce the issue with 6.6 RC3.
  • Reverting [58334] resolves it.

The revert of [58334] is captured in this PR https://github.com/WordPress/wordpress-develop/pull/7026.

Marking the revert for commit.

#79 @joemcgill
5 months ago

In 58710:

Editor: Revert caching of global styles for blocks.

This reverts [58334] to fix a bug where edits to block styles made in the site editor were not showing in the front end.

Props joemcgill, spacedmonkey, andrewserong, hellofromtonya, audrasjb.
See #59595.

#80 @joemcgill
5 months ago

  • Keywords dev-feedback added

Leaving this open for backporting to the 6.6 branch.

#82 @hellofromTonya
5 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[58710] LGTM for backport to the 6.6 branch.

Note: This revert was discussed in Making WordPress #core slack channel. Consensus and approved by @audrasjb (6.6 Core Tech Lead).

#83 @joemcgill
5 months ago

  • Owner changed from thekt12 to joemcgill
  • Status changed from reopened to assigned

#84 @joemcgill
5 months ago

In 58711:

Editor: Revert caching of global styles for blocks.

This reverts [58334] to fix a bug where edits to block styles made in the site editor were not showing in the front end.

Reviewed by hellofromtonya.
Merges [58710] to the 6.6 branch.

Props joemcgill, spacedmonkey, andrewserong, hellofromtonya, audrasjb.
See #59595.

#85 @joemcgill
5 months ago

  • Keywords changes-requested commit dev-reviewed removed
  • Milestone changed from 6.6 to 6.6.1

After reverting [58334], I'm leaving this open, but moving this to the 6.6.1 milestone to reinstate the cache with a fix for the issue reported by @andrewserong in https://core.trac.wordpress.org/ticket/59595#comment:71.

I'd still propose reviewing PR #6897 as an updated approach and will update that PR to apply cleanly against trunk following the revert.

@joemcgill commented on PR #6879:


5 months ago
#87

Thanks all, I'll update this PR so it applies cleanly after the revert of the original caching so we can try again for 6.6.1

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


5 months ago

@joemcgill commented on PR #6879:


5 months ago
#89

I've refreshed this PR against trunk, which adds back the caching that was reverted in https://core.trac.wordpress.org/changeset/58710 using the updated approach.

I spent some time trying to write a new PHPUnit test that will confirm that the block meta will change whenever the wp_global_styles post is updated but discovered that the query in WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme() ) does not return the same post that is created if you first call WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme(), true ) to create the post. This seems like an unrelated bug so will report it upstream, but wanted to mention in case anyone had any insight into why this might be expected behavior.

#90 @joemcgill
5 months ago

  • Milestone changed from 6.6.1 to 6.7
  • Owner changed from joemcgill to spacedmonkey
  • Type changed from enhancement to defect (bug)

At @spacedmonkey's request, I've created #61679 to focus on reimplementing the cache for wp_add_global_styles_for_blocks() that was reverted here so this ticket can remain focused on optimizing the WP_Theme_JSON::compute_style_properties method.

@joemcgill commented on PR #6879:


5 months ago
#91

Thanks for the review, @costdev. This is ready for another look.

@joemcgill commented on PR #6879:


5 months ago
#92

I spent some time trying to write a new PHPUnit test that will confirm that the block meta will change whenever the wp_global_styles post is updated but discovered that the query in WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme() ) does not return the same post that is created if you first call WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme(), true ) to create the post. This seems like an unrelated bug so will report it upstream, but wanted to mention in case anyone had any insight into why this might be expected behavior.

Gutenberg issue opened, along with a unit test that demonstrates the behavior.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


2 months ago

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


8 weeks ago
#95

Trac ticket:

#96 @joemcgill
8 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 59253:

Editor: Improve performance of WP_Theme_JSON::compute_style_properties

This improves the logic in WP_Theme_JSON::compute_style_properties to address a number of performance issues.

Props spacedmonkey.
Fixes #59595.

#98 @joemcgill
8 weeks ago

Looks like the fix here was committed to Gutenberg during the 6.6 cycle but never merged to WP-dev trunk. I've committed PR #6895 to close this out.

@joemcgill commented on PR #6879:


8 weeks ago
#99

I've refreshed this PR and added an additional unit test to confirm that the cache gets invalidated whenever the global-styles post is updated in the editor.

@joemcgill commented on PR #6879:


8 weeks ago
#100

I've run a set of benchmarks on this locally, which compares 40 runs to the homepage of Twenty Twenty Four.

URL trunk This PR Difference Percentage Difference
Response Time (p10) 70.75 69.21 -1.54 -2.22%
Response Time (p25) 71.51 69.77 -1.74 -2.50%
Response Time (p50) 72.75 70.65 -2.10 -2.97%
Response Time (p75) 74.45 72.05 -2.40 -3.33%
Response Time (p90) 76.19 76.28 0.09 0.12%
wp-before-template (p10) 31.48 31.21 -0.27 -0.86%
wp-before-template (p25) 31.83 31.54 -0.29 -0.92%
wp-before-template (p50) 32.25 32.17 -0.08 -0.25%
wp-before-template (p75) 33.25 33.13 -0.12 -0.36%
wp-before-template (p90) 33.8 34.42 0.62 1.80%
wp-template (p10) 36.39 35.08 -1.31 -3.73%
wp-template (p25) 36.91 35.43 -1.48 -4.18%
wp-template (p50) 37.83 35.96 -1.87 -5.20%
wp-template (p75) 39.14 37.75 -1.39 -3.55%
wp-template (p90) 40.15 39.07 -1.08 -2.69%
wp-total (p10) 68.47 66.85 -1.62 -2.42%
wp-total (p25) 69.49 67.45 -2.04 -3.02%
wp-total (p50) 70.48 68.3 -2.18 -3.19%
wp-total (p75) 71.56 69.64 -1.92 -2.68%
wp-total (p90) 73.91 74.3 0.39 0.53%

#101 @joemcgill
8 weeks ago

In 59256:

Editor: Cache global styles for blocks.

This caches the generated CSS from block nodes in merged Theme JSON data to avoid repeated costly operations required to compute style properties for blocks. The generated CSS is saved to a transient that expires every hour.

This is a follow-up that reimplements [58334], which was previously reverted in [58710].

Props thekt12, spacedmonkey, pereirinha, mukesh27, isabel_brison, oandregal, andrewserong, ramonjd, joemcgill, costdev, aaronrobertshaw, peterwilsoncc.
Fixes #61679. See #59595.

Note: See TracTickets for help on using tickets.