- Add new method privatemsg_message_load_multiple() which can load multiple messages in one query and then loops over the load hook.
- Renamed _privatemsg_load to privatemsg_load_message() to be consistent. this function is now simply a wrapper for multiple (this is what core is doing with user/node_load_multiple
- Added static cache for thread_load() - As I noticed it is called twice. I could also rip that into an own patch.

This kills a lot of queries, especially because of the static cache.. a lot user_load() and follow queries.

CommentFileSizeAuthor
#11 privatemsg.load_multiple6.patch6.3 KBBerdir
#9 privatemsg.load_multiple5.patch4.42 KBBerdir
#7 privatemsg.load_multiple4.patch4.89 KBBerdir
#6 privatemsg.load_multiple3.patch5.01 KBBerdir
#2 privatemsg.load_multiple2.patch4.93 KBBerdir
privatemsg.load_multiple.patch5.58 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

litwol’s picture

litwol CreditAttribution: litwol commented
Status: Needs review » Needs work

As per description of the patch, it seem to add more than one feature. I advise to split the patch per feature. It will become easier to review as well as easier to track issues if any become introduced.

Thanks.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
privatemsg.load_multiple2.patch4.93 KB

Re-roll, third item of that list splittet into an own patch at #444672: static cache for thread_load

NaheemSays’s picture

NaheemSays CreditAttribution: NaheemSays commented

Tested and this also works as expected, code also looks sane and well documented, but it is a more involved patch, so will leave at cnr for now.

NaheemSays’s picture

NaheemSays CreditAttribution: NaheemSays commented
Status: Needs review » Reviewed & tested by the community

I have been using this and 7 privatemsg other patches on my site for quite a few days now with no problems. Settings to rtbc.

NaheemSays’s picture

NaheemSays CreditAttribution: NaheemSays commented
Status: Reviewed & tested by the community » Needs work

This patch breaks messages/new/% with the following error:

* warning: array_fill() [function.array-fill]: Number of elements must be positive in C:\wamp\www\therevival\includes\database.inc on line 241.
* warning: implode() [function.implode]: Invalid arguments passed in C:\wamp\www\therevival\includes\database.inc on line 241.
* user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')) AND (pmi.uid = 3)' at line 1 query: SELECT pm.mid, pm.author, pm.subject, pm.body, pm.timestamp, pmi.is_new, pmbu.recipient AS is_blocked, pmi.thread_id FROM pm_message pm INNER JOIN pm_index pmi ON pm.mid = pmi.mid LEFT JOIN pm_block_user pmbu ON (pm.author = pmbu.author AND pmi.uid = pmbu.recipient) WHERE (pmi.mid IN ()) AND (pmi.uid = 3) in C:\wamp\www\therevival\sites\all\modules\privatemsg\privatemsg.module on line 1470.

Not sure how or why this patch does that as it seems to not touch the same areas.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
privatemsg.load_multiple3.patch5.01 KB

Added a check to see if an empty array is passed in to load_multiple.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
privatemsg.load_multiple4.patch4.89 KB

Tested for the wrong variable, should be fixed now.

NaheemSays’s picture

NaheemSays CreditAttribution: NaheemSays commented
Status: Needs review » Reviewed & tested by the community

Yes this works.

The problem was that the menu system was calling privatemsg_thread_load which was calling privatemsg_message_load_multiple without any pmid's and that was throwing up an error. That has been fixed.

I do think there is a deeper problem, probably with the core menu system though as messages/views/%privatemsg_thread should IMO NOT be acting on all Messages/%/% paths.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Reviewed & tested by the community » Needs review
FileSize
privatemsg.load_multiple5.patch4.42 KB

Ok, here is a re-roll of that patch.

Funny, I used ...2.patch but then re-discovered and re-fixed the empty $pmids bug again in pretty much the same way, just with a better comment I think, before looking at this issue :)

The re-roll was not easy as there were quite some changes, so this needs to be tested again, but it seems to work as expected for me. setting back to needs review.

NaheemSays’s picture

NaheemSays CreditAttribution: NaheemSays commented
Status: Needs review » Reviewed & tested by the community

This still works well and makes sense.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Reviewed & tested by the community » Needs review
FileSize
privatemsg.load_multiple6.patch6.3 KB

Thanks to the new tests, I was able to catch and fix a few bugs of the patch. Attached is a re-roll.

I want this for 1.0, but it's quite a deep change and I'd like to have some more tests/reviews from others.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Fixed

Added to 7.x-1.x-dev and 7.x-1.x-dev in the hope that the tests are good enough :) This should be a great performance inprovement for big threads.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.