Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#7237 closed enhancement (fixed)

Incrementor-based query caching for the Activity component

Reported by: boonebgorges's profile boonebgorges Owned by: boonebgorges's profile boonebgorges
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch
Cc: espellcaste@…

Description

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.

Attachments (2)

7237.diff (8.7 KB) - added by boonebgorges 8 years ago.
7237.2.diff (9.9 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (18)

#1 @espellcaste
8 years ago

  • Cc espellcaste@… added

@boonebgorges
8 years ago

#2 @boonebgorges
8 years ago

  • Keywords has-patch added

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.

#3 follow-up: @dcavins
8 years ago

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!

#4 @DJPaul
8 years ago

I like.

#5 @johnjamesjacoby
8 years ago

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.

#6 in reply to: ↑ 3 @johnjamesjacoby
8 years ago

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.

#7 @boonebgorges
8 years ago

  • Milestone 2.7 deleted
  • Status changed from new to closed

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.

#8 @boonebgorges
8 years ago

  • Milestone set to 2.7
  • Status changed from closed to reopened

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

@boonebgorges
8 years ago

#9 @boonebgorges
8 years ago

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.

#10 @dcavins
8 years ago

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.

#11 @boonebgorges
8 years ago

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 :)

#12 @boonebgorges
8 years ago

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.

#13 @boonebgorges
8 years ago

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

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.

#14 @boonebgorges
8 years ago

In 11057:

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

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

Fixes #7242.

#15 @boonebgorges
8 years ago

In 11060:

bp_core_reset_incrementor() should return a boolean.

Props DJPaul.
See #7237, #7245.

#16 @boonebgorges
8 years ago

In 11076:

Activity: Invalidate query cache when activity meta is modified.

See #7237.

Note: See TracTickets for help on using tickets.