bp_notifications_get_all_notifications_for_user gets all records and can cause memory leak

[hberberoglu]

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.

[r-a-y]

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.

[DJPaul]

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.

[hberberoglu]

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.

[m_uysl]

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

[mauteri]

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

[mauteri]

Another approach to issue

[mauteri]

Bug in code, ksort in wrong place.

[DJPaul]

Hey, thanks for the patch @mauteri. Sorry we didn't see this until now -- we'll look at this for 3.0.

[mauteri]

cool thx :-)

[DJPaul]

Going to leave this one to @boonebgorges for a second opinion, as he's better at this stuff than I.

[DJPaul]

@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.

[boonebgorges]

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.

[m_uysl]

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

[boonebgorges]

In 11851:

Notifications: User count query should be separately fetched and cached.

Previously, notification counts were calculated by fetching the entire
notification objects.

Props m_uysl.
See #7130.

[mauteri]

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.

[boonebgorges]

@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.

[mauteri]

@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!!

[m_uysl]

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.

[boonebgorges]

@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).

[mauteri]

@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.

[DJPaul]

🤩

[boonebgorges]

Thanks @mauteri !

[boonebgorges]

In 11888:

Move notification-grouping logic to MySQL.

The notification-grouping logic required to show the collapsed Notifications
toolbar - "You have 3 pending friend requests", etc - is currently done in
PHP, in a manner that requires all notifications to be loaded into memory
and processed. On sites where users have large numbers of notifications, this
operation requires extensive resources. By leaving the grouping to MySQL,
we can speed up processing in almost all situations.

This changeset also adds specific caching for grouped notifications.

Props m_uysl.
See #7130.

[boonebgorges]

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!