Use wp_cache_add_global_groups() so cache is applicable throughout multisite

[wpdennis]

In a multisite installation with object cache BP_Messages_Thread::get_inbox_count() will be cached once per blog. After reading a message, only the main blog cache entry gets invalidated. All other blogs are showing wrong numbers.

Maybe the cache key "bp_messages_unread_count" should be added as a global group:

wp_cache_add_global_groups('bp_messages_unread_count');

[wpdennis]

Digging a little bit deeper:

shouldn't all/most cache groups be added as global groups on a multisite installation? Like "bp_last_activity", all the "xprofile_data_*", "bp_group_invite_count" etc.

They should contain the same value on each blog and a correct cache invalidation isn't possible with local cache groups.

[r-a-y]

Good catch, wpdennis.

The one concern I have is sites using a WP multi-network with BuddyPress via a plugin like BP Multi-Network or BuddyPress Multi-Network. Not many people are using WP multi-networks with BuddyPress (I know I haven't!), but for those that are, this would probably cause tons of cache issues.

We could probably refactor the existing bp_core_add_global_group() function to add the additional cache groups and add a filter to bail out of registering these groups if necessary.

Would need some testing with one or both of those BP multi-network plugins.

[wpdennis]

Thanks for the confirmation r-a-y.

I have no experience with mutli-network plugins, but would'nt the user specific values (profile data, last activity, inbox) be the same in each network, since it's one user base? So they would benefit from global cache groups too, wouldn't they?

But the ability to bail out sounds good anyway.

[DJPaul]

I think this is going to be a bit of a can of worms, but it is something we should talk about in terms of how BuddyPress does/should work across multisite.

[johnjamesjacoby]

Replying to DJPaul:

I think this is going to be a bit of a can of worms, but it is something we should talk about in terms of how BuddyPress does/should work across multisite.

There are actually three potential cache scopes to consider:

• Global
• Network
• Blog

Unfortunately, most traditional cache plugins only consider $global_prefix and $blog_prefix without support for a $network_prefix group or bucket. These prefixes typically use the $table_prefix global (traditionally defined in wp-config.php) for namespacing the difference between global and blog scopes, and since that doesn't change between networks, neither do the cache buckets.

See the snippets below for a typical approach:

// Assign global and blog prefixes for use with keys
if ( function_exists( 'is_multisite' ) ) {
	$this->global_prefix = ( is_multisite() || defined( 'CUSTOM_USER_TABLE' ) && defined( 'CUSTOM_USER_META_TABLE' ) ) ? '' : $table_prefix;
	$this->blog_prefix = ( is_multisite() ? $blog_id : $table_prefix ) . ':';
}

Combined with:

public function buildKey( $key, $group = 'default' ) {
	if ( empty( $group ) )
		$group = 'default';

	if ( false !== array_search( $group, $this->global_groups ) )
		$prefix = $this->global_prefix;
	else
		$prefix = $this->blog_prefix;

	return preg_replace( '/\s+/', '', WP_CACHE_KEY_SALT . "$prefix$group:$key" );
}

The Global scope will traditionally include:

• users, userlogins, usermeta, site-options, site-lookup, blog-lookup, blog-details, & rss WordPress core buckets
• The bp bucket for BuddyPress when network activated on a 1 network installation, added via bp_core_add_global_group().

The Blog scope includes basically everything else.

• Note that if bp_is_multiblog_mode is enabled, our global buckets should revert to blog buckets, though they currently do not.

---

The conclusion I've come to is three-fold:

• What we consider to be "global" needs an audit. (I.E. Should bp_activity_sitewide_front really be global, or is bp_activity more appropriate?)
• The bp global cache bucket needs a filter on it, so a network ID can be added in multi-network installations, and so they can be downgraded to blog buckets in multi-blog mode.
• Our non-global cache keys may also need filters, as they are presumed to always be per-blog though they are actually per-network. I have not done any work towards testing this theory.

[johnjamesjacoby]

Proof of concept

[johnjamesjacoby]

The above patch likely won't apply cleanly to trunk (it's from the Flox repository.) Since Flox is multi-network, it's as good place as any to let this idea soak. The patch does a few things:

• Introduces bp_core_get_global_cache_key() with a filter to allow changing bp to anything else.
• Introduces bp_core_get_global_cache_groups() with a filter to allow plugins or core components to plug into the global cache groups array. In the future, I could see automatically adding cache groups as part of the BP_Component class.
• Replaces hard-coded references of bp with bp_core_get_global_cache_key().

I'll monitor memcached and report back the results in a few days.

[johnjamesjacoby]

Revised BP Multi Network mu-plugin, based largely on the original by Boone & Ron

[boonebgorges]

johnjamesjacoby - The wrapper functions with filters look good to me. I'd imagine there's a very slight performance hit from running through apply_filters(), but much less than not caching at all :)

I also agree that we need to have a clearer sense of global vs network vs blog buckets. Please continue to update us here or elsewhere with your thoughts on this, and we can work on codifying it in our documentation and in the codebase.

[r-a-y]

5733.patch looks okay, but instead of doing a find and replace for 'bp' everywhere, perhaps we should create wrapper wp_cache_X() functions and replace all wp_cache_X() calls with bp_cache_X(). This is so we just have to change the global cache group in one place.

The BP Multi Network plugin also reminds me of how our notification meta keys suck :) I've got one suggestion in the plugin - why not mirror all user meta keys that bp_update_user_meta() calls? Chances are that this would be useful for any plugin calling bp_update_user_meta().

---

The conclusion I've come to is three-fold

What we consider to be "global" needs an audit. (I.E. Should bp_activity_sitewide_front really be global, or is bp_activity more appropriate?)

Some really good thoughts here. I think we definitely need a global group solely for counts as wpdennis initially brought up.

[boonebgorges]

Looks okay, but instead of doing a find and replace for 'bp' everywhere, perhaps we should create wrapper wp_cache_X() functions and replace all wp_cache_X() calls with bp_cache_X(). This is so we just have to change the global cache group in one place.

But most of our caching doesn't actually use the global group - it uses object-type-specific cache groups (like 'bp_activity'). Creating a wrapper in one place suggests creating a wrapper everywhere, and I don't want to go wrapper-crazy without good argument :)

The BP Multi Network plugin also reminds me of how our notification meta keys suck

Truth.

[r-a-y]

But most of our caching doesn't actually use the global group - it uses object-type-specific cache groups (like 'bp_activity'). Creating a wrapper in one place suggests creating a wrapper everywhere, and I don't want to go wrapper-crazy without good argument :)

For sure, but we can default to one global group, which should handle the majority of spots.

For those that need new groups, we add it in the additional parameter - eg. bp_cache_set( 'bp_messages_unread_count', $count, 'bp_counts' );

You're right about how the majority of our cache calls are object-specific cache groups. Bah! Forget I said anything!

Creating a wrapper in one place suggests creating a wrapper everywhere, and I don't want to go wrapper-crazy without good argument :)

I see your point here as well :)

[johnjamesjacoby]

I'm usually all for wrapper functions for our own convenience, but because caching is already a bit of a black box, and because we're still moving things around, let's not think about that until we feel we've come to some conclusions.

I may change my mind later, but right now I really like the idea of coming up with a convention for defining the scope of cache groups, and having components register them via BP_Component. This likely will not be totally linear, as components can have several scopes, but we'll learn more once we fully audit our current usages.

Regarding the array of user-meta keys in the BP Multi Network plugin, since we don't currently register meta (using register_meta() or otherwise) we don't have any central arrays of component meta-keys to make this easy anywhere. bbPress does this for users ​here and options
​here but not anywhere for post-meta yet (and also does not use register_meta as it wasn't finalized for WordPress 3.3 when bbPress development started.) Hence, we're stuck cherry-picking the appropriate meta-keys, and it's completely possible I missed some, or newer ones exist that I didn't add.

[johnjamesjacoby]

I think we definitely need a global group solely for counts as wpdennis initially brought up.

The problem then becomes purging the entire group becomes costly for no reason. And because lists of items are linked to counts, caching them in separate groups could result in a mismatch of results VS recalculated count. I'd like to keep counts together with their components, unless there's a compelling performance-oriented reason to do otherwise.

[johnjamesjacoby]

We also have wp_cache_add_non_persistent_groups() at our disposal for things like counts. The function name is a little nondescript, but it caches results for the current page-load and skips the persistent cache. WordPress currently uses this for a few different groups:

• 'comment'
• 'counts'
• 'plugins'
• 'themes', with a wp_cache_themes_persistently filter to switch it to persistent

This would allow us a bit of wiggle-room for adding caching without worrying too much about invalidation, at least not right away. One odd but good example of using this is WordPress comments. The 'comment' cache group is non-persistent, and all comments queries cache to that group. This means comment objects are not persistent, which was a pretty surprising realization. Helper functions like clean_comment_cache() and update_comment_cache() are used to update or invalidate multiple objects, and a last_changed hashed key is used to separate result sets, like: $key = md5( serialize( wp_array_slice_assoc( $this->query_vars, array_keys( $defaults ) ) ) );

An approach like above makes more sense than our approach with bp_activity_sitewide_front for example. That custom front-page cache approach is a fine work-around for not having any caching, but does make the assumption that the front-page of the activity directory is the same for everyone. Once any parameters change, we stop caching queries, which leaves us room for improvement.

Conversely for the posts group, it's non-global, and persistent, which seems more accurate for our components and their objects and queries. There are helper functions for building cache keys (similar to my proposal above) and objects are expected to be cached and deleted rather frequently, up to and including functions like wp_insert_post() that delete the new post's cache and then immediately prime the cache with a call to get_post().

Still sifting through WordPress's innards, but we've always known there would be pretty enormous performance gains once we get our cache usages correct, and it's nice to reconfirm that.

[johnjamesjacoby]

Forgot to mention, running my attached patch on several BuddyPress networks has yielded positive and expected results. Having the ability to manipulate the global cache key helps separate the groups into separate global buckets, and completely avoids the cross-talk I was seeing earlier.

It does, however, beg us to ask a separate question, which is: what constitutes a truly global cache group for BuddyPress? Maybe surprisingly, network activating BuddyPress isn't enough. Even though we (safely) assume BuddyPress's tables should be created as 'global tables', that doesn't immediately mean the installation type mirrors that, or that cached data across networks should share data, cached or otherwise.

WordPress's global cache groups are almost always truly global groups. In the event completely separate groups are wanted, the WP_CACHE_KEY_SALT constant (dangerously) exists to allow even users to have separate groups for each global and blog prefix.

In these (somewhat edge-case) scenarios, having the ability to unhook and/or override bp_core_add_global_group() is helpful, as is the ability to filter bp_core_get_global_cache_key at runtime. The application layer doesn't care what other global groups exist or are/were/aren't registered, so we can safely toggle them.

TL;DR: My proposed patch is working great, is an incremental improvement over what we have today, and helps us start to map out our cache group vs. installation scope relationships.

[boonebgorges]

johnjamesjacoby - I notice that your patch doesn't touch some of the other more specific cache buckets in BP - say ​https://buddypress.trac.wordpress.org/browser/tags/2.1.1/src/bp-groups/bp-groups-classes.php#L188. I assume that's because the cache keys themselves in those buckets are generally numeric, which would cause clashes between object types. (This mirrors WP's conventions for cache keys.) So, in order to make the system more flexible, I wonder if it makes sense to have something like bp_get_cache_group( $group_type ), to which you'd pass 'groups' or 'activity' or 'core' or whatever, so you could modify any of our cache buckets however you'd like.

Another thought that is more of a question that comes out of my limited knowledge of how various caching backends are configured: How much of the customization can we offload onto, eg, Memcached? I assume it's possible to set up, at the level of the cache config, rules about how to route the various cache groups (as in the case of multiple memcached servers, or whatever). I don't want to go too far in the direction of building customizability into BP that would be more efficiently handled in the caching layer itself.

[johnjamesjacoby]

I notice that your patch doesn't touch some of the other more specific cache buckets in BP

Good eye. Because these buckets aren't registered as global, under a stereotypical memcached or APC setup, they'll end up being prefixed with either the $blog_id or $table_prefix globals. This isn't ideal for network-wide installations, because we run into the issue that wpdennis opened this ticket for originally, with duplicate cache entries for each blog they are queried from.

I assume it's possible to set up, at the level of the cache config, rules about how to route the various cache groups

You are correct, and in many ways this does come down to a deficiency in configuring buckets, and WordPress core not needing per-network buckets, at least not yet. We basically need smart cache keys that are appropriately namespaced, based on the installation type. Too broad, and cached results will pour over into other networks; too narrow, and we cache the same data for each blog like wpdennis points out.

The widespread fix is a full audit of all cache keys, funneling them through cache-key building functions, and filtering them based on the single/multi/site/blog/network configuration and activation types.

[johnjamesjacoby]

As it stands today, the only component that should be using the bp global cache key is Members, and even it would benefit from leaning more on WordPress's core functions and global cache groups. bp_core_get_core_userdata is one of those functions where we are duplicating the work of WP_User and update_user_caches(). All we save by having our own user cache is skipping initializing roles and capabilities (see #5979.)

Every other component would benefit from a non-global, filtered cache key, so we can adjust the scope to be either global, for the network, or for the blog, based on the installation and activation.

[r-a-y]

Ran into a similar issue while working on a blog component plugin and needing to reference the blog meta on sub-sites. I'm getting different returned results on each site with an object cache plugin enabled with the same call - bp_blogs_get_blogmeta( SAME_BLOG_ID, SAME_META_KEY ).

We either need to use switch_to_blog( bp_get_root_blog_id() ) / restore_current_blog() inside our internal meta functions (bp_blogs_get_blogmeta(), groups_get_groupmeta(), etc.).

Or, the alternative is registering our meta cache groups to WP's global object cache groups:

• 'blog_meta'
• 'activity_meta'
• 'group_meta'
• 'message_meta'
• 'xprofile_meta'

FWIW, WordPress registers 'user_meta' as a global cache group. The most important is 'blog_meta' since it does make sense to actually check blog meta on a sub-site. The rest are less important, but still valid.

For now, I'm working around this by calling switch_to_blog( bp_get_root_blog_id() ) before calling on our blog meta functions mirroring blog meta to the site's wp_X_options table and using get_blog_option(), but it would be nice to address this.

[johnjamesjacoby]

Phew!

[johnjamesjacoby]

​5733.02.patch is a monolithic patch that does the following:

• Introduces bp_get_root_blog_cache_key() to get the cache key for the root blog in all BuddyPress installation types.
• Introduces bp_cache_get/set/delete functions to act as wrappers for wp_cache_ equivalents, and replaces all usages.
• Introduces bp_setup_cache_groups sub-action, which components will use to announce their cache groups.
• Introduces BP_Component::setup_cache_groups() for allowing components to setup their cache group needs.
• Introduces methods for each component to setup their cache group.
• Deprecates bp_core_add_global_group() as it's now part of BP_Core::setup_cache_groups()
• Introduces bp_xprofile_get_non_cached_field_ids() to handle a refactored bp_xprofile_data cache group, keyed by $user_id rather than grouped (as dynamic cache groups cannot reliably exist.)

This fixes a host of single-site & multiblog/site/network cache collisions, and allows for more work to go into non-persistent cache groups for things like counts that might be worth querying for on each page load.

I've deployed this patch to Flox, and will report back any findings and/or update should any anomalies arise.

Getting really close now, everyone!

[johnjamesjacoby]

In 9312:

Add setup_cache_groups method to BP_Component. See #5733.

[johnjamesjacoby]

In 9313:

Deprecate bp_core_add_global_group() and move it to BP_Core::setup_cache_groups().

Also introduce bp_cache_get/set/delete functions for appending the network ID to keys in global cache groups.

See #5733.

[johnjamesjacoby]

In 9314:

Introduce bp_setup_cache_groups() sub-action, for hooking into and appropriately prioritizing cache group additions. See #5733.

[johnjamesjacoby]

5733.02.patch minus the above commits

[johnjamesjacoby]

Same as 03, but without accidental bp_cache_flush change

[r-a-y]

05.patch fixes all unit tests that 04.patch breaks except the activitymeta, groupmeta, blogmeta and messagemeta ones.

The problem with r9313 is our cache for activitymeta, groupmeta, blogmeta and messagemeta will not work properly.

If you take a look at /wp-includes/meta.php, there are calls to wp_cache_X(). We'd have to somehow add in our own bp_cache_X() calls on the available metadata actions or filters in order to trump WP here.

[johnjamesjacoby]

Same as 05 but applies more cleanly

[r-a-y]

05.patch applies cleanly for me; 06.patch fails on line 1561 in src/bp-core/bp-core-classes.php of the patch.

---

Anyway, after spending some time trying to get the bp_cache_X() idea working without breaking some of our metadata caching (5 tests fail in PHPUnit, 8 with multisite testing), I think for 2.2, we separately commit the global cachegroups stuff and the removal of XProfile dynamic cachegroups.

The bp_cache_X() idea will need to be looked at for 2.3 if we're going to do a refactor of the metadata API to make it pass the metadata unit tests.

Unless there's a neat way around this :)

[johnjamesjacoby]

Replying to r-a-y:

05.patch applies cleanly for me; 06.patch fails on line 1561 in src/bp-core/bp-core-classes.php of the patch.

---

Anyway, after spending some time trying to get the bp_cache_X() idea working without breaking some of our metadata caching (5 tests fail in PHPUnit, 8 with multisite testing), I think for 2.2, we separately commit the global cachegroups stuff and the removal of XProfile dynamic cachegroups.

The bp_cache_X() idea will need to be looked at for 2.3 if we're going to do a refactor of the metadata API to make it pass the metadata unit tests.

Unless there's a neat way around this :)

I don't think that there is. Agree with your synopsis, too. I'll clean that up so its ready for beta tomorrow.

[johnjamesjacoby]

In 9333:

Remove bp_cache_ functions introduced in r9313; bumping to 2.3. See #5733.

[johnjamesjacoby]

In 9334:

Fix speeling error from r9313. See #5733.

[johnjamesjacoby]

In 9336:

XProfile data cache group updates:

• Switches from dynamic cache group to unique key for $user_id:$field_id.
• Introduces bp_xprofile_get_non_cached_field_ids() as a helper, based on bp_get_non_cached_field_ids().
• Registers bp_xprofile and bp_xprofile_data as global cache groups.
• Updates unit tests to pass with new cache keys.

Props r-a-y. Fixes #6100. See #5733.

[johnjamesjacoby]

Global cache groups

[johnjamesjacoby]

In 9338:

Switch most cache groups to global using new BP_Component::setup_cache_groups() methods.

This fixes issues with querying BuddyPress data from non-root sites, possibly creating duplicate cache entries across sites, and generally wrecking havoc with persistent cache setups.

Props r-a-y. See #5733.

[wpdennis]

I have just migrated from BuddyPress 2.0.x to 2.2 this week and it's working great so far! Even the xprofile data is cached in a single group instead of "per user groups", which is so much better.

All in all, object caching in 2.2 was huge step for multisites and is really awesome out of the box now! Thanks!

I will have a closer look the next fews days and report back, if I find something like #6219. :-)

[johnjamesjacoby]

Related: #6299.

[johnjamesjacoby]

In 9676:

XProfile: Introduce bp_xprofile_groups cache group, to replace bp cache group usage.

• Reduces field group cache keys down to just their ID, using a unique cache group
• Use all key instead of 'xprofile_groups_inc_empty`
• Use default_visibility_levels key instead of xprofile_default_visibility_levels

See #6341, #5733.