Opened 8 years ago
Closed 7 years ago
#7130 closed defect (bug) (fixed)
bp_notifications_get_all_notifications_for_user gets all records and can cause memory leak
Reported by: | hberberoglu | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 3.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Toolbar & Notifications | Keywords: | has-patch |
Cc: |
Description
Hi there, we have more than 1.6M users on our database. Around 700.000 records on wp_bp_notifications. We use https://tr.wordpress.org/plugins/memcached/ for object caching.
Some users have 37k wp_bp_notifications record, another 20k, another 17k, 11k and goes on.
When we enter bp profile page of these users bp_notifications_get_all_notifications_for_user runs and tries to get all these records, and it can get but wp_cache_set can not set cache. It gives that error:
PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 2097152 bytes) in /var/www/wp-content/object-cache.php on line 308
If user have a lot of notification record it gives that error, if users doesnt have much record there is no problem.
Attachments (5)
Change History (28)
#1
@
8 years ago
- Component changed from Core to Performance
- Type changed from defect (bug) to enhancement
#2
@
8 years ago
- Component changed from Performance to Toolbar & Notifications
- Milestone changed from Awaiting Review to Future Release
- Type changed from enhancement to defect (bug)
It looks like we should change this function to cache the Notification IDs, and separately implement caching for Notification objects (if we don't already?).
We also need to consider where/how we use this data, and ideally set some upper-bound for the amount of objects queried to stop memory exhaustion. Like, bp_notifications_get_unread_notification_count
just gets the count, so maybe its own SQL count for that is a better solution.
#3
@
8 years ago
I agree with @DJPaul. My thought was the same with him when I'm sending this ticket.
On my side we've already solved this problem in some way but it required to write code in core BuddyPress which we never done before. We wrote code core of WordPress because of problems like this (big database, high traffic) before but this is the first time we writing to core BuddyPress. Actually we just wanted to get count of notifications of some user and show it on our mobile applications via rest api. We use default memory limit and didn't/don't want to increase it so it caused memory exhaustion.
Btw maybe we can solve it by using bp_notifications_get_where_conditions but we didn't looked this filter yet. It is again not a good way I think. Yes it solves to writing code to core but it is not the best way.
I hope it can solve in core.
#4
@
8 years ago
I think bp_notifications_get_unread_notification_count
should use BP_Notifications_Notification::get_total_count
and bp_notifications_get_notifications_for_user
shouldn't make expensive calculations, we can use group query to fetch latest unread notifications and total count.
So, I just added the patch that reduces memory usage.
Benchmarking:
notification count | unread | external object cache | patch status | memory usage |
37980 | 37977 | none | without patch | 141,150 kB |
37980 | 37977 | memcache | without patch | 118,976 kB |
30001 | 26999 | none | without patch | 109,732 kB |
30001 | 26999 | memcache | without patch | 94,342 kB |
37980 | 37977 | none | with patch | 41,136 kB |
37980 | 37977 | memcache | with patch | 40,056 kB |
30001 | 26999 | none | with patch | 41,138 kB |
30001 | 26999 | memcache | with patch | 40,056 kB |
(Probably, the patch needs to be updated, it may not be ready to merge but can give some thoughts.)
#6
@
7 years ago
I ran into same issue and was intending on putting in a new ticket, but then checked and saw that it was already reported. I approached problem differently in my patch, by simply passing another argument that is an array that can address query itself (by passing per_page and page to limit what is queried and what is cached as well).
#7
@
7 years ago
- Milestone changed from Future Release to 3.0
Hey, thanks for the patch @mauteri. Sorry we didn't see this until now -- we'll look at this for 3.0.
#9
@
7 years ago
Going to leave this one to @boonebgorges for a second opinion, as he's better at this stuff than I.
#10
@
7 years ago
@boonebgorges or perhaps @r-a-y You're both cache experts, I would appreciate your thoughts on this patch so we can get back to @mauteri, thanks.
#11
@
7 years ago
Hi all - thanks for the work on this ticket, and sorry for the delay in reviewing.
First, it seems reasonable to me to modify the 'count' query/caching as suggested by @m_uysl in 7130.diff. I've separated this suggestion out into its own patch 7130-counts.diff so it can be considered by itself.
The rest of the patch by @m_uysl is focused on caching the grouped notifications. I'm curious about benchmarking just for this. My impression is that the costly part of bp_notifications_get_notifications_for_user()
is the formatting, not the grouping. Does that seem correct? If so, I think it's probably worth adding another layer of caching for formatted items.
More specific caching for notification queries, as suggested by @mauteri in 7130.2.diff, also seems like a decent idea, though it wouldn't do much out of the box, since we don't pass $args
to this function anywhere in BP. Do we think it's wise to add an upper-bound to the number of notifications queried when populating the formatted notification dropdown? What if bp_notifications_toolbar_menu()
, by default, only queried for (say) the 100 most recent unread notifications for a user? In cases where there are more notifications than this, the grouped notification messages would be incorrect - "You have 23 group invitations" would only reflect the group invitations in the first 100, etc - but it would have a pretty dramatic impact on performance in cases where users have many thousands of unread messages. If we did something like this, it would make more sense to introduce a finer-grained cache key for notification queries, as suggested by @mauteri.
#12
@
7 years ago
Hi,
@boonebgorges thanks for the separated patch.
My impression is that the costly part of bp_notifications_get_notifications_for_user() is the formatting, not the grouping. Does that seem correct?
Yeah, seems correct. But, this part:
for ( $i = 0, $count = count( $notifications ); $i < $count; ++$i ) {
$notification = $notifications[$i];
$grouped_notifications[$notification->component_name][$notification->component_action][] = $notification;
}
a little annoying and the rest of the patch is almost same with the core. (code formatting could be the worst thing for a patch)
since we don't pass $args to this function anywhere in BP
We can use filter instead but the grouped notification messages would be still incorrect.
What if we change the behavior of bp_notifications_toolbar_menu()
. I've checked various social networks and the basic approach is:
- Show the unread notification count
- Show the latest
$x
amount notification
(IMHO) that would make notifications more streamlined ( and totally deserves to be an another ticket :) )
#14
@
7 years ago
If we did something like this, it would make more sense to introduce a finer-grained cache key for notification queries, as suggested by @mauteri.
@boonebgorges should i open another ticket for this in that case? This is on the extreme, but I did a comment import without removing the new-comment notification hook and had some users that had 5 figures of notifications :-O Site ran out of memory and fatal error at the header when user logged in. Not putting a limit on things never seems like a great idea especially when you consider large sites.
The grouping is problematic, as limiting makes the call inaccurate, however, in reality having that many unread notifications seems to be unlikely for most users. Maybe that upper limit should be 500 or even 1000 rather than 100 as you suggested. I do think there should be an upper limit here. Also to go along with a finer-grain cache key, we can apply a new filter before the ksort, so if BuddyPress limits the number of notifications returned by default, developers can easily override it.
#15
@
7 years ago
@m_uysl After [11851] I took a second, closer look at your patch, and I think I really like the idea. In my very rough tests, the GROUP BY
query is much faster than doing similar logic in PHP. Can you look at the new patch to see if I've captured it correctly? (I've regenerated after [11851] and done a small amount of documentation changes.)
I agree that it might be worthwhile to rethink the notification toolbar a bit more radically, but you're correct that this is a larger project, and it shouldn't hold up improvements like this one.
@mauteri Before we go any further down the road of limiting the query, could you have an initial look at 7130.3.diff? The important change here is that the heavy work of grouping has been handed off to MySQL in get_grouped_notifications_for_user()
. If by any change you have a test rig that has the amounts of data you've discussed here (5 figures!!), it would be hugely helpful if you could apply the patch to see what kind of numbers you see against a very large notifications table.
#16
@
7 years ago
@boonebgorges sure, i can do a local patch and simulate a 5 figure new notification response from a production dump on that table :-D. Will let you know what happens!!
#17
@
7 years ago
Can you look at the new patch to see if I've captured it correctly?
@boonebgorges looks good to me.
I've tested patches with ~24k notifications and ~98M memory usage reduced to ~36M, page generation time improved as well.
#18
@
7 years ago
@m_uysl Excellent, thank you!
@mauteri Have you had a chance to simulate the large amounts of data you suggested above? It'd be great to have another data point before throwing this in - and I'd love to have it for 3.0, so that only gives us a couple weeks (I think).
#19
@
7 years ago
@boonebgorges tested your patch and it works fine. I tested with 104059 new notifications for a user. Current version of BuddyPress had a fatal error and the admin bar did not show. Patched version (7130.3.diff) of BuddyPress did not fatal and admin bar showed up fine.
#23
@
7 years ago
- Resolution set to fixed
- Status changed from assigned to closed
Given that the improvements in [11888] seem to be really significant, I'm tentatively going to close this ticket so that we can have some closure for this milestone. Some of the earlier ideas about cache specificity are worth looking into, but not urgent for 3.0. If anyone wants to take on one of those projects, please feel free to start a new ticket. Thanks to all for help with this one!
Great that you have a thriving community, @hberberoglu!
I think in this instance, you should limit the number of notifications that is pulled in
bp_notifications_get_all_notifications_for_user()
.We don't have a suitable filter to modify the arguments in
bp_notifications_get_all_notifications_for_user()
, however you could try the'bp_notifications_get_where_conditions'
filter to modify the SQL statement.Another thing that you might want to consider is automatically deleting notifications that are older than a certain date with a WP cron job.