Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 17 months ago

#5979 closed defect (bug) (fixed)

`bp_core_userdata_` cache is unnecessary

Reported by: johnjamesjacoby Owned by:
Milestone: 3.0 Priority: high
Severity: normal Version: 1.2
Component: Members Keywords:
Cc:

Description

A note in bp-core-classes.php incorrectly states that:

// WP_User_Query doesn't cache the data it pulls from wp_users,
// and it does not give us a way to save queries by fetching
// only uncached users. However, BP does cache this data, so
// we set it here.

WP_User_Query does cache data pulled from wp_users via calls to WP_User for each queried result. WP_User::get_user_by gets from and sets each user object as needed.

Our judicious use of bp_core_get_core_userdata() helps act as a funnel for calls to deeper methods, so our abstraction will help keep any fixes relatively small and low-impact.

Attachments (4)

5979.patch (2.2 KB) - added by johnjamesjacoby 5 years ago.
5979.02.patch (3.1 KB) - added by johnjamesjacoby 5 years ago.
5979.process_spammer.patch (640 bytes) - added by r-a-y 5 years ago.
5979-tests.patch (1.5 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (22)

#1 @johnjamesjacoby
5 years ago

  • Keywords has-patch added

Related to #5733.

#2 @johnjamesjacoby
5 years ago

Related to #5903, which hints at how this additional caching layer introduced a bug that wouldn't exist had we trusted WP_User_Query and the WP_User objects.

Last edited 5 years ago by johnjamesjacoby (previous) (diff)

#3 @johnjamesjacoby
5 years ago

and it does not give us a way to save queries by fetching only uncached users

WP_User_Query does this for us by calling cache_users() which uses _get_non_cached_ids() to get the missing users, then calling update_user_caches() and update_meta_cache( 'user', $ids ); on them.

#4 follow-up: @boonebgorges
5 years ago

  • Keywords 2nd-opinion removed

cache_users() is only called if we call fields=all_with_meta, which BP_User_Query does not do. But yes, you are correct about WP_User.

Would be nice to have a unit test that shows it's cached by WP, and I shudder at your ternary syntax, but I'm on board with tearing out our redundant caching.

#5 in reply to: ↑ 4 @johnjamesjacoby
5 years ago

Replying to boonebgorges:

cache_users() is only called if we call fields=all_with_meta, which BP_User_Query does not do. But yes, you are correct about WP_User.

Just ran into that. You beat me to the updated patch. I see no reason why BP_User_Query shouldn't assume all_with_meta considering our current usages.

Would be nice to have a unit test that shows it's cached by WP, and I shudder at your ternary syntax, but I'm on board with tearing out our redundant caching.

I'll get test coverage next.

#6 @boonebgorges
5 years ago

Just ran into that. You beat me to the updated patch. I see no reason why BP_User_Query shouldn't assume all_with_meta considering our current usages.

all_with_meta is pretty heavy-duty - it primes the usermeta cache for each matched user. On the typical BP setup, I don't think we use much usermeta in the loop that we're not fetching elsewhere, and in my experience, a lot of plugins can leave a lot of junk in usermeta. So I'm wary of defaulting to it. As long as the core wp_users data is being cached (which it is in WP_User) I lean toward not using all_with_meta, and maybe allowing it as an option in BP_User_Query.

Regarding tests, just to give you a head start: look at $wpdb->num_queries before and after running :)

#7 @johnjamesjacoby
5 years ago

If we don't use all_with_meta then we don't get the cache goodness. And we do make quite a few calls to bp_get_user_meta() that (without access to the code at the moment) I assume boil back down to a cached user object. I agree that needlessly getting usermeta doesn't make sense, but neither does skipping the cache just because we don't need the meta at that point.

It's definitely worth scrutinizing more. I have a hunch the tests will be pretty revealing. Thanks for the hint on where to look.

#8 @boonebgorges
5 years ago

And we do make quite a few calls to bp_get_user_meta() that (without access to the code at the moment) I assume boil back down to a cached user object

If this is true, then yes, we should probably cache it. Your best resource here will be SAVEQUERIES: if you turn on all_with_meta, load the user directory, and see an extra 20 database queries, you'll know that we don't need the extra data. It's also worth remembering that user_login, user_nicename, and user_email will be cached by WP_User, and IIRC they are the only values that we regularly need in the loop. But all of this should of course be driven by metrics, so thanks for looking into it more deeply!

#9 @johnjamesjacoby
5 years ago

We also have our own caches for nicenames and such that I'm intentionally not touching until I have a chance to get real metrics about what we are and should be caching for user objects.

#10 @johnjamesjacoby
5 years ago

In 9116:

Use uppercase ID in WP_User based objects to prevent deprecated notices. See #5979.

#11 @djpaul
5 years ago

In 9119:

Core: partial revert of r9116 (user->id to user->ID changes).

In our legacy user fetching functions, we use something similar to "select ... ID as id" in the SQL. In the reverted changes, we are only using the property to return an array of integers, and the change of the property name was causing PHP to throw "undefined property" messages.

As these lines would have never thrown a WordPress core deprecated warning about using id instead of ID, I'm changing them back to clear the warning message and fix the unit tests.

See #5979

#12 @johnjamesjacoby
5 years ago

In 9122:

Partial revert of r9116 (user->id to user->ID changes)

Has potential for breakage in themes, and requires more investigation before fully addressing.

See #5979.

#13 follow-up: @r-a-y
5 years ago

Not sure what the status of this ticket is, but I ran into another 'bp_core_userdata_' cache invalidation issue when marking a user as a spammer.

I've attached a patch.

#14 in reply to: ↑ 13 ; follow-up: @boonebgorges
5 years ago

Replying to r-a-y:

Not sure what the status of this ticket is, but I ran into another 'bp_core_userdata_' cache invalidation issue when marking a user as a spammer.

I've attached a patch.

Hm. 5979-tests.patch shows that your fix is not necessary. And it's because on 'bp_make_spam_user' and 'bp_make_ham_user', we call bp_core_remove_data(), which flushes the entire cache!! Um, we probably should not do this.

Last edited 5 years ago by boonebgorges (previous) (diff)

#15 in reply to: ↑ 14 @johnjamesjacoby
5 years ago

Replying to boonebgorges:

we call bp_core_remove_data(), which flushes the entire cache!! Um, we probably should not do this.

This sounds vaguely familiar. I think because spamming a user intersects several counts and queries, it made more sense to brute-force-flush at the time this was put in. Maybe to flush friends or group memberships or something else too?

We'll no doubt run into a few more cases like this, where a more targeted caching strategy should be implemented.

#16 @DJPaul
4 years ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from 2.2 to Future Release

#17 @DJPaul
4 years ago

  • Keywords dev-feedback added; has-patch needs-testing needs-unit-tests removed

#18 @dcavins
17 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Contributions to 3.0
  • Resolution set to fixed
  • Status changed from new to closed
  • Version set to 1.2

This issue was revisited in #7595 and fixed in r11705.

Note: See TracTickets for help on using tickets.