Skip to content

Transient based caching#6219

Closed
kt-12 wants to merge 3 commits intoWordPress:trunkfrom
10up:feature/cache-add-transient-support
Closed

Transient based caching#6219
kt-12 wants to merge 3 commits intoWordPress:trunkfrom
10up:feature/cache-add-transient-support

Conversation

@kt-12
Copy link
Copy Markdown
Member

@kt-12 kt-12 commented Mar 4, 2024

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 4, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props thekt12, joemcgill.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 4, 2024

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Copy link
Copy Markdown
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Oh, this is interesting. I'm going to think on it a bit.

My knee-jerk reaction is that making such a big change the the main wp_cache_ functions, is likely not the right approach, but there is something kind of clever about trying to make it easier to optionally back caches in non-persistent environments with transients without a lot of repeated code.

I wonder if it would be easier to handle from the opposite perspective, given that transients already get saved to external object cache when available? I think the main things that are missing for transients are the ability to set a cache group besides 'transient' which is where they all get stored now, and a way to ensure transients that are stored in the DB options table get flushed whenever the cache group they belong to gets flushed.

@kt-12
Copy link
Copy Markdown
Member Author

kt-12 commented Mar 4, 2024

Oh, this is interesting. I'm going to think on it a bit.

My knee-jerk reaction is that making such a big change the the main wp_cache_ functions, is likely not the right approach, but there is something kind of clever about trying to make it easier to optionally back caches in non-persistent environments with transients without a lot of repeated code.

I wonder if it would be easier to handle from the opposite perspective, given that transients already get saved to external object cache when available? I think the main things that are missing for transients are the ability to set a cache group besides 'transient' which is where they all get stored now, and a way to ensure transients that are stored in the DB options table get flushed whenever the cache group they belong to gets flushed.

The reason I was inclined to the changes here is :

  • Consistency - mainly in naming convention. If we have a consistent name, in future it will be easy to implement flushing.
  • Secondly, we will have too many filters to manage and a lot of repetitive code.

I was analysing both the functions wp_cache_ side functions and transient side functions.
The way I have written this code is transient cache is conditionally enabled only when $transient_cache flag is set and object_cache is not enabled. So older code will work the same way. This shift of logic applies only to places where we want that logical check, so it's easy to ensure BC.

The problem I felt with making changes to transient side functions is that;

  • We will have to be extra careful while adding support for groups in caching code as there might be some sites reliant on object cache within the transient functions. Ensuring BC might be a bit more complex here as compared to wp_cache base approach.
  • Handling the expiry situation might become complex (I cannot say for sure though) as we will have to add a bunch of code to differentiate between regular transient and transient for caching.
  • We will have to replace wp_cache_ at all the places with transient functions, which might get confusing. Also, key name logic will be bit more complex inside transient functions.

Lastly, I felt it was more of a caching problem than a transient problem (even though they mean the same), so I added my code to wp_cache_.

@joemcgill
Copy link
Copy Markdown
Member

Thanks for explaining your thinking in a bit more detail. Still, I don't think caching block theme files warrants a major modification of the Cache API to make use of Transients as a first class way to provide persistence in non-persistent environments. I think doing so confuses the responsibilities of these two APIs somewhat. For context, here's how I would describe the purpose of both APIs:

Cache API:

  • Primarily stores computed data in memory during a single request so it can be reused without being recomputed in the same request
  • Is non-persistent by default, but can optionally can be made persistent with the use of an external object cache

Transients API:

  • Primarily provides a standard way to persist some cached data between requests.
  • Enhances the Options API with a way to allow cached data saved to options to expire to avoid DB bloat.
  • In environments that support persistent object caches, transients will be offloaded to the cache system and not be in the DB at all.

Realistically, for sites that want cached data to persist, the solution is to install a persistent object cache. Backing these caches with transients is usually redundant and unnecessary. For data that should persist, regardless of an external object cache, the data should be stored in a transient, which will get offloaded to an external object cache if available. For block template files, if we want to ensure persistence, we could simply prioritize the use of transients. However, since we want to provide a way for environments with persistent caches to flush these caches using cache groups, we will need to manually check for whether wp_using_ext_object_cache() returns true, and if so, use the Cache API with the theme_files group.

@joemcgill
Copy link
Copy Markdown
Member

@kt-12 let's close this in favor of #6137, which seems much closer to how we would want to implement theme file caching.

@joemcgill joemcgill closed this Mar 11, 2024
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.

2 participants