Make WordPress Core

Opened 6 months ago

Last modified 5 days ago

#59595 assigned enhancement

Improve performance of WP_Theme_JSON::compute_style_properties

Reported by: spacedmonkey's profile spacedmonkey Owned by: thekt12's profile thekt12
Milestone: 6.6 Priority: normal
Severity: normal Version: 5.8
Component: Editor Keywords: has-patch changes-requested
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 6 months ago.
Request overview before
Screenshot 2023-10-24 at 22.38.16.png (1.1 MB) - added by pereirinha 6 months ago.
Function details before
Screenshot 2023-10-24 at 22.37.29.png (1.2 MB) - added by pereirinha 6 months ago.
Request overview after
Screenshot 2023-10-24 at 22.38.19.png (1.1 MB) - added by pereirinha 6 months ago.
Function details after

Change History (32)

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


6 months ago

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


6 months ago

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


6 months ago

@pereirinha
6 months ago

Request overview before

@pereirinha
6 months ago

Function details before

@pereirinha
6 months ago

Request overview after

@pereirinha
6 months ago

Function details after

#4 @pereirinha
6 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.


6 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.


6 months ago

#7 @mukesh27
6 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:


6 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:


6 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.


6 months ago

@mukesh27 commented on PR #5567:


6 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.


4 months ago

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


4 months ago

@pereirinha commented on PR #5567:


4 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:


4 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.


3 months ago

@isabel_brison commented on PR #5567:


3 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:


3 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:


3 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
3 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.


3 months ago

#22 @joemcgill
2 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.


3 weeks ago

#24 @joemcgill
3 weeks 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.


6 days ago
#25

Trac ticket: 59595

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

#26 @thekt12
6 days 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.

Last edited 6 days ago by thekt12 (previous) (diff)

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


6 days ago

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


5 days ago

Note: See TracTickets for help on using tickets.