mirrored from git://develop.git.wordpress.org/
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lazy load post meta #4216
Open
spacedmonkey
wants to merge
28
commits into
WordPress:trunk
Choose a base branch
from
spacedmonkey:fix/lazy-load-post-meta
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Lazy load post meta #4216
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
e77768b
Lazy load post meta
spacedmonkey a9e45a1
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey b5f297e
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey efcdbc7
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey 85ad4ae
Rebase
spacedmonkey 9f19cac
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey 77fa22e
Fix
spacedmonkey 753379d
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey 093b224
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey ef8e797
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey b2e72e1
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey 657aebb
Update callback name and implement lazyload_post_meta function
spacedmonkey ee9e65e
Update expected database queries in tests
spacedmonkey 14aec68
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey d6d9934
Add filter for REST API menu read access check
spacedmonkey 8f33b9a
Update post meta cache function call
spacedmonkey e61d194
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey 55b32c3
Revert these changes.
spacedmonkey 4330c69
Apply suggestions from code review
spacedmonkey 4f90281
Merge branch 'trunk' into fix/lazy-load-post-meta
spacedmonkey 046cdf5
Add $lazy_load_post_meta parameter to WP_Query and use it in post con…
spacedmonkey cbe16f1
Add $lazy_load_post_meta parameter in wp_get_post_revisions.
spacedmonkey dc7344a
Fix tests.
spacedmonkey a077f46
Improve tests.
spacedmonkey f4ec367
Fix tests again.
spacedmonkey 59a3aee
Add tests.
spacedmonkey a6b8d34
wp_dashboard_recent_posts lazy load post meta.
spacedmonkey 4e3a8ec
Apply suggestions from code review
spacedmonkey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
<?php | ||
|
||
/** | ||
* @group query | ||
* @group meta | ||
*/ | ||
class Tests_Lazy_Load_Post_Meta extends WP_UnitTestCase { | ||
/** | ||
* Post IDs. | ||
* | ||
* @var int[] | ||
*/ | ||
private static $post_ids = array(); | ||
|
||
public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) { | ||
// Register CPT for use with shared fixtures. | ||
register_post_type( 'wptests_pt' ); | ||
|
||
self::$post_ids = $factory->post->create_many( 5, array( 'post_type' => 'wptests_pt' ) ); | ||
foreach ( self::$post_ids as $post_id ) { | ||
update_post_meta( $post_id, 'foo', 'bar' ); | ||
} | ||
} | ||
|
||
/** | ||
* @dataProvider data_lazy_load_post_meta | ||
* @ticket 57496 | ||
*/ | ||
public function test_lazy_load_post_meta( $query_args ) { | ||
wp_cache_delete_multiple( self::$post_ids, 'posts' ); | ||
wp_cache_delete_multiple( self::$post_ids, 'post_meta' ); | ||
$action1 = new MockAction(); | ||
$action2 = new MockAction(); | ||
add_filter( 'update_post_metadata_cache', array( $action1, 'filter' ), 10, 2 ); | ||
add_action( 'metadata_lazyloader_queued_objects', array( $action2, 'action' ) ); | ||
|
||
new WP_Query( $query_args ); | ||
|
||
$args1 = $action1->get_args(); | ||
$args2 = $action2->get_args(); | ||
$last = end( $args2 ); | ||
$this->assertSameSets( self::$post_ids, $last[0], 'wp_lazyload_post_meta() was not executed.' ); | ||
$this->assertSameSets( array(), $args1, 'update_meta_cache() was executed.' ); | ||
$num_queries = get_num_queries(); | ||
get_post_meta( self::$post_ids[0], 'foo', true ); | ||
$this->assertSame( $num_queries + 1, get_num_queries(), 'wp_lazyload_post_meta() was not executed.' ); | ||
$args1 = $action1->get_args(); | ||
$last = end( $args1 ); | ||
$this->assertSameSets( self::$post_ids, $last[1], 'update_meta_cache() was not executed.' ); | ||
} | ||
|
||
/** | ||
* Provides test data for lazy loading post metadata. | ||
* | ||
* @return array | ||
*/ | ||
public function data_lazy_load_post_meta() { | ||
return array( | ||
'lazy load post meta' => array( | ||
array( | ||
'post_type' => 'wptests_pt', | ||
'lazy_load_post_meta' => true, | ||
), | ||
), | ||
'lazy load post meta fields id' => array( | ||
array( | ||
'post_type' => 'wptests_pt', | ||
'fields' => 'ids', | ||
'lazy_load_post_meta' => true, | ||
), | ||
), | ||
'lazy load post meta fields id=>parent' => array( | ||
array( | ||
'post_type' => 'wptests_pt', | ||
'fields' => 'id=>parent', | ||
'lazy_load_post_meta' => true, | ||
), | ||
), | ||
'lazy load post meta - update_post_meta_cache true' => array( | ||
array( | ||
'post_type' => 'wptests_pt', | ||
'update_post_meta_cache' => true, | ||
'lazy_load_post_meta' => true, | ||
), | ||
), | ||
'lazy load post meta - update_post_meta_cache false' => array( | ||
array( | ||
'post_type' => 'wptests_pt', | ||
'update_post_meta_cache' => false, | ||
'lazy_load_post_meta' => true, | ||
), | ||
), | ||
'lazy load post meta - cache_results false' => array( | ||
array( | ||
'post_type' => 'wptests_pt', | ||
'cache_results' => false, | ||
'lazy_load_post_meta' => true, | ||
), | ||
), | ||
); | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole method is a line for line duplication of
lazyload_meta_callback()
except for this line. Why is the logic different here, and could this be simplified by adding a check for$meta_type
to return the$check
rather than adding the$object_id
to the array of IDs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joemcgill This is the secret sause of this change. Unlike other meta types like, site, comment and term, post meta gets called ALOT. So to stop every call to post meta loading this lazy load queue, this only loads if the queue if an item is in the post meta queue.
For, if I have 10 items of post meta loaded and 5 items in the lazy load queue, if you request an post id in the load meta, you DONT want the queue items to load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey I think the logic here should be slightly different. If the object ID passed to
get_post_meta()
is already cached, then it makes sense not to load the queued data. If the object ID is not cached, then a database call will be made and it makes sense to load the queued data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc that is not how the other meta types work. IDs are put in the queue. If the id is already loaded, when wp_cache_get_multiple, is called, if it is only object is not cache is loaded.
I don't see lots of cases where an id in the queue but also primed. I expect that ids that might be used will be added to the queue, for example attachment meta or post parent meta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey I realise that is not how it works for other meta data (potentially a bug), but you are proposing that post meta works differently.
However, the code in it's current form will trigger a database query for
get_post_meta( $uncached_post_id )
without priming other data in the queue. As the idea of lazy loading meta data is to ride another database query, it seems we should do that here even if the uncached object is not queued.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterwilsoncc So you are saying, we should not have special behavour for post meta, it should work like other meta? That every time
get_post_meta
is called it should clear the queue? If that is the case, as post meta is called so much, then there is no performance benefit. The queue would be cleared so much, that we might as well not bother.It is less about saving database queries I guess. It useful in the following use case.
I have loaded 5 posts and there meta. But I have also loaded, all those post parents, ( say for revisions or attachments ). Also add the parents post meta to the queue, as it MAY get used. In cases, where it is, load them all at once, in other cases the items in the queue may never get loaded, saving a database query. There are LOTs of places in core, where post meta is loaded, because it MAY be used a plugin, ( example if you have a filter of the title to change it a value loaded for post meta ). But core itselve is not using this post meta. Once we have a queue, where it says maybe load post meta, we can stop priming post meta in lots of places where it is not required by core, but turning off the priming might effect sites with plugins that except the post meta to be loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not what I am saying.
What I am saying is that IF a database query is required then the queue should be cleared. For example:
Cached post meta IDs: 1, 2, 3, 4
Queued post meta IDs: 5, 6, 7, 8
get_post_meta( 1 )
would not clear the queue as it does not require a database callget_post_meta( 10 )
does require a database call so would use the opportunity to clear the queue and prime the cache for the queued meta data.