[buddypress-trac] [BuddyPress Trac] #7130: bp_notifications_get_all_notifications_for_user gets all records and can cause memory leak

[buddypress-trac]

#7130: bp_notifications_get_all_notifications_for_user gets all records and can
cause memory leak
-------------------------------------+------------------
 Reporter:  hberberoglu              |       Owner:
     Type:  defect (bug)             |      Status:  new
 Priority:  normal                   |   Milestone:  3.0
Component:  Toolbar & Notifications  |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch                |
-------------------------------------+------------------

Comment (by 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 [attachment:7130.diff]. I've separated this
 suggestion out into its own patch [attachment: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 [attachment: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.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7130#comment:11>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac