#6978 closed enhancement (fixed)
Friends: Add caching to BP_Friends_Friendship class
Reported by: | r-a-y | Owned by: | dcavins |
---|---|---|---|
Milestone: | 2.7 | Priority: | normal |
Severity: | normal | Version: | 1.0 |
Component: | Friends | Keywords: | |
Cc: |
Description
We should cache calls for this class so it is similar to other components.
This will save us one DB query when a user is viewing another user's profile page.
Attachments (4)
Change History (21)
#2
@
8 years ago
I've just added a patch that:
Adds cache item bp_friends_friendships
.
Introduce BP_Friends_Friendship::get_friendships()
which uses the WP cache to store friendships and wp_list_filter()
to filter them to return the right results.
@r-a-y I realized as I was working on this that I wasn't sure this is the kind of caching you were talking about. Ha ha. Even if it isn't, I'd love some feedback. :)
#4
follow-up:
↓ 5
@
8 years ago
@dcavins General approach seems pretty good to me. Does 6978.caching.01.patch need a refresh after recent changes to bp_get_user_groups()
? See eg [10978], [10977], #7208.
How much extra work would it be to split friendship queries, so that we're doing an ID query and then filling the objects from the cache? (I think that's what @r-a-y was originally suggesting.) This would better parallel what's happening in other components, and would give performance improvements when viewing a single user's profile.
#5
in reply to:
↑ 4
@
8 years ago
Replying to boonebgorges:
@dcavins General approach seems pretty good to me. Does 6978.caching.01.patch need a refresh after recent changes to
bp_get_user_groups()
? See eg [10978], [10977], #7208.
Do you mean avoiding the use of wp_list_filter()
as you've done in the groups membership functions? That shouldn't be a big deal, since you've already developed an approach that works.
How much extra work would it be to split friendship queries, so that we're doing an ID query and then filling the objects from the cache? (I think that's what @r-a-y was originally suggesting.) This would better parallel what's happening in other components, and would give performance improvements when viewing a single user's profile.
I don't think it would make it too much more complex. I'll update the patch.
Thanks for taking a look at this!
#6
@
8 years ago
- Keywords has-patch added
- Owner set to dcavins
- Status changed from new to accepted
I'm attaching a new patch that follows the more-normal BP pattern:
- The IDs of the friendship objects are cached for each user.
- The friendship objects are cached separately, keyed by friendship ID.
The new patch avoids the use of wp_list_filter()
and does the list filtering inline.
I've added a couple of tests to verify that the caching is working as expected. All other friendship tests are passing.
I may have uncovered a small bug in BP_Friends_Friendship::delete_all_for_user()
, so I'd like an extra eye particularly on my changes to that function. The problem is that the function fetches friend IDs via BP_Friends_Friendship::get_friend_user_ids()
, but that function either returns confirmed friendships, or requests only, depending on the arguments, so I imagine that when users are removed currently, unconfirmed friendship records are being left in the database.
Thanks!
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
#9
@
8 years ago
I haven't done a line-by-line review, but the approach here looks good.
Re delete_all_for_user()
. It looks like we only use this in core when deleting/spamming a user, in which case it's definitely a bug for unconfirmed friendships to be left behind. Could you do a search of the plugin repo and/or GitHub to see whether there are other uses of it? I can imagine possible cases where you'd rely on the current behavior. If you can't find anything obvious, go ahead with the change as you've proposed it.
#10
@
8 years ago
My maybe bug isn't a bug after all. The function is currently using the confirmed friendships only for cache busting of the "total_friendships" cache for friends of the user who is being removed. My earlier change will also clear the total_friend_count
for unconfirmed friends, which is not strictly necessary. I'll update the delete_all...
function to be more conservative.
#11
@
8 years ago
I've updated this patch to do the following:
- More closely maintain the behavior of
delete_all_for_user()
(and clear all of the related cache items). - Add a cache fetch in
BP_Friends_Friendship::populate()
. This means that the objects returned inBP_Friends_Friendship::get_friendships()
can be realBP_Friends_Friendship
objects and still use the primed cache. (This was basically lifted from @boonebgorges' recent changes toBP_Groups_Group
which are awesome.)
Unit tests are passing, and the changes seem to be working as expected. Thanks for your feedback!
Cache a user's friendships.