Skip to:
Content

BuddyPress.org

Opened 7 years ago

Closed 5 years ago

#5407 closed enhancement (fixed)

Improvements to object caching in bp-groups

Reported by: boonebgorges Owned by:
Milestone: 2.0 Priority: normal
Severity: normal Version:
Component: Groups Keywords:
Cc:

Description

The groups component uses object caching in some places, but it's incomplete and in some cases appears to be incorrect. This is a general ticket for commits that address the issue for the 2.0 cycle.

Change History (15)

#1 @boonebgorges
7 years ago

In 7956:

Overhaul the way that individual group objects are cached

The existing implementation was broken in a number of ways. Most critically,
user-specific data (like is_member) was being stored in the persistent object
cache without respect to user.

This refactor excludes the user-specific data from being cached along with the
group object. It also reworks the way that items are keyed in the cache, for
greater consistency with WordPress and the other BP components.

See #5407

#2 @boonebgorges
7 years ago

In 7974:

Fix logic error in caching in BP_Groups_Group::populate()

Introduced in r7956, this bug cause the populate logic not to be run when
the cache was hit successfully. Mega-anti-props boonebgorges.

See #5407

#3 @boonebgorges
7 years ago

In 7980:

Exclude 'last_activity' and 'total_member_count' from the bp_groups item cache

These items are stored in groupmeta, and thus are cached independently.
Furthermore, including them in the group cache will require that cache to be
busted more frequently than necessary.

See #5407

#4 @r-a-y
7 years ago

In 8001:

Properly set object cache for a new BP_Groups_Group object.

See #5407

#5 @johnjamesjacoby
7 years ago

I think r8001 is incorrect. We only want to cache complete group objects, not partials. We do cache the group already on line 223 already, after admins and moderators are added.

It's probably worth discussing how we query for and cache supplemental component data, both including and excluding meta.

#6 @r-a-y
7 years ago

Good call.

I was getting cache misses when grabbing the groups object for secondary avatars in the activity loop.

Will revert and look into it some more.

#7 @boonebgorges
7 years ago

johnjamesjacoby is correct. The caching happens here: https://buddypress.trac.wordpress.org/browser/trunk/bp-groups/bp-groups-classes.php?rev=8001#L223 We could do multiple layers of caching here if we thought it was worth the trouble (one for basic data, and another for populate_extras data). My thought is that it's *not* currently worth the trouble (which is why it currently works the way it does), but I'd be happy to discuss.

It's probably worth discussing how we query for and cache supplemental component data, both including and excluding meta.

Metadata - in the technical sense of "stored in the meta tables" - should never be cached with the object. We have separate object caches for that.

In an ideal world, we would have fine-grained caches for all other supplemental data - stuff like admins_mods_of_group_54. But this can be a lot of work, because it multiplies the amount (and different kinds) of invalidation we have to do. So, where it's easier, like in this case, I think it's fine to cache with the object.

r-a-y - Just saw your point about secondary avatars. That would be the one place where populate_extras=false caching might make sense. If you feel like breaking it up into two layers of cache, please be my guest. (Please write unit tests for it :) )

#8 @r-a-y
7 years ago

That would be the one place where populate_extras=false caching might make sense. If you feel like breaking it up into two layers of cache, please be my guest.

Yup, that's what I plan on doing. And yes, I'll write some unit tests for this :)

#9 @r-a-y
7 years ago

In 8008:

Remove duplicate wp_cache_set() call in BP_Groups_Group::populate().

See #5407

#10 @boonebgorges
7 years ago

  • Milestone changed from 2.0 to 2.1

The groups component still needs a fair amount of attention wrt to object caching, so I'm moving this to 2.1 for further work.

#11 @boonebgorges
7 years ago

See #5436 for some related discussion.

#12 @boonebgorges
7 years ago

See r8610. I think we now can move forward with some of the changes we'd initially discussed (removing the direct queries for admin/mod info, and using BP_Group_Member_Query instead)

#13 @DJPaul
7 years ago

  • Milestone changed from 2.1 to 2.2

#14 @DJPaul
7 years ago

  • Milestone changed from 2.2 to Future Release

#15 @r-a-y
5 years ago

  • Milestone changed from Future Release to 2.0
  • Resolution set to fixed
  • Status changed from new to closed

Closing this one.

We've done quite a bit of improvements to group caching over the past few releases.

Moving this back to the older 2.0 milestone where the initial improvements were implemented.

Note: See TracTickets for help on using tickets.