For a sufficiently large thread (lots of users) using in_array() becomes expensive and time consuming. It would be better to do this limitation by sql. Attached is a patch that does this without modifying the result set.

CommentFileSizeAuthor
#1 privatemsg_views.patch1.99 KBelliotttf
privatemsg_views.patch2.08 KBelliotttf

Comments

elliotttf’s picture

elliotttf commented
StatusFileSize
new privatemsg_views.patch1.99 KB

Sorry, left some debug code in there, this one's better.

crea’s picture

crea commented

Any benchmarks ?

elliotttf’s picture

elliotttf commented

Just a seat of the pants test where a thread with 30k subscribers timed out on the in_array searches.

For very small sets the difference is going to be negligible, but for large sets introducting an in_array() on each loop causes issues. You're essentially doing O(n^2) since for every insertion to the array you are also searching the whole array for the presence of the new participant.

crea’s picture

crea commented

I want to note that stuff like 30k subscribers per thread is not something Privatemsg (and Privatemsg Views) is designed to work with at all. This is a private messaging system, not notifications system.
AFAIK there is an issue about sending to roles for Privatemsg 2.0 where similar problems are discussed (you don't want to send to role by sending to each role user).
I agree that we can fix this, though.

crea’s picture

crea commented
Status: Needs review » Needs work

The patch makes query invalid: in GROUP BY queries every column should be either an aggregate function or present in GROUP BY