-
Notifications
You must be signed in to change notification settings - Fork 799
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
Add caching for VideoPress functions with meta queries #14803
Conversation
These involve meta queries, so it'd be good to have some kind of caching.
@@ -108,8 +108,8 @@ public function __construct( $guid, $maxwidth = 0, $options = array() ) { | |||
|
|||
if ( ! defined( 'WP_DEBUG' ) || WP_DEBUG !== true ) { | |||
$expire = 3600; | |||
if ( isset( $video->expires ) && is_int( $video->expires ) ) { | |||
$expires_diff = time() - $video->expires; | |||
if ( isset( $this->video->expires ) && is_int( $this->video->expires ) ) { |
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.
With unset( $video );
above, it looks like isset( $video->expires )
will never be true
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.
Nice catch.
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: April 7, 2020. |
@@ -655,9 +678,13 @@ function video_get_post_by_guid( $guid ) { | |||
|
|||
$query = new WP_Query( $args ); | |||
|
|||
$post = $query->next_post(); |
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.
There can be an error here if the query didn't find any posts.
tests/php/modules/videopress/test_functions.utility-functions.php
Outdated
Show resolved
Hide resolved
@@ -111,7 +117,9 @@ function videopress_get_attachment_id_by_url( $url ) { | |||
$cropped_files = wp_list_pluck( $meta['sizes'], 'file' ); | |||
|
|||
if ( $original_file === $file || in_array( $file, $cropped_files ) ) { | |||
return (int) $attachment_id; | |||
$attachment_id = (int) $attachment_id; | |||
wp_cache_set( $cache_key, $attachment_id, $cache_group, HOUR_IN_SECONDS ); |
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.
Maybe the cache expiration time should be different
$file = basename( $url ); | ||
$cache_key = 'videopress_get_attachment_id_by_url_' . md5( $url ); | ||
$cache_group = 'videopress'; | ||
$cached_id = wp_cache_get( $cache_key, $cache_group ); |
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.
Quick question; did you consider using transients instead of object caching? Would it be better for performance on sites that do not use caching plugins?
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.
Good question, I'll look at that.
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.
Sure, I could add a transient here if you'd like.
But since the attachment ID is from a database query (WP_Query), I'm not sure that using a transient to store it in the database (wp_options) would be the best approach.
It would probably improve performance for sites without persistent object caching. But it could fill the wp_options
table.
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.
@kienstra Expiring transients aren't autoloaded and are deleted once they expire.
Do you think we need to worry about polluting wp_options
under these circumstances?
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.
@kienstra Also, is videopress_get_attachment_id_by_url
ever called?
I think the original usage was in videopress_shortcode_override_for_core_shortcode
(771097d), but has been superseded by 32dbcd7, but the method itself has never been removed.
I ran git log -p -S videopress_get_attachment_id_by_url
to confirm.
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.
- Use transients, but store only the post ID
- We could use the object cache to store the post object, avoiding multiple calls to get_post() if we want.
Thanks, what do you think about the latest commits, which apply those 2 points?
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.
And to wrap things up, I'd get the post ID in a separate method, which would be a direct replacement for the method we are deprecating :) .
Hm, did you have in mind something like:
/**
* Using a GUID, find a post ID.
*
* @param string $guid The guid to use to find the post ID.
* @return int|false The post ID.
*/
function video_get_post_id_by_guid( $guid ) {
$post = video_get_post_by_guid( $guid );
if ( is_object( $post ) && 'WP_Post' === get_class( $post ) ) {
return $post->ID;
}
return false;
}
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.
Thanks, what do you think about the latest commits, which apply those 2 points?
What I thought is that we can use both transients and wp_cache at the same time, each one for a purpose. Transients to store the post ID, so we avoid making the costly SQL SELECT many times though several requests. Object Caching to avoid calling get_post()
several times within the same request.
Hm, did you have in mind something like:
Not exactly. I would actually leave the WP_Query
call in video_get_post_id_by_guid
, adding the 'fields' => 'ids'
. I would call it from video_get_post_by_guid
and then call get_post
. So I would have the same behavior every time. (Always fetch the ID and then call get_post
)
Looks a bit cleaner to me. Each method doing just one thing, and avoiding things like $cached_post
sometimes holding the actual post, and sometimes holding just the post id.
Also it's important to add an expiration time for the transient.
This is how I see it. (Please consider this just as an opinion!)
function video_get_post_by_guid( $guid ) {
// ...
$cached_post = wp_cache_get( $cache_key, $cache_group );
if ( $cached_post ) {
return $cached_post;
}
$post_id = $this->video_get_post_id_by_guid( $guid );
if ( is_int( $post_id ) ) {
$post = get_post( $cached_post );
wp_cache_set( $cache_key, $post, $cache_group, HOUR_IN_SECONDS );
if ( $post ) {
return $post;
}
}
}
function video_get_post_id_by_guid( $guid ) {
// ...
$cached_id = get_transient( $cache_key );
if ( $cached_id ) {
return $cached_id;
}
// do the query
set_transient( $cache_key, $id, HOUR_IN_SECONDS );
return $id;
}
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.
Thanks, and sorry for the delay. I'm working on this.
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.
How does this look? It mainly uses your snippet above.
tests/php/modules/videopress/test_functions.utility-functions.php
Outdated
Show resolved
Hide resolved
As Jeremy mentioned, 8.3.0 now has a code freeze.
Hi @dero, Could you please review this? |
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.
@kienstra Changes look good. I had a question about transients vs. wp_cache_* usage and I also think one of the methods that receives caching is no longer used in the codebase and can be removed.
$file = basename( $url ); | ||
$cache_key = 'videopress_get_attachment_id_by_url_' . md5( $url ); | ||
$cache_group = 'videopress'; | ||
$cached_id = wp_cache_get( $cache_key, $cache_group ); |
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.
@kienstra Expiring transients aren't autoloaded and are deleted once they expire.
Do you think we need to worry about polluting wp_options
under these circumstances?
$file = basename( $url ); | ||
$cache_key = 'videopress_get_attachment_id_by_url_' . md5( $url ); | ||
$cache_group = 'videopress'; | ||
$cached_id = wp_cache_get( $cache_key, $cache_group ); |
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.
@kienstra Also, is videopress_get_attachment_id_by_url
ever called?
I think the original usage was in videopress_shortcode_override_for_core_shortcode
(771097d), but has been superseded by 32dbcd7, but the method itself has never been removed.
I ran git log -p -S videopress_get_attachment_id_by_url
to confirm.
As Dero mentioned, it looks like this isn't used in Jetpack.
As Jeremy mentioned, this should give third-party developers a chance to change to something else.
Marking for author reply pending resolution of the |
Thanks, I responded above. |
Also, store the full WP_Post in the cache, if it is available.
There is actually an existing function that does the same thing.
Add video_get_post_id_by_guid(), and a unit test for it.
To correspond to its position in utility-functions.php.
Following Leo's snippet, this should have an expiration.
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.
Hi @kienstra ,
Thanks a lot for the changes.
There are few small things though:
- Use the
fields => ids
param in the query. Since this PR is all about performance, this is a must-have. - Add the
@since
keywork to the new functions. (see https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#1-functions-class-methods) - Now we have a perfect replacement for that deprecated function! Add there in the
@see
keyword there with our new function. - I see that this file have functions prefixed both with
video
and withvideopress
. Most of them withvideopress
and, since this is the name of the module, we should stick to it. Those functions that are using onlyvideo
as prefix we can review another time. But I think we should enforce the most appropriate prefix. So, change the prefix of the new functions tovideopress
tests/php/modules/videopress/test_functions.utility-functions.php
Outdated
Show resolved
Hide resolved
The see tag points to a possible replacement. Also, the DocBlock summary references the wrong function.
Also change the transient cache key. Also, simply return the post, instead of accessing the ->ID property.
OK, is 64918b1 what you had in mind? It renames |
videopress_get_post_id_by_guid() now has a @SInCE tag, in addition to other dataProviders
This applied when it was part of video_get_post_by_guid(), but not to the new function it's in.
Hi @leogermani, |
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.
Hi @kienstra ,
Thanks for all the work, it looks great now!
I had a quick chat with the team and we thought it was worth to rename one of the old functions to have a bit more consistency. I already did it since it doesn't change anything with the logic you implemented.
Hi @leogermani,
Sounds good, thanks for pushing that change. |
* Initial changelog entry * Changelog: add #14904 * Changelog: add #14910 * Changelog: add #14913 * Changelog: add #14916 * Changelog: add #14922 * Changelog: add #14924 * Changelog: add #14925 * Changelog: add #14928 * Changelog: add #14840 * Changelog: add #14841 * Changelog: add #14842 * Changelog: add #14826 * Changelog: add #14835 * Changelog: add #14859 * Changelog: add #14884 * Changelog: add #14888 * Changelog: add #14817 * Changelog: add #14814 * Changelog: add #14819 * Changelog;: add #14797 * Changelog: add #14798 * Changelog: add #14802 * Changelog: add #13676 * Changelog: add #13744 * Changelog: add #13777 * Changelog: add #14446 * Changelog: add #14739 * Changelog: add #14770 * Changelog: add #14784 * Changelog: add #14897 * Changelog: add #14898 * Changelog: add #14968 * Changelog: add #14985 * Changelog: add #15044 * Changelog: add #15052 * Update to remove Podcast since it remains in Beta * Changelog: add #14803 * Changelog: add #15028 * Changelog: add #15065 * Changelog:add #14886 * Changelog: add #15118 * Changelog: add #14990 * Changelog: add #14528 * Changelog: add #15120 * Changelog: add #15126 * Changelog: add #15049 * Chanegelog: add #14852 * Changelog: add #15090 * Changelog: add #15138 * Changelog: add #15124 * Changelog:add #15055 * Changelog: add #15017 * Changelog: add #15109 * Changelog: add #15145 * Changelog:add #15096 * Changelog:add #15153 * Changelog: add #15133 * Changelog: add #14960 * Changelog: add #15127 * Changelog: add #15056 * Copy current changelog to changelog archive. * Clarify changelog description
Changes proposed in this Pull Request:
WP_Query
WP_Query
to improve performance, like'no_found_rows' => true
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
(Regression testing for VideoPress)
Ensure VideoPress is enabled:
Create a new post
Add a Video block
Upload a video to it
Expected: the video plays in the editor.
Expected: the video plays on the front-end
Proposed changelog entry for your changes: