WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#21434 closed enhancement (fixed)

Don't obliterate the object cache in switch_to_blog()

Reported by: ryan Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4.1
Component: Cache API Keywords:
Focuses: Cc:

Description

switch_to_blog() calls wp_cache_init() which destroys the current $wp_object_cache object and creates a new one. This can be really expensive, especially with the non-persistent default cache. Instead of reinitializing, let's introduce something like wp_cache_switch_to_blog(). The memcached backend already builds the blog id into keys not in global groups, making ::switch_to_blog() very easy to implement. The default cache can do the same (I think it was keyed by blog id once upon a time).

Attachments (10)

21434.diff (6.2 KB) - added by ryan 4 years ago.
First draft, untested
21434.2.diff (7.9 KB) - added by ryan 4 years ago.
21434-ut.diff (2.0 KB) - added by ryan 4 years ago.
21434-ut.2.diff (2.6 KB) - added by ryan 4 years ago.
21434.3.diff (8.7 KB) - added by ryan 4 years ago.
Performance twaeks, wp_start_object_cache() changes
21434.4.diff (9.2 KB) - added by ryan 4 years ago.
Deprecate reset
21434.5.diff (9.6 KB) - added by ryan 4 years ago.
Turn global_groups into an associate array for faster isset checks
21434.6.diff (602 bytes) - added by SergeyBiryukov 4 years ago.
21434.7.diff (1.0 KB) - added by ryan 4 years ago.
Deprecation phpdoc for wp_cache_reset().
21434.8.diff (1020 bytes) - added by ryan 4 years ago.
Remove wp_cache_switch_to_blog() from _deprecated_function(). Note using wp_cache_init() in unit tests in phpdoc.

Download all attachments as: .zip

Change History (23)

#1 @scribu
4 years ago

  • Cc scribu added

@ryan
4 years ago

First draft, untested

@ryan
4 years ago

@ryan
4 years ago

@ryan
4 years ago

@ryan
4 years ago

Performance twaeks, wp_start_object_cache() changes

@ryan
4 years ago

Deprecate reset

@ryan
4 years ago

Turn global_groups into an associate array for faster isset checks

#2 @ryan
4 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [21403]:

Introduce wp_cache_switch_to_blog() and WP_Object_Cache::switch_to_blog() as a lighter/faster way to switch the cache to a new blog id.

Add the blog id to the cache keys for multisite installs.

Use wp_cache_switch_to_blog() instead of wp_cache_init() in switch_to_blog().

Use wp_cache_switch_to_blog() instead of wp_cache_reset() in wp_start_object_cache().

Deprecate wp_cache_reset().

This avoids the many queries needed to re-prime the cache after switch_to_blog() on multisite installs using the default cache backend.

fixes #21434

#4 follow-up: @convissor
4 years ago

The new, efficient code for switching between blogs is an excellent improvement. Thanks.

Can you please elaborate on the reasoning behind deprecating wp_cache_reset() / WP_Object_Cache::reset() and why the deprecation message instructs people to use
wp_cache_switch_to_blog() / switch_to_blog()? The reset() and switch_to_blog() methods serve two separate purposes. (Though some people may have used them in conjunction.)

May I suggest the deprecation message direct people to wp_cache_flush() / flush() instead or have the replacement parameter mention both the flush and switch methods (eg "flush() or switch_to_blog()").

The reason I bring this up is I use wp_cache_reset() in unit tests for my plugin to clean up the environment between tests so data is pulled from the database, not the cache. The existing deprecation message sent me on a wild goose chase in which I found this ticket, spent time composing this message (during which I thought to seek a similar method in the class, which led me to flush()). All in all, a good chunk of time, wasted. If the message had directed me to the similarly functioning flush method, I'd have been on my way in moments.

Oh, and can you please add the @since 3.5.0 tags to the new function / method?

#5 @convissor
4 years ago

  • Cc danielc@… added

#6 @SergeyBiryukov
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

21434.6.diff adds @since for wp_cache_switch_to_blog() and WP_Object_Cache::switch_to_blog().

#7 in reply to: ↑ 4 ; follow-up: @ryan
4 years ago

Replying to convissor:

The new, efficient code for switching between blogs is an excellent improvement. Thanks.

Can you please elaborate on the reasoning behind deprecating wp_cache_reset() / WP_Object_Cache::reset() and why the deprecation message instructs people to use
wp_cache_switch_to_blog() / switch_to_blog()? The reset() and switch_to_blog() methods serve two separate purposes. (Though some people may have used them in conjunction.)

wp_cache_reset() was only ever implemented by the default, built-in cache. The other backends did not implement it. Now that we have switch_to_blog() it is even more useless since core never calls it.

#8 @ryan
4 years ago

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

In [22086]:

Add @since to wp_cache_switch_to_blog(). Props SergeyBiryukov. fixes #21434

#9 in reply to: ↑ 7 @convissor
4 years ago

Replying to ryan:

wp_cache_reset() was only ever implemented by the default, built-in cache. The other backends did not implement it. Now that we have switch_to_blog() it is even more useless since core never calls it.

Deprecating and removing the reset methods is fine. Even if the reset method was never used anywhere inside WP's core, the method was there for the general public to use. And they have, I'm living proof.

The existing deprecation messages point people to the switch_to_blog() method, but that method does not clear the cache.

The deprecation messages needs to be updated, please, to point people to a proper replacement: flush().

#10 @convissor
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#11 @ryan
4 years ago

flush() is not the proper replacement. It does nothing with some backends, especially for multisite.

We don't want to encourage flushing/resetting the cache, we want to encourage switch_to_blog(). For unit tests that use the default backend, consider overwriting the cache object. wp_cache_flush() is fine, though, when testing a specific backend that is known to support flushing for both single and multisite.

I'll add a note to the phpdoc. Patch coming up.

@ryan
4 years ago

Deprecation phpdoc for wp_cache_reset().

@ryan
4 years ago

Remove wp_cache_switch_to_blog() from _deprecated_function(). Note using wp_cache_init() in unit tests in phpdoc.

#12 @ryan
4 years ago

In [22111]:

Update phpdoc and deprecation for wp_cache_reset(). see #21434

#13 @ryan
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.