Incrementor-based query caching for the Activity component

[boonebgorges]

See #6643 for a more general discussion.

Yesterday a client site was brought to its knees by a complex activity query (on the front page, being called thousands of time, and crashing MySQL). I solved the problem by adding a caching layer around that query. Let's do it automatically in the Activity component.

I'll work up a first patch, so others can get a sense of the approach.

[boonebgorges]

7237.diff​ is a basic first pass at the functionality.

• The main caching logic for the activity query itself is inside of BP_Activity_Activity::get(). As you can see, it's quite simple. I've opted to use the SQL string as an identifier for the query, since it's guaranteed to make queries unique, and will be a commonality across all components.
• I believe that activity queries only need invalidation on activity addition, edit, and deletion.
• bp-core-cache.php contains a couple new utility functions for setting and getting ID query caches. The implementation is designed to be fairly opaque for the developer - you don't need to know how it works under the hood, and you definitely don't need to know anything about incrementors to cache data in a able-to-be-invalidated-in-bulk way. (This is in contrast to WP, where all incrementors are done inline.) I'm happy to reconsider the naming and function signatures of these new functions, to make sure they accurately describe what they're doing.
• The tests demonstrate that the basic caching is working properly. Let me know if there are edge cases I've missed.

Would especially like feedback from @r-a-y and @dcavins, though I'd welcome comments from all.

[dcavins]

I really like the generalized functions for handling "last changed" cache invalidation. It will make it much easier to standardize across BP, and make it easier to add caching without adding the "last changed" check everywhere.

Is there any reason to limit the function bp_core_get/set_cached_ids() to IDs? Could it more generally be bp_core_get/set_cached_data() and some cached data just happens to be IDs?

I think this is a great idea overall as it will make performance improvements easier to do in a lot of places. Cool!

[DJPaul]

I like.

[johnjamesjacoby]

Nice!

If we ever wanted to introduce a "common" library outside of the getting-pretty-cluttered "core", this might be a good candidate for that.

[johnjamesjacoby]

Replying to dcavins:

Is there any reason to limit the function bp_core_get/set_cached_ids() to IDs?

The IDs are only used as cache keys. The entire activity object itself is cached, these functions just help purge and prime the correct IDs.

[boonebgorges]

I think @dcavins is correct that this isn't just for ID arrays; I realized after I (hurriedly) posted the patch this morning that I'd forgotten the same treatment for the COUNT query. cached_data seems to general, though; this is, specifically, data that's cached using a key hashed with an incrementor so it can be invalidated en masse. Not sure how to summarize that in a function name :) Maybe bp_core_set_cached_data_with_incrementor()? It's not melodious, but it's fairly descriptive.

If we ever wanted to introduce a "common" library outside of the getting-pretty-cluttered "core", this might be a good candidate for that.

Agreed - mental note.

[boonebgorges]

See also ​https://core.trac.wordpress.org/ticket/37464, which I just noticed, and which happens to introduce an identical function to WP :)

[boonebgorges]

Thanks for the helpful feedback so far! 7237.2.diff​ implements the cache for the activity count, and changes the function names to bp_core_set_incremented_cache() and bp_core_get_incremented_cache(). The new names are a bit of a mouthful, but I think they're fairly clear. Would love to have a +1 on the new names, and then I'll put this in so we can get some testing out of it.

[dcavins]

I don't think that bp_core_set_incremented_cache() is too unhandy of a name, since it's very clear what you intend it to do and not overlong. Although I seem to recall you telling me that a BP Docs patch of mine included a new function name that was "self-documenting," so maybe I'm the wrong person to ask. :)

My only comment is that the argument name in and documentation for bp_core_set_incremented_cache() still references storing IDs.

Looking forward to using this in a patch I'm reworking.

[boonebgorges]

My only comment is that the argument name in and documentation for bp_core_set_incremented_cache() still references storing IDs.

Ah, good catch.

Let's go with it and see what breaks :)

[boonebgorges]

In 11053:

Introduce cache incrementor functions.

A cache incrementor is a unique string that allows for easy bulk
invalidation of specific cache items. In the BuddyPress context, this
is useful for things like object queries. Object queries have more
permutations of query parameters than can be individually invalidated.
As a workaround, we can ensure that all cache keys for query results
are hashed with the same incrementor; bumping this incrementor will
then effectively invalidate all of these caches.

See #7237.

[boonebgorges]

In 11054:

Cache the results of activity ID and count queries.

Query results are cached using an incrementor, and are invalidated in
bulk whenever an activity item is created, updated, or deleted.

Fixes #7237.

[boonebgorges]

In 11057:

Activity: Remove front-page-specific activity query cache.

We now have more general caching for activity queries. See #7237.

Fixes #7242.

[boonebgorges]

In 11060:

bp_core_reset_incrementor() should return a boolean.

Props DJPaul.
See #7237, #7245.

[boonebgorges]

In 11076:

Activity: Invalidate query cache when activity meta is modified.

See #7237.