Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 4 years ago

#6220 closed enhancement (fixed)

Cache BP_Messages_Thread::get_recipients() to reduce inbox queries by 20

Reported by: wpdennis Owned by: boonebgorges
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Messages Keywords: has-patch commit
Cc: info@…

Description

The inbox (and sentbox) calls BP_Messages_Thread::get_recipients() twice per conversation in messages-loop.php. Once via bp_has_message_threads() and once via bp_message_thread_total_and_unread_count().

So, in an inbox/sentbox with 10 conversations, that are 20 queries on each page load. I added some caching to get_recipients(), to remove these 20 queries:

public function get_recipients() {
	$recipients = wp_cache_get( 'recipients-' . $this->thread_id, 'bp_messages' );

	if ( false === $recipients ) {
		global $wpdb, $bp;

		$results = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM {$bp->messages->table_name_recipients} WHERE thread_id = %d", $this->thread_id ) );

		foreach ( (array) $results as $recipient ) {
			$recipients[$recipient->user_id] = $recipient;
		}

		wp_cache_set( 'recipients-' . $this->thread_id, $recipients, 'bp_messages' );
	}

	/**
	 * Filters the recipients of a message thread.
	 *
	 * @since BuddyPress (2.2.0)
	 *
	 * @param array $recipients Array of recipient objects.
	 * @param int   $thread_id  ID of the current thread.
	 */
	return apply_filters( 'bp_messages_thread_get_recipients', $recipients, $this->thread_id );
}

Is there a way to change the recipient in a conversion where we would need to add some cache invalidation? I haven't found one, except maybe the deletion of an user?

Attachments (1)

6220.read.patch (3.3 KB) - added by r-a-y 4 years ago.

Download all attachments as: .zip

Change History (8)

#1 @wpdennis
5 years ago

While digging deeper, I opened a second ticket: #6221

I think we have two issues here:

a) messages-loop.php calls things in a way, where redundant code (with db queries) is executed twice. This could be worth a look on its own and if so, #6221 would be the better ticket to adress this issue.

b) get_recipients() should be trivial to cache, since the data is really persistant. So we could save at minimum 10 queries in any scenario and in the current implementation even 20 queries. So we could keep this ticket just for the caching of get_recipient(), despite possible changes to messages-loop.php.

#2 @boonebgorges
5 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 2.3

Very good catch on get_recipients(). We should definitely invalidate cache on thread deletion. I'm not sure whether it's possible in the BP interface to add a user to a thread when sending a subsequent message, but it's possible in the data model, so to be safe let's also invalidate a thread's recipient when a message is added to the thread.

#3 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

#4 @r-a-y
4 years ago

  • Keywords has-patch added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Need to bust the recipient cache whenever a thread is read / unread.

See 6220.read.patch.

@r-a-y
4 years ago

#5 @boonebgorges
4 years ago

  • Keywords commit added
  • Milestone 2.3 deleted
  • Status changed from reopened to closed

#6 @boonebgorges
4 years ago

  • Milestone set to 2.3
  • Status changed from closed to reopened

#7 @r-a-y
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 9714:

Messages: Bust recipient cache when a message thread is marked as read or unread.

See r9482, which introduces message recipient caching.

Fixes #6220.

Note: See TracTickets for help on using tickets.