Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cache results from Imagick::queryFormats #6936

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from
+27 −4
Open

Cache results from Imagick::queryFormats #6936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d454fa8
Cache results from Imagick::queryFormats
joemcgill Jun 28, 2024
e017d8a
Create initial supports array.
joemcgill Jun 28, 2024
d0a1b15
Space invaders
joemcgill Jun 28, 2024
d1fc4ca
Merge remote-tracking branch 'upstream/trunk' into fix/61532-cache-im…
desrosj Aug 28, 2024
5d499ed
Revert changes to Imagick editor
joemcgill Oct 4, 2024
a86a618
Cache wp_image_editor_supports results
joemcgill Oct 4, 2024
c9297b7
Add new image_editor global cache group
joemcgill Oct 4, 2024
33bc1c0
Revert "Cache wp_image_editor_supports results"
joemcgill Oct 4, 2024
6056c51
Cache result of _wp_image_editor_choose
joemcgill Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/wp-includes/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,7 @@ function wp_start_object_cache() {
'blog-lookup',
'blog_meta',
'global-posts',
'image_editor',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to using a global cache group.

'networks',
'network-queries',
'sites',
Expand Down
28 changes: 24 additions & 4 deletions src/wp-includes/media.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4123,7 +4123,22 @@ function _wp_image_editor_choose( $args = array() ) {
* 'WP_Image_Editor_Imagick', 'WP_Image_Editor_GD'.
*/
$implementations = apply_filters( 'wp_image_editors', array( 'WP_Image_Editor_Imagick', 'WP_Image_Editor_GD' ) );
$supports_input = false;

$editors = wp_cache_get( 'wp_image_editor_choose', 'image_editor' );

if ( ! is_array( $editors ) ) {
$editors = array();
}

// Cache the chosen editor implementation based on specific args and available implementations.
$cache_key = md5( serialize( array( $args, $implementations ) ) );

if ( isset( $editors[ $cache_key ] ) ) {
Comment on lines +4127 to +4136
Copy link
Member

@felixarntz felixarntz Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what's the rationale for nesting all the cache values in a single cache key "bucket". In most places in Core we store individual values per cache key, and the dynamic portion becomes part of the actual cache key.

For example the actual cache key would be:

$cache_key = 'wp_image_editor_choose_' . md5( serialize( array( $args, $implementations ) ) );

Since these caches would expire after 1 day anyway, I don't think there's any harm in storing in individual cache keys. This approach here might work too, but there's potential concerns about scalability (if too many different combinations of arguments are all stored under a single cache key) and diverging from how I believe we typically handle such caches in Core.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have any real intention when I chose to implement it this way, but I can see a few benefits.

Generally, including the hash as part of the name is done for the purpose of avoiding stale caches (i.e., if the values that are hashed change, the wp_get_cache() call will result in a cache miss). That's not really a concern for this cache because the need to store the data for long periods while avoiding returning stale values is not that high.

This specific function can get called multiple times during the same run with different args. Having the results stored in multiple distinct caches would mean that we need to load separate cache values for each specific set of args. By saving all of the values to one cache, all of the cached values are loaded to memory the first time _wp_image_editor_choose() is called, and can be accessed from memory on each subsequent call.

Another minor benefit is that it makes it a bit easier to work with a cache that has a static name in the DB instead of a dynamic one. As a theoretical example you could do something like wp cache get wp_image_editor_choose image_editor or wp cache delete wp_image_editor_choose image_editor to review or remove the values, which is more difficult with a dynamic name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me, makes sense. Let's go with this approach then.

return $editors[ $cache_key ];
}

// Assume no support until a capable implementation is identified.
$editor = false;

foreach ( $implementations as $implementation ) {
if ( ! call_user_func( array( $implementation, 'test' ), $args ) ) {
Expand Down Expand Up @@ -4157,15 +4172,20 @@ function _wp_image_editor_choose( $args = array() ) {
* This implementation supports the input type but not the output type.
* Keep looking to see if we can find an implementation that supports both.
*/
$supports_input = $implementation;
$editor = $implementation;
continue;
}

// Favor the implementation that supports both input and output mime types.
return $implementation;
$editor = $implementation;
break;
}

return $supports_input;
$editors[ $cache_key ] = $editor;

wp_cache_set( 'wp_image_editor_choose', $editors, 'image_editor', DAY_IN_SECONDS );

return $editor;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions src/wp-includes/ms-blogs.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,7 @@ function switch_to_blog( $new_blog_id, $deprecated = null ) {
'blog-lookup',
'blog_meta',
'global-posts',
'image_editor',
'networks',
'network-queries',
'sites',
Expand Down Expand Up @@ -648,6 +649,7 @@ function restore_current_blog() {
'blog-lookup',
'blog_meta',
'global-posts',
'image_editor',
'networks',
'network-queries',
'sites',
Expand Down
Loading