Delete Cache by Group

[pbearne]

WordPress/performance#38

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

[spacedmonkey]

If this to live in core there needs to be a compat function found in core in cache-compat.php. This function would need to work for ALL object cache implementation, including and not limited to memcached.

I personally think this is going be nearly impossible, as it is hard to know the implementation details of every object cache. But without that compatible layer, this can not live in core 😞

[tillkruss]

This function would need to work for ALL object cache implementation, including and not limited to memcached.

It's a FAQ and the suggested approch is using prefixes, which I believe wp-memcached is already using heavily.

[spacedmonkey]

Please use remove test for wp_cache_get_linked_meta and revert changes to comments and spacing that are not required for this change.

[spacedmonkey]

If this is going to make it into core, then we are going to need some constant, like WP_ALLOW_CACHE_GROUPS. Drop-in providers, will have to define this. That way, developers / core can work out if we can even use this function.

Thoughts?

[tillkruss]

If this is going to make it into core, then we are going to need some constant, like WP_ALLOW_CACHE_GROUPS. Drop-in providers, will have to define this. That way, developers / core can work out if we can even use this function.

Reminds me of get_theme_support() and add_theme_support().

If we default to flushing the entire cache (in the compatibility method), can you give an example how and where WP_ALLOW_CACHE_GROUPS would be used?

[pbearne]

I am not sure we do need the global WP_ALLOW_CACHE_GROUPS as I test for a flush_group function
method_exists( $wp_object_cache, 'flush_group' )
and return false if not available so the calling code and handle it

[tillkruss]

Revert debug changes.

[dd32]

This is a dangerous fallback for this method, having plugin code expecting to flush a specific cache group(s) and then flushing the entire cache is not going to end well.

Additionally, IMHO it's not a _doing_it_wrong() E_USER_DEPRECATED, possibly a E_USER_NOTICE level warning instead though?
(I say this, because with many object caches, it's not possible to flush a specific group, as key-value stores often don't have a "flush keys with this prefix" - Memcache object cache dropin gets around this by prefixing the timestamp to keys, so that it can just switch to a new set of prefixes when flushed)

[spacedmonkey]

The worry here, is that if you call this function can expect and clear your group and it doesn't we need to warn developer this is happening. Maybe we should return a WP_Error object?

So you could do this in your own code.

$result = wp_cache_flush_group( 'posts' );
if( is_wp_error( $result ) ) {
    wp_cache_flush();
}

But give devs more control here.

[tillkruss]

I like the WP_Error approach.

[dd32]

I think return false would suffice myself if it wasn't going to flush everything.. "Clear this cache group, sorry, that failed.".

But as mentioned in #2368 (comment) variants whose cache server that don't support clearing by prefix, could rotate a prefix to flush the group.

[spacedmonkey]

So that means that false, could mean a lot of things. Unsupported and the cache group flush failed. That is unclear for developers that using this function, why the failure.

[tillkruss]

Some feedback from @felixarntz:

What I would suggest is considering making support of this more explicitly optional. For example

• Not put the flush_group() method in the base class, or alternatively put it in the base class as a no-op that just returns false.
• Introduce wp_cache_flush_group(), but explicitly document it that it may not be supported and therefore returns a boolean to indicate success, which is critical to check here (other than maybe on other cache functions).
• In the function, depending on my first point, either check if the method exists on the cache implementation and if not return false, or otherwise just call the method and return through its boolean result.

Alternatively, we could make it even more explicit to clarify that developers need to be aware that this may not work, e.g. by having the boolean function just as a check that has to be explicitly called (e.g. wp_cache_can_flush_group()) before calling wp_cache_flush_group(). The latter could cause a _doing_it_wrong() if not supported or throw an exception.

So TLDR whichever way we go, I definitely think it should be explicitly built and marked as optional.

[spacedmonkey]

@felixarntz's comments here reflect statement I have made throughout this code review. These needs to be a way for developers to know if running this function is going to work. They have to know if the object cache implement supports it.

I recommend adding a const, like WP_CACHE_SUPPORTS_GROUP_FLUSH, so developers of object cache drop-ins can "opt-in" to support this feature. If this const is not defined, then, developers know not to use this function.

See these simplier examples.

[tillkruss]

I'd suggest is using WP_OBJECTCACHE_* since WP_CACHE refers to WP's "page caching" mechanism.

[peterwilsoncc]

I left a half thought in a Slack thread that I'll leave in full detail here so it doesn't get lost.

For a lot of groups currently, WP uses the last_changed cache key to determine when the group was last changed. It acts as a faux group flushing mechanism.

wordpress-develop/src/wp-includes/post.php

Line 7406 in 8277055

wp_cache_set( 'last_changed', microtime(), 'posts' );

For drop-ins that support the flush_group() method, WP could call that and trust the drop in developer to have checked their persistent cache supports it.

For drop-ins that do not include the method, WP could include something similar to polyfill the feature.

// Within caches
if ( ! method_exists( ... )  && 'wp_group_suffiixes' !== $group ) {
   $group = $group . '::' . wp_cahce_group_suffix( $group );
}

// Function
function wp_cahce_group_suffix( $group, $update = false ) {
  $suffix = wp_cache_get( $group, 'wp_group_suffiixes' );
  if ( ! $update ) {
    return $suffix;
  }
  
  $new_suffix = $suffix;
  while ( $new_suffix === $suffix ) {
     $new_suffix = wp_generate_password( 8, true, true );
  }

  return $new_suffix;
}

At this stage, I'd call this a thought bubble and it absolutely needs further thinking through. One of the issues Dion mentioned in Slack is that there is potential to hit key length issues in some persistent caches.

It's also possible this is a terrible idea.

[spacedmonkey]

I think the discussion for this ticket should move back to the original tickets. GH is a code review tool and not best designed for this.

[tillkruss]

The new constant needs to be added to the beginning of src/wp-includes/cache.php. I can't do that in the PR unfortunatly.

define( 'WP_OBJECT_CACHE_SUPPORTS_GROUP_FLUSH', false );

[spacedmonkey]

There seems to be unit tests failing now and coding standards need fixing.

[tillkruss]

Suggested change

	$this->assertTrue( $results );
	$this->assertFalse( wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_group: group-test should return false' );
	$this->assertSame( $val, wp_cache_get( $key, 'group-kept' ), 'test_wp_cache_flush_group: group-kept should still contain my-val' );
	if (WP_OBJECT_CACHE_SUPPORTS_GROUP_FLUSH) {
	$this->assertTrue( $results );
	$this->assertFalse( wp_cache_get( $key, 'group-test' ), 'test_wp_cache_flush_group: group-test should return false' );
	$this->assertSame( $val, wp_cache_get( $key, 'group-kept' ), 'test_wp_cache_flush_group: group-kept should still contain my-val' );
	} else {
	$this->assertWPError( $results );
	$this->assertSame( 'unsupported', $results->get_error_code() );
	}

[aksld]

Hello,

What is the next step ?

Do you need help for something ?

[tillkruss]

@aksld The memcache object cache used by the tests needs support for this feature. Likely by using prefixes that increment.

[pbearne]

closed by #2969

[spacedmonkey]

Committed