Skip to:
Content

Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

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

6978.caching.01.patch (16.4 KB) - added by dcavins 2 years ago.
Cache a user's friendships.
6978.2.patch (21.3 KB) - added by dcavins 22 months ago.
Cache friendship objects separately from a user's friendships.
6978.3.patch (22.9 KB) - added by dcavins 21 months ago.
Return BP_Friends_Friendships objects in get_friendships().
6978.4.patch (23.6 KB) - added by dcavins 21 months ago.
One more place could potentially use a cached object.

Download all attachments as: .zip

Change History (21)

#1 @DJPaul
2 years ago

  • Milestone changed from 2.6 to Future Release

@dcavins
2 years ago

Cache a user's friendships.

#2 @dcavins
2 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. :)

#3 @dcavins
2 years ago

  • Keywords dev-feedback added

#4 follow-up: @boonebgorges
22 months 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 @dcavins
22 months 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 @dcavins
22 months 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!

@dcavins
22 months ago

Cache friendship objects separately from a user's friendships.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


22 months ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


22 months ago

#9 @boonebgorges
22 months 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 @dcavins
22 months 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 @dcavins
21 months 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 in BP_Friends_Friendship::get_friendships() can be real BP_Friends_Friendship objects and still use the primed cache. (This was basically lifted from @boonebgorges' recent changes to BP_Groups_Group which are awesome.)

Unit tests are passing, and the changes seem to be working as expected. Thanks for your feedback!

@dcavins
21 months ago

Return BP_Friends_Friendships objects in get_friendships().

@dcavins
21 months ago

One more place could potentially use a cached object.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


21 months ago

#13 @dcavins
21 months ago

In 11122:

Add bp_friends_friendships cache group.

Add friendship caching by the ID of the friendship.
Check the cache before querying for friendship
details in BP_Friends_Friendship::populate.

Update get_user_ids_for_friendship() to create
a new friendship object so it could possibly use
the cached object.

Props dcavins, boonebgorges.

See #6978.

#14 @dcavins
21 months ago

In 11123:

Introduce BP_Friends_Friendships::get_friendships().

Introduce a new function, BP_Friends_Friendships::get_friendships(),
for fetching and filtering friendships for a
specific user. Also add a new cache group,
bp_friends_friendships_for_user that
get_friendships() interacts with.

This is the second step in moving toward a
split cache approach for friendships.

Props dcavins, boonebgorges.

See #6978.

#15 @dcavins
21 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 11124:

Use new function BP_Friends_Friendships::get_friendships().

Take advantage of the caching introduced
in BP_Friends_Friendships::get_friendships()
by using get_friendships() as the underlying function in
get_friend_user_ids(), get_friendship_id(), total_friend_count(),
check_is_friend(),
and delete_all_for_user().

Props dcavins, boonebgorges.

Fixes #6978.

#16 @dcavins
21 months ago

  • Keywords dev-feedback has-patch removed
  • Milestone changed from Future Release to 2.7

#17 @dcavins
21 months ago

In 11125:

Friends tests: Set date_created to ensure reliable ordering.

Some of the tests for friendship functionality use assertEquals(),
which requires that the elements of arrays be in the same order and
each value be equal. In some cases, the order of the elements returned
by functions like friends_get_friendship_request_user_ids() was
unreliable, probably because phpunit was working quickly enough that
the date_created was indeterminate. This change set manually sets the
earlier friendship back in time so that the return values can be
checked reliably.

See #6978.

Note: See TracTickets for help on using tickets.