Make WordPress Core

Opened 15 months ago

Closed 14 months ago

Last modified 11 months ago

#57901 closed enhancement (fixed)

Only process queue in lazy loading meta api, if request id is in queue.

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version: 4.5
Component: Options, Meta APIs Keywords: has-patch has-dev-note
Focuses: performance Cc:

Description

The existing lazy loading meta api, creates a queue of ids, to be primed, if the get_comment_meta or get_term_meta functions are called. Once they it is called for the first time, then queue is primed in cache and the queue is clear. However, this does not check to see the request piece of meta, was already in the queue. Take this scenario for example.

  1. Add 3 ids to the lazy load queue, 123, 456, 789.
  2. Call get_term_meta, with id 444.
  3. This will prime all term meta in lazy load queue and not prime 444.

There should be a simple check in check processing the queue, to see in requested id is in the queue.

Attachments (2)

Screenshot 2023-03-20 at 10.48.03.png (341.5 KB) - added by spacedmonkey 14 months ago.
Trunk, that does two database queues.
Screenshot 2023-03-20 at 10.49.02.png (38.7 KB) - added by spacedmonkey 14 months ago.
In this PR. One database query instead two database queries

Download all attachments as: .zip

Change History (13)

#1 @spacedmonkey
15 months ago

  • Summary changed from Improvement the performance of lazy loading meta api to Only process queue in lazy loading meta api, if request id is in queue.

This ticket was mentioned in PR #4211 on WordPress/wordpress-develop by @spacedmonkey.


15 months ago
#2

  • Keywords has-patch added

#3 @peterwilsoncc
14 months ago

I'm not sure this is a problem.

The goal of lazy loading is to reduce the number of database queries by reducing the number of queries to the *meta table.

If the IDs presence is checked before lazy loading, the following code will result in an increase in db queries:

<?php
// Queue meta for terms 123, 456, 789 for lazy loading.
get_term_meta( 444 ); // Query 1 with and without change.
get_term_meta( 123 ); // Query 2 in changed code, no query in existing code.

This seems worse to me.

#4 @spacedmonkey
14 months ago

@peterwilsoncc Not sure what you mean here. The goal of this change.

If the term id is not a list of id to prime in cache, do not bother to prime them. If you load a single term meta for one id that is not in the queue to prime, then it may load data is very uses.

Take the following example.

get_term_meta( 999999 );

If 999999, you may load other term meta for now reason. See attached screenshots.

Another solution to this problem, is dynamically adding the missing id to the queue, but again, this may result in loading data that is not needed.

@spacedmonkey
14 months ago

Trunk, that does two database queues.

@spacedmonkey
14 months ago

In this PR. One database query instead two database queries

#5 @peterwilsoncc
14 months ago

Gotcha, in that case I think it better to dynamically add the ID of the current object the the lazy loading method:

<?php
public function lazyload_term_meta( $check, $object_id ) {
        if ( ! empty( $this->pending_objects['term'] ) ) {
                $object_ids = array_keys( $this->pending_objects['term'] );
                if ( ! in_array( $object_id, $object_ids, true ) ) {
                        $object_ids[] = $object_id;
                }
                update_termmeta_cache( $object_ids );

                // No need to run again for this set of terms.
                $this->reset_queue( 'term' );
        }

        return $check;
}

To my mind, queuing meta data for lazy loading is to say: only query the meta data table if there is an indication a theme or plugin is using the meta data for that object type.

Calling get_type_meta() for any object ID, whether or not it's queued, is that indication.

Another solution to this problem, is dynamically adding the missing id to the queue, but again, this may result in loading data that is not needed.

I acknowledge that is a risk, but it's always been there. Using QM's profiling feature didn't show a great deal of extra memory usage in my quick testing.

#6 @spacedmonkey
14 months ago

I have updated the PR based on @peterwilsoncc. I am happy with the current solution.

#7 @spacedmonkey
14 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#8 @spacedmonkey
14 months ago

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

In 55608:

Options, Meta APIs: Improve the lazy loading meta API to include current object id.

The existing lazy loading meta api, creates a queue of ids, to be primed, if the get_comment_meta or get_term_meta functions are called. However, it did not check to see if the requested id was in the queue, before prime all the ids in the queue. Now, it adds the id to the queue, is not already in the queue, saving a cache lookup / database query.

Props spacedmonkey, peterwilsoncc, mukesh27, flixos90.
Fixes #57901.

#10 @spacedmonkey
11 months ago

  • Keywords needs-dev-note added; commit removed
Note: See TracTickets for help on using tickets.