messages-loop.php executes BP_Messages_Thread::populate() twice per conversation

[wpdennis]

messages-loop.php calls ​BP_Messages_Thread::populate() twice per conversation:

bp_has_message_threads()
=> BP_Messages_Box_Template->__construct()
=> BP_Messages_Thread::get_current_threads_for_user()
=> BP_Messages_Thread->__construct()

and

bp_message_thread_total_and_unread_count()
=> bp_get_message_thread_total_count(), 
=> BP_Messages_Thread_Template->__construct(), 
=> BP_Messages_Thread->__construct()

For 10 conversations this produces 20 db queries.

Related: I opened a ticket with the exact same problem for BP_Messages_Thread::get_recipients() #6220.

In get_recipients() caching would definitly be a good way, even if it is called just once per conversation.

On populate() I'm not so sure if object caching is the right answer. But if it's not, it shouldn't be called twice.

On the other hand... Even with caching, it's maybe worth a look if it can be avoided to create the BP_Messages_Thread object twice per conversation.

[boonebgorges]

In 9482:

Cache the results of BP_Messages_Thread::get_recipients().

This can save a large number of queries on certain inbox views.

Props wpdennis.
See #6221. Fixes #6220.

[boonebgorges]

[9482] adds caching for get_recipients().

Given that fix, I'm not sure how pressing it is to deal with the multiple calls to BP_Messages_Thread(). Strictly speaking, the objects in the two different calls you've listed here are different objects, so they have to go through the constructor twice. For single instantiations, we should of course try to ensure that __construct() is idempotent, but I don't think that's the case here.

wpdennis, in your view, does the caching fix in [9482] mitigate any other problems with building multiple BP_Messages_Thread objects for the same thread on a single pageload?

[wpdennis]

Oh no, I don't think it's pressing to reduce the amount of instance of BP_Messages_Thread. But while I was digging why there are so many queries, I saw that BP_Messages_Thread is created twice per conversation, which looked odd to me and I thought it's worth a look from you guys.

But if it stays like this, could populate() being cached? This line is executed twice per conversation:

if ( !$this->messages = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM {$bp->messages->table_name_messages} WHERE thread_id = %d ORDER BY date_sent " . $order, $this->thread_id ) ) ) {
	return false;
}

Resulting in 20 queries per page load for a standard inbox with 10 conversations. I'm not sure how often the data will change. At least I have no option in the UI to modify the sort order, so maybe just when messages are added or deleted?

If so, it could be cached permanently till invalidation. If not, maybe it could be cached at least non persistant? So it would only query the dabatase once per conversation?

[r-a-y]

Thanks for all the caching tickets for the Messages component, Dennis.

We're aware that this is one of the last components that barely has any caching. We'll make this a priority in 2.3.

[boonebgorges]

Replying to r-a-y:

Thanks for all the caching tickets for the Messages component, Dennis.

We're aware that this is one of the last components that barely has any caching. We'll make this a priority in 2.3.

+1. Thanks, wpdennis.

It seems unwise to cache the whole object, but certainly we should cache the 'messages' query you've referenced here.

[boonebgorges]

Looking a bit more closely at this, we should probably do the following:

1. Create a separate get_messages() method, which is called from populate().
2. Cache the *IDs* of the thread's messages, and add the appropriate busting logic (namely, when adding a message to a thread, when deleting a message from a thread, or when deleting a thread).
3. Cache individual messages individually. Then use our bp_get_non_cached_ids() logic to assemble a list of uncached messages, and get them in one fell swoop. (In other words, a split query.) This will allow us to use the same message cache (and invalidation logic) here and in BP_Messages_Message.

[r-a-y]

01.patch basically handles Boone's point 1 in comment:6.

• Introduces BP_Messages_Thread::get_messages() - caches fetching messages by thread ID
• Introduces BP_Messages_Thread::get_recipients_by_thread() - Static method so we can reference the existing get_recipients() method for easy access.
• Converts BP_Messages_Thread::check_access() and BP_Messages_Thread::is_valid() methods from using direct DB queries to reference the cache.
• Includes unit tests.
• Cache clearing functions are also consolidated rather than being separate (see bp-messages-cache.php).

This handles the majority of uncached message DB queries.

[boonebgorges]

Thanks for working on this, r-a-y.

Small request: Can we move toward using integers for cache keys, and more verbose cache groups? So wp_cache_set( $thread_id, 'bp_messages_threads' ) etc. It's more consistent, and makes it easier to do things like bp_get_non_cached_ids() in the future.

I see that you didn't do individual message caching, but the more I think about it, the more I guess it's probably not necessary. Messages are only shown in the context of threads, and so they'll always be able to take advantage of the thread cache. And messages can only belong to a single thread, so there's not much point in making smaller, more portable message caches. So this really covers my point (1), and replaces my suggestions (2) and (3) with a simplified version. Seems good to me.

[r-a-y]

Can we move toward using integers for cache keys, and more verbose cache groups?

Done in 02.patch. The cache keys in the first patch were based on the existing message recipients cache key using the 'bp' cache group.

I see that you didn't do individual message caching, but the more I think about it, the more I guess it's probably not necessary.

Exactly what I was thinking!

[boonebgorges]

Three thumbs up.

[r-a-y]

In 9752:

Messages: Add $thread_id parameter to BP_Messages_Thread::get_recipients().

Also added new static BP_Messages_Thread::get_recipients_for_thread()
method for convenience.

See #6221.

[r-a-y]

In 9753:

Messages: Cache calls to BP_Messages_Thread class.

This commit:

• Introduces static method - BP_Messages_Thread::get_messages() - caches fetching messages by thread ID and introduces the 'bp_messages_threads' cache group.
• Converts BP_Messages_Thread::check_access() and BP_Messages_Thread::is_valid() methods from using direct DB queries to reference the cache.
• Includes unit tests.
• Consolidates cache clearing functions in bp-messages-cache.php.

This handles the majority of uncached message DB queries. For the inbox
page, if there are 10 message threads in the loop, we save 20 DB queries.
On a single message thread, we save 5 DB queries.

Fixes #6221.