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

wp_get_global_stylesheet: substitute 1-minute transient by non-persistent object cache #3712

Closed
wants to merge 13 commits into from
Closed

wp_get_global_stylesheet: substitute 1-minute transient by non-persistent object cache #3712

wants to merge 13 commits into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Dec 1, 2022

Trac tickets: https://core.trac.wordpress.org/ticket/56910
Related to WordPress/gutenberg#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

@oandregal
Copy link
Member Author

@oandregal oandregal changed the title wp_get_global_stylesheet: substitute 1-minute transient by object cache wp_get_global_stylesheet: substitute 1-minute transient by non-persistent object cache Dec 1, 2022
@oandregal
Copy link
Member Author

Sharing the context I know in case we want to test this in a plugin, first. This function is used in a couple of places:

Gutenberg substitutes the front-end styles here and the editor styles here. I presume the test plugin should do something along those lines.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nitpitcs, but this is looking okay. Will provide profiles in time.

* @access private
*/
function _wp_add_non_persistent_theme_json_cache_group() {
wp_cache_add_non_persistent_groups( 'theme_json' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be here. See this example,

wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) );
. Add theme_json to this list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see wp_cache_add_non_persistent_groups is used in a few places:

I'm not sure where is best. Why should we use load.php instead of having a dedicated place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 this further. It looks like there is no a single place for this kind of thing. If so, I'd rather try to collocate wp_cache_add_non_persistent_groups( 'theme_json' ) with the actual code that uses the cache.

The rationale for doing so is that if we move this to a different part of the codebase it becomes "out of sight, out of mind": we need to resort to human memory to remember, which is far from ideal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for cross-reference and extra clarity. I'm not sure where this should be hooked. Using plugins_loaded was the way we came up with at WordPress/gutenberg#46150 (comment) Is there a better place?

Copy link

@anton-vlasenko anton-vlasenko Dec 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if it's better to move

add_action( 'plugins_loaded', '_wp_add_non_persistent_theme_json_cache_group' );

to src/wp-includes/default-filters.php.
System-wide hooks/actions are supposed to go into that file.
At the same time, _wp_add_non_persistent_theme_json_cache_group() can stay in global-styles-and-settings.php.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the hook to default-filters.php in ff7fd1e I had missed that, thanks for bringing it up!

Copy link
Contributor

@azaozz azaozz Dec 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where this should be hooked. Using plugins_loaded was the way we came up with at WordPress/gutenberg#46150 (comment) Is there a better place?

Mostly depends on whether plugins may need it right away (while loading).

The other question is: does it make sense for this to be removable by plugins? As far as I understand it, no.

As @spacedmonkey points out above the similar wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) ); is hard-coded and happens earlier. Thinking it makes sense for wp_cache_add_non_persistent_groups( 'theme_json' ); to be there too.

Then the hook and the helper function _wp_add_non_persistent_theme_json_cache_group() won't be needed and the only change to the code would be to add theme_json to the first one:

wp_cache_add_non_persistent_groups( array( 'counts', 'plugins', 'theme_json' ) );

Or perhaps even better would be to add the separate wp_cache_add_non_persistent_groups( 'theme_json' ); at the bottom of wp_start_object_cache() and add the nice explanation from the docblock as a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other question is: does it make sense for this to be removable by plugins? As far as I understand it, no.

That's a good point. 🤔

Is there a way for it to happen earlier but still be collocated in this file?

Alternatively, if it is moved elsewhere, how can we make sure people reading this code has clarity about it being non-persistent? I can only think of adding comments in every function that uses that group.

Copy link
Contributor

@azaozz azaozz Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way for it to happen earlier but still be collocated in this file?

Don't think so unless there is a function that is called on including that file (which is generally a bad idea). Also not sure if that is particularly important. Thinking that the best option here is to add it at the bottom of wp_start_object_cache() which seems to be the best place for such settings, and add the explanation as inline comment.

Many modern IDEs (probably all) would show cross references. Having a good explanation of what is being set and why, as in the current docblock, would be more than enough imho. It is easy to find.

can only think of adding comments in every function that uses that group

Not sure that's needed every time the cache group is used. If you're worried that somebody may miss it, perhaps adding an inline comment (pointing to where it is set) the first time it is used in global-styles-and-settings.php would make it even easier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 45969f1

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @oandregal! Can we get some PHPUnit tests brought over from the Gutenberg PR(s)?

@oandregal
Copy link
Member Author

Can we get some PHPUnit tests brought over from the Gutenberg PR(s)?

While WordPress/gutenberg#45679 introduced a test for this behavior, it had to be removed later at WordPress/gutenberg#45993 due to late-removal of a filter that enabled testing this. This is an ongoing conversation happening at WordPress/gutenberg#45912 so, I'm afraid I cannot bring any test here.

@oandregal
Copy link
Member Author

Thanks for the PR @oandregal! Can we get some PHPUnit tests brought over from the Gutenberg PR(s)?

While I couldn't bring any test here for the reasons I've mentioned, I've added a new one at ef450b6. This is not perfect in that it does not test the cache, though it's still useful. When we settle on how to do this going forward, it is easy to tweak.

@oandregal
Copy link
Member Author

@costdev @azaozz @spacedmonkey @anton-vlasenko all feedback has been addressed. What's left for this to be merged?

@ironprogrammer
Copy link

Thanks for the updates, @oandregal! I've tested these latest PR changes from WordPress 5.9.5 and 6.0.3, and it looks good 👍🏻

In response to @oandregal:

all feedback has been addressed. What's left for this to be merged?

While local dev testing shows this to fix the stale cache issue with inline CSS, we should wait for reporters to install the hotfix plugin (or alternate test plugin) to determine if either resolves the issue in non-isolated environments.

Once more feedback has been received, it will inform us of next steps. In particular, it is important to understand how this plays with plugin or host-level caching.

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.

Looking good, @oandregal!

@ironprogrammer: I appreciate the hotfix testing. How long do you estimate we should wait for feedback?

@azaozz
Copy link
Contributor

azaozz commented Dec 14, 2022

LGTM too, just one typo (/** is reserved for docblocks).

@ironprogrammer
Copy link

In response to @mcsf:

@ironprogrammer: I appreciate the hotfix testing. How long do you estimate we should wait for feedback?

That's a good question, and maybe best addressed by one or more core committers. Maybe @azaozz or @spacedmonkey could share some thoughts?

My personal take: While we've seen a general vote of confidence in this from core contributors, we have yet to hear back from reporters. I think it's worth allowing another couple weeks for them to test and provide feedback, since the earliest this could ship is mid-January anyways -- that is, assuming it qualifies as a fix to changes introduced in 6.1.1, and isn't pushed to 6.2 (maybe March).

@azaozz
Copy link
Contributor

azaozz commented Dec 15, 2022

How long do you estimate we should wait for feedback?

...the earliest this could ship is mid-January anyways -- that is, assuming it qualifies as a fix to changes introduced in 6.1.1

Yes, there's some time until WP 6.1.2. This PR is a fix for https://core.trac.wordpress.org/ticket/56970 that most likely will be there. However thinking it can be committed to trunk (WP 6.2-alpha) now, no need to wait. That will also facilitate some more testing (in nightly builds, etc.).

If there's feedback that requires changes, it can be addressed in another ticket/PR.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, in my testing, I am seeing a large performance regression.

Before

Screenshot 2022-12-15 at 23 01 30

After

Screenshot 2022-12-15 at 23 02 03

It is 25ms slower, this is after a merge in trunk.

This should NOT be merged until this is resolved.

I wonder if porting WordPress/gutenberg#46074 and WordPress/gutenberg#46112 over first would help this issue.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@felixarntz
Copy link
Member

felixarntz commented Dec 16, 2022

@spacedmonkey Can you provide some more context on the performance regression? Is it just because the value was already in the object cache before?

Since it is now a non persistent cache, I'd say it's expected that it is now behaving worse than before on second load. However, a 1-minute transient is arguably a less proper solution than a non-persistent cache. And on first page load performance should still be the same.

I'm onboard with the current solution, however I also think we should continue working towards an actually working persistent solution to improve performance for sites that use a persistent object cache.

@oandregal
Copy link
Member Author

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 #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
Copy link
Member Author

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

@hellofromtonya
Copy link
Contributor

Noting: Similar work is being done in wp_get_global_settings() in Trac 57502 and #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
Copy link
Member Author

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.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oandregal Overall this looks almost good to go for me, however I have one potentially critical concern based on what we saw with the related #3556 approach.

@hellofromtonya
Copy link
Contributor

Test Report

As @felixarntz notes #3712 (comment), 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:
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 ✅
Screen Shot 2023-01-26 at 12 59 31 PM

Works as expected ✅

@hellofromtonya hellofromtonya dismissed spacedmonkey’s stale review January 26, 2023 19:23

Dismissing as it was for wp_get_global_settings() - a different PR.

@hellofromtonya
Copy link
Contributor

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

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here match the previously agreed to caching strategy in changeset https://core.trac.wordpress.org/changeset/55086.

From my testing, it does not appear to impact load-styles.php when in the admin area.

From a code point of view, LGTM 👍 Will rely on @felixarntz and @spacedmonkey to confirm performance gains within a single request (since it's non-persistent).

Well done everyone 👏

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hellofromtonya for testing and verifying this is not an issue here. In that case, good to go from my end!

@felixarntz
Copy link
Member

@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
Copy link
Contributor

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

@felixarntz done in d9c0909

Retested in WP Admin. No issues 💹

@ironprogrammer
Copy link

Test Report

Tested with wp-config.php adjustments and steps outlined in #3712 (comment) above.

Patch tested: this PR since d9c0909.

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
Copy link
Contributor

Thanks @ironprogrammer for testing it!

@felixarntz should be okay to commit.

Thank you everyone for your contributions!

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to come in late. Source looks good, I just have a suggestion for an additional assertion.

Comment on lines +280 to +283
switch_theme( 'block-theme' );
$stylesheet_for_block_theme = wp_get_global_stylesheet();
switch_theme( WP_DEFAULT_THEME );

Copy link
Contributor

@peterwilsoncc peterwilsoncc Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
switch_theme( 'block-theme' );
$stylesheet_for_block_theme = wp_get_global_stylesheet();
switch_theme( WP_DEFAULT_THEME );
switch_theme( 'block-theme' );
$post_switch_cache = wp_cache_get( 'wp_get_global_stylesheet', 'theme_json' );
$stylesheet_for_block_theme = wp_get_global_stylesheet();
switch_theme( WP_DEFAULT_THEME );
// This needs to be tested after switching back to the default screen, thus the variable.
$this->assertFalse( $post_switch_cache );

@felixarntz
Copy link
Member

@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
Copy link
Contributor

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

@azaozz
Copy link
Contributor

azaozz commented Jan 27, 2023

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 or it will reset/empty your DB. Usually better to do npm run env:pull (before starting it) to update.
  3. Don't forget to do npm run env:stop at the end.

@oandregal
Copy link
Member Author

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.

@felixarntz
Copy link
Member

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

@felixarntz felixarntz closed this Jan 27, 2023
@azaozz
Copy link
Contributor

azaozz commented Jan 27, 2023

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.

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

Successfully merging this pull request may close these issues.