Make WordPress Core

Opened 21 months ago

Closed 18 months ago

Last modified 18 months ago

#56910 closed enhancement (fixed)

Improve caching in `wp_get_global_stylesheet`

Reported by: spacedmonkey's profile spacedmonkey Owned by: flixos90's profile flixos90
Milestone: 6.2 Priority: normal
Severity: normal Version: 5.9
Component: Editor Keywords: has-patch has-unit-tests gutenberg-merge commit
Focuses: performance Cc:

Description (last modified by flixos90)

Update: Originally this ticket also covered the wp_get_global_styles_svg_filters function which is subject to a similar change, but since that is a separate effort it makes sense to break it out in a separate ticket.

The wp_get_global_stylesheet (and wp_get_global_styles_svg_filters) functions cache data in transient for 1 minute. This means that one high traffic sites, the cache will be warmed and performance improved. But for sites with lower traffic or CDN caching ( where less traffic hits the origin ) this cache will not be warned and performance will suffer. Global styles only change if the owner of the site, makes a change or a theme is changed. These caches should be set forever and correctly invalidated. This will improve performance and mean that front end users who hit a page will not suffer poor performance.

Change History (28)

This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.


21 months ago

#2 @ironprogrammer
21 months ago

  • Type changed from defect (bug) to enhancement

Thanks for this suggestion, @spacedmonkey! Looking forward to seeing an update like this!

Updating to an enhancement, but if I've missed something here, please feel free to provide any additional info and correction.

#3 @andrewserong
20 months ago

Great idea to tweak the caching logic to be more performant!

Global styles only change if the owner of the site, makes a change or a theme is changed

Some of the other times that the output will be changed is when a plugin is installed or updated, or when core files change, since any of these can alter the output of the core theme.json processing.

There are reports over in https://core.trac.wordpress.org/ticket/56970 and https://github.com/WordPress/gutenberg/issues/45713 that sound related to core (and its theme.json file and processing logic) being updated, but new styles from the core theme.json file (and the associated processing logic) not being included. So, I think any updates to the caching logic should ideally factor in WP updates or plugin installations and upgrades, too.

I suppose there's also the possibility of plugins allowing configuration over things that might affect global styles output, too, so personally, I'd still probably lean toward there being some kind of fallback timeout for cache invalidation rather than caching indefinitely, since this area is difficult to predict. However, things like plugin installations or updates happen pretty infrequently, so I agree that the timeout could be much longer than one minute.

#4 @spacedmonkey
20 months ago

  • Milestone changed from Awaiting Review to 6.2

#5 @spacedmonkey
18 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

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


18 months ago
#6

  • Keywords has-patch has-unit-tests added; needs-patch removed

Trac tickets: https://core.trac.wordpress.org/ticket/56910
Related to https://github.com/WordPress/gutenberg/issues/45171

Backports the relevant parts of these PRs:

This PR:

  • provides a fix to an issue reported for 6.1.1 (see trac ticket and gutenberg issue)
  • backports part of the work of introducing object cache to theme.json APIs (see)

Props @mmtr @felixarntz @spacedmonkey @aristath

@hellofromTonya commented on PR #3712:


18 months ago
#7

As @felixarntz pointed out, this PR needs a rebase and potentially a rebuild of the individual commits to remove what is no longer relevant.

That said, there's an open discussion still happening on the caching solution (due to fatal errors in the admin area) (See the Trac ticket https://core.trac.wordpress.org/ticket/56975). Until a solution is committed, might be best to wait on the rebase.

@oandregal commented on PR #3712:


18 months ago
#8

Rebased and removed the no longer relevant parts. Changes for src/wp-includes/load.php, src/wp-includes/ms-blogs.php, tests/phpunit/includes/abstract-testcase.php are still relevant here as they were reverted from core at https://github.com/WordPress/wordpress-develop/pull/3864

I was unable to reproduce the bug raised at https://core.trac.wordpress.org/ticket/56975#comment:32 in this PR. I'll need to go back there and see if I can have some testing steps. I want to understand how to reproduce to either introduce the fix in this PR directly or that we don't block this from landnig if it doesn't.

As I shared in other places, this week I'm unable to contribute for more than half an hour here and there. So, go ahead when you feel is ready and don't wait for my feedback. Or ping me via DM if there's something I must absolutely review.

@oandregal commented on PR #3712:


18 months ago
#9

@felixarntz @oandregal I have created a PR for making SVG function use this same caching. https://github.com/oandregal/wordpress-develop/pull/1

That's a good next step, thanks for working on it. As it is now, it is based on this PR. I'd recommend preparing a separate PR instead as they are unrelated changes (though, sensibly, going in the same direction). Ideally, that work happens in Gutenberg first, so it is tested by users before merging it into core.

#10 @hellofromTonya
18 months ago

  • Keywords gutenberg-merge added

Marking gutenberg-merge as some (or all) of the work here is to backport/copy from Gutenberg PR(s) into Core (such as PR 3712. This keyword helps to track backports.

@hellofromTonya commented on PR #3712:


18 months ago
#11

Noting: Similar work is being done in wp_get_global_settings() in Trac 57502 and https://github.com/WordPress/wordpress-develop/pull/3789. That scope of work captures the latest conversations and consensus. I'm thinking it might be worth landing PR #3789 first, then rebasing and updating this PR to remove the duplicate code, and then copy the code patterns from it.

@felixarntz @spacedmonkey @oandregal Do you agree?

@oandregal commented on PR #3712:


18 months ago
#12

I see two blocking reviews, though I think I've addressed all the feedback. Please, let me know if I should look at anything to unblock this.

@hellofromTonya commented on PR #3712:


18 months ago
#13

# Test Report

As @felixarntz notes https://github.com/WordPress/wordpress-develop/pull/3712#discussion_r1088143732, need to test the PR in the WP Admin area to determine if a fatal error happens from not having wp_cache_*() functions available when load-styles.php runs.

## Testing Instructions

  • Step 1: Added the following constants to wp-config.php:

{{{php
define( 'CONCATENATE_SCRIPTS', true );
define( 'WP_DEBUG', false );
define( 'SCRIPT_DEBUG', false );
}}}

  • Step 2: Pull or apply this PR
  • Step 3: Start your localhost environment.

If using Docker, then do:

npm install
npm run build:dev
npm run env:start
npm run env:install
  • Step 4: Log in and open the WP Admin area.

Does a fatal error occur?
Is the load-styles.css stylesheet loading?

If no, navigate through the screens. Does a fatal error occur?
If no, then this PR is not impacting load-styles.php

## Testing Results

Env:

  • Localhost: Docker with npm
  • OS: macOS
  • Browser: Chrome, Firefox, and Edge
  • WP Version: trunk
  • Plugins: none
  • Theme: TT3

No fatal error ✅

Opened the Network tab in Dev Tools: yes, load-styles.php is being requested and has a 200 status ✅
https://i0.wp.com/user-images.githubusercontent.com/7284611/214925556-7d6eaa5e-3fc5-48aa-9846-3ebd508f2f25.png

Works as expected ✅

@hellofromTonya commented on PR #3712:


18 months ago
#14

Does this cache need to be reset in wp_clean_theme_json_cache()?

#15 @flixos90
18 months ago

  • Description modified (diff)
  • Keywords commit added
  • Owner changed from spacedmonkey to flixos90
  • Status changed from assigned to reviewing
  • Summary changed from Improve caching in `wp_get_global_stylesheet` and `wp_get_global_styles_svg_filters` functions to Improve caching in `wp_get_global_stylesheet`

@spacedmonkey I am changing this ticket to focus just on the wp_get_global_stylesheet() function, since https://github.com/WordPress/wordpress-develop/pull/3712 is pretty much ready, and those two efforts are separate, even though the work is very similar.

Can you open a separate ticket to make the change to wp_get_global_styles_svg_filters()? I see that work is still pending a Gutenberg PR https://github.com/WordPress/gutenberg/pull/47460, and only then we can backport it to core, so it may take a bit longer.

@flixos90 commented on PR #3712:


18 months ago
#16

@hellofromtonya

Does this cache need to be reset in wp_clean_theme_json_cache()?

Good catch, I missed that 🙃

Can you please add the matching wp_cache_delete call to that function?

@hellofromTonya commented on PR #3712:


18 months ago
#17

Can you please add the matching wp_cache_delete call to that function?

@felixarntz done in https://github.com/WordPress/wordpress-develop/pull/3712/commits/d9c0909951b2a894d137324c1082a50167a2d114

Retested in WP Admin. No issues 💹

@ironprogrammer commented on PR #3712:


18 months ago
#18

## Test Report
Tested with wp-config.php adjustments and steps outlined in https://github.com/WordPress/wordpress-develop/pull/3712#issuecomment-1405464211 above.

Patch tested: this PR since https://github.com/WordPress/wordpress-develop/commit/d9c0909951b2a894d137324c1082a50167a2d114.

### Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 12.6.2
  • Browser: Safari 16.2, Mozilla Firefox 109.0
  • Server: nginx/1.23.3
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Theme: twentytwentythree v1.0
  • Gutenberg: tested both enabled/disabled

### Actual Results

  • ✅ No fatal errors occurred while navigating through or refreshing WP admin.
  • ✅ Requests to load-styles.php returned successfully, and CSS loaded in WP admin.

@hellofromTonya commented on PR #3712:


18 months ago
#19

Thanks @ironprogrammer for testing it!

@felixarntz should be okay to commit.

Thank you everyone for your contributions!

#20 @flixos90
18 months ago

A final brief performance benchmark on this one:

  • The use of WP cache API makes the code more performant than when using a transient, for when the value for the transient is not set (~4.5ms compared to ~8.8ms).
  • When the transient has a value set, it is of course much faster than the WP cache API since we are now using a non-persistent cache (~0.6ms compared to ~4.5ms).

This is expected since the transient is cached between requests, making subsequent lookups much faster - however it's only cached for 1 minute, which is a hacky approach to get some sort of "semi-persistent" cache for something where we haven't yet figured out cache invalidation.

Overall the function is called only 1 time per regular page load, so the non-persistent cache here does not improve performance. However, it still makes more sense than the transient due to the above. As we are working towards a persistent cache solution, this is a step in the right direction, even though performance-wise it is a zero-sum game.

#21 @flixos90
18 months ago

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

In 55148:

Editor: Use a non-persistent object cache instead of transient in wp_get_global_stylesheet().

This changeset is part of a greater effort to enhance the caching strategy for theme.json based data. Similar to [55138], the cache is currently ignored when WP_DEBUG is on to avoid interrupting the theme developer's workflow.

Props oandregal, spacedmonkey, hellofromtonya, flixos90, ironprogrammer, azaozz, aristath, costdev, mcsf.
Fixes #56910.

@flixos90 commented on PR #3712:


18 months ago
#22

@peterwilsoncc Apologies, this feedback came a bit too late, I had already committed this in https://core.trac.wordpress.org/changeset/55148.

If it's crucial, we can do a follow up commit?

@peterwilsoncc commented on PR #3712:


18 months ago
#23

@felixarntz I think it should be fine without. I noticed my terrible timing ;)

@azaozz commented on PR #3712:


18 months ago
#24

Just FYI: ideally testing should happen when WP runs from /build as it is closest to "production" (concatenated and minified js and css, etc.). Docker/wp-env can be set to run from there. The testing instructions are slightly different:

  1. WP_DEBUG and SCRIPT_DEBUG should not be set (or commented out) in wp-config.php.
  2. In the terminal:
    • npm run build or just grunt. No need to do npm install as Grunt will run it if needed.
    • LOCAL_DIR=build npm run env:start to start from /build
    • npm run env:install only if this is the first time using wp-env here
  3. Don't forget to do npm run env:stop at the end.

@oandregal commented on PR #3712:


18 months ago
#25

I was able to reproduce with the steps Tonya provided.

@azaozz I tried to use the steps you suggested, but npm run build doesn't work for me:

Running "verify:old-files" task
Warning: build/wp-includes/blocks/heading/editor.css should not be present in the $_old_files array. Use --force to continue.

Aborted due to warnings.

I tried a few things, including clearing the build folder or passing --force to the build command. None worked.

---

It seems this PR has been committed, so I guess it can be closed/deleted.

#26 @spacedmonkey
18 months ago

@flixos90 Created a breakout ticket here #57568

@flixos90 commented on PR #3712:


18 months ago
#27

Thanks all for the additional feedback. No actionable change in terms of committing per the above, so I'll indeed close this for now.

@azaozz commented on PR #3712:


18 months ago
#28

tried to use the steps you suggested, but npm run build doesn't work

Uhh, sorry, this is a bug in core. Generally when you build to /src, it leaves the source in a "dirty" state. Trying to actually build to /build copies some of these left-over files and the build fails.

There's a ticket for that already, will try to fix it soon.

Note: See TracTickets for help on using tickets.