WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 22 months ago

#20875 assigned enhancement

Introduce wp_cache_get_multi()

Reported by: nacin Owned by: boonebgorges
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Cache API Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: performance Cc:
PR Number:

Description

Both options (see #10274) and themes (see #20103) could benefit from a cache backend that implements get_multi(). For options, this means we can use individual keys. For both themes and options, we'd be able to make fast multiple gets.

Both APC and Memcached (but not Memcache) implement multiple-get. A fallback would be looping over get().

Separately, as this would be a new function we use in core, we probably need to start doing some kind of versioning for drop-ins like db.php and object-cache.php.

Attachments (11)

20875.diff (3.3 KB) - added by tollmanz 7 years ago.
UT20875.diff (5.1 KB) - added by tollmanz 7 years ago.
20875.2.diff (2.4 KB) - added by wonderboymusic 7 years ago.
20875.3.diff (2.5 KB) - added by boonebgorges 3 years ago.
20875-example-usage.diff (1.5 KB) - added by boonebgorges 3 years ago.
20875-implementation-apc.php (1.6 KB) - added by boonebgorges 3 years ago.
20875.4.diff (3.5 KB) - added by boonebgorges 3 years ago.
20875.5.diff (3.8 KB) - added by spacedmonkey 22 months ago.
20875-update_object_term_cache.diff (824 bytes) - added by spacedmonkey 22 months ago.
20875-update_meta_cache.diff (1.2 KB) - added by spacedmonkey 22 months ago.
20875-_get_non_cached_ids.diff (1.0 KB) - added by spacedmonkey 22 months ago.

Download all attachments as: .zip

Change History (47)

#1 @tollmanz
7 years ago

  • Cc zack@… added

@tollmanz
7 years ago

@tollmanz
7 years ago

#2 @tollmanz
7 years ago

  • Cc tollmanz@… added
  • Keywords has-patch added

I imagine that there is a lot to be discussed about how this should work, but I am adding a patch to move things along. I added a first attempt at a get_multi function with this patch. Additionally, I have written unit tests and uploaded them here (would appreciate some direction on if these should also be added to the unit test trac and what's the right way to do that organizationally).

This get_multi function uses the get function to retrieve values from the $cache var. An array of keys and groups are sent to the function. The function attempts to figure out the relationship between the $keys and $groups values. The logic is:

  • If $keys and $groups are both strings, it is assumed to be a simple get.
  • If $keys and $groups are arrays and are the same length, $keys[n] is assumed to be in $groups[n].
  • If there are more $groups than $keys, $keys[n] is assumed to be in $groups[n] until all values in $groups is exhausted. At that point, the remaining $keys values are assumed to belong to the group 'default'.
  • If $keys is an array and $groups is a string or single value array, all $keys are assumed to be in $groups (if string) or $groups[0] (if array).

Values are returned as an array with top level keys being groups. The second level keys are the cache key values. This should match the organization within the $cache var itself.

A number of different scenarios have been tested in the unit tests and this seems to be working well for the assumptions I have laid out.

#3 @wonderboymusic
7 years ago

I cribbed the Memcache code to implement it here: http://wordpress.org/extend/plugins/memcached-redux/

#4 @aaroncampbell
7 years ago

  • Cc aaroncampbell added

#5 @ryan
7 years ago

For back compat reasons, the API here should handle the call signature used in the memcached plugin. Looks like the patch might already handle that. http://plugins.svn.wordpress.org/memcached/trunk/object-cache.php

Last edited 7 years ago by ryan (previous) (diff)

#6 @wonderboymusic
7 years ago

  • Milestone changed from Awaiting Review to 3.6

I squeezed the code into new code - I don't think the return value needs to be associative and bucketed. I think a standard use-case for get_multi in action is:

list( $one, $two, $three ) = wp_cache_get_multi( array( 'one', 'two', 'three' ), 'numbers' );

#7 @ryan
7 years ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release

#8 @wonderboymusic
6 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#9 @nacin
6 years ago

  • Keywords early added; 3.7-early removed
  • Milestone changed from 3.7 to Future Release

Holding off on this until ryan can weigh in. We don't have an acute need for it in core.

#10 @chriscct7
4 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Lack of interest in 2 years. Closing as maybelater. Feel free to reopen if interest re-emerges and there is a demonstrable need for it in core (per comment:9)

#11 @jeremyfelt
4 years ago

  • Milestone set to Future Release
  • Resolution worksforme deleted
  • Status changed from closed to reopened

While there hasn't been an immediate need for multi-get, I don't think it's a non-interest yet. While we may not end up using it in core directly, having it as part of the API could be beneficial to developers.

#12 @spacedmonkey
4 years ago

+1 to what @jeremyfelt said. I know a number of plugins already using get multi. Some of these are plugins provided my Automattic. Having to write logic to check if get multi exists is a pain. It would be a big win for me if this goes in, as I don't have write those checks anymore and forces object cache implementations to have these functions available.

#14 @boonebgorges
3 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch early removed

#36953 is a concrete use case for wp_cache_get_multi() in core.

How can we move this ticket forward? Since it looks like plugins and cache drop-ins are already implementing wp_cache_get_multi(), maybe a good place to start is to do an inventory to understand what the interface has got to look like. I'm not sure how wp_cache_get_multi( $keys, $groups ) would work, since individual keys are group-specific. https://github.com/ericmann/Redis-Object-Cache/blob/master/object-cache.php#L120 seems like it has to be the correct function signature.

It would be helpful to get developers of some popular cache plugins looped into the conversation, so that we can all be on the same page with function signatures, etc.

@boonebgorges
3 years ago

#16 @boonebgorges
3 years ago

  • Keywords has-patch has-unit-tests 2nd-opinion added; needs-patch needs-unit-tests removed
  • Milestone changed from Future Release to 4.6

A couple of patches, in the interest of moving this ticket along.

20875.3.diff is a core patch that shows what the default wp_cache_get_multi() might look like. Instead of the ( $keys, $groups ) syntax suggested by @tollmanz, I've gone for the more straightforward multidimensional array:

$cached = wp_cache_get_multi( array(
    'cache_group_1' => array( 'foo', 'bar' ),
    'cache_group_2' => array( 'baz' ),
) );

This syntax is more verbose in many cases, but (a) it's more consistent and thus clearer, and (b) it better parallels the format of the array returned by the function. This format is what's expected by https://github.com/ericmann/Redis-Object-Cache/blob/master/object-cache.php#L546 and https://plugins.svn.wordpress.org/memcached/trunk/object-cache.php, but different from what's expected by https://github.com/mgmartel/memcached-redux/blob/master/object-cache.php#L256 and https://github.com/tollmanz/wordpress-pecl-memcached-object-cache/blob/master/object-cache.php#L1384 (the latter two expect @tollmanz's syntax, or some subset of it). We are going to have to break compatibility with something, so I'd suggest we choose the syntax we like the best. I prefer what's in my patch, but I don't feel strongly about it.

20875-example-usage.diff is an example of how it'd be used in core. The example is from #36953. This is a particularly interesting example because it's fetching from lots of cache *groups* at the same time. (If we go forward with this ticket, I'd move the logic from this patch into get_object_term_cache() - the patch is for demonstration only.)

20875-implementation-apc.php is a totally untested PoC patch for the APC drop-in.

I haven't accounted for the $force or $found parameters that get() has. It's hard to deal with them without making the syntax even more convoluted. Worth noting that the Redis and Memcached plugins listed above also do not support these flags, for get_multi() *or* for get().

It's still #yolofriday where I live, so I'm putting this up for 4.6 consideration.

#17 @spacedmonkey
3 years ago

I think it is important this is in core ASAP. Third party plugins have supported get multi for years and other plugin providers already started using the function. Example is this.

https://github.com/Automattic/advanced-post-cache/blob/master/advanced-post-cache.php#L128

Get multi needs to be in core for the simple reason that a consistent api needed in place to stop future issues with plugins like the above.

#18 @dd32
3 years ago

cross-linking to #31245 which has a lot of input on multi_get(), https://core.trac.wordpress.org/ticket/31245#comment:6 is an example of how splintered the current ecosystem is.

I'm hesitant to comment here, as I think it should be handled in #31245 simply due to the amount of discussion there (or someone should summarize that ticket here, and direct all further discussion here).

I think the only route forward here will be to find a new name, as the wp_cache_get_multi() function signature is just not viable thanks to existing users (even the ones which have insane return values)
wp_cache_multi_get()? wp_multi_cache_get()? wp_cache_get_multiple()? wp_cache_getS()?

@boonebgorges
3 years ago

#19 @boonebgorges
3 years ago

Thanks, @dd32. There is lots of useful discussion in #31245, but that ticket is covering two separate issues: (a) the implementation of get_multi(), and (b) whether and how get_multi() can help to solve the alloptions problem. Because we're developing good use cases for get_multi() outside of alloptions, I would hate to see the Cache API improvement held up because we can't come to a consensus about alloptions.

I agree that going with a different function name probably makes sense, given the compatibility concerns. wp_cache_get_multiple() seems best to me.

In https://core.trac.wordpress.org/ticket/31245#comment:3, @rmccue asks:

I'm not entirely sure on the _wp_cache_compat_get_multi naming/usage pattern here, but doing it in a compatibility file requires us to either define the function inside wp_start_object_cache or in a new file after wp_start_object_cache but before default-filters.php is loaded. Thoughts?

This compatibility pattern seems bad to me because it means that we need to a function_exists() check every time we want to use the new function. I suggest loading a compatibility file *inside of* wp_start_object_cache(). We do this already, when object-cache.php is not found. 20875.4.diff does this; it also switches to wp_cache_get_multiple(), uses @rmccue's variable names instead of my confusing ones, and simplifies by setting values not found in the cache to be set to false (rather than leaving them out of the array altogether), which makes the code simpler and is also closer to expected behavior, IMO.

#20 @spacedmonkey
3 years ago

Another really good use for get multi would be in _get_non_cached_ids function. Currently this loops around all the ids it gets out of cache. For performance it would be much better to do this in one, if possible.

I like the idea of calling the function wp_cache_get_multiple. It stops issues with existing object-caching drop-ins.

#21 follow-up: @rmccue
3 years ago

I'd like to get feedback from @tollmanz and @tellyworth on this one, as it affects a lot of caches and affects WP.com too. We may need to introduce this without using it for alloptions in #31245 if performance of alloptions is going to be a large concern.

Definitely agreed that alloptions/multi-get shouldn't block each other. alloptions is a very specific use case for multi-get that doesn't necessarily fit the typical use case for it; for most uses, an iterated single get (i.e. the fallback) isn't hugely different.

#22 @chriscct7
3 years ago

  • Owner set to boonebgorges
  • Status changed from reopened to assigned

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


3 years ago

#24 in reply to: ↑ 21 @boonebgorges
3 years ago

Replying to rmccue:

I'd like to get feedback from @tollmanz and @tellyworth on this one, as it affects a lot of caches and affects WP.com too. We may need to introduce this without using it for alloptions in #31245 if performance of alloptions is going to be a large concern.

Definitely agreed that alloptions/multi-get shouldn't block each other. alloptions is a very specific use case for multi-get that doesn't necessarily fit the typical use case for it; for most uses, an iterated single get (i.e. the fallback) isn't hugely different.

Agree 100%, especially about the last part. Functions like _get_non_cached_ids() already do a bunch of separate _get() calls. Switching to use getMulti() when available will do no harm at all in cases when the object cache doesn't implement it.

I think the main outstanding questions here have to do with function naming and syntax. On this point, @tollmanz's thoughts would be especially helpful, since he originally suggested (and implemented in his own cache backends) the more sophisticated syntax.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 years ago

#26 @ocean90
3 years ago

  • Milestone changed from 4.6 to Future Release

This seems to require some feedback by authors of the leading cache drop-ins. Punting.

#27 @jipmoors
3 years ago

Wanted to throw in another variation for the naming: wp_cache_get_bulk

#28 @boonebgorges
3 years ago

See #37762. The cache_results parameter of WP_Query was introduced to skirt problems with bulk cache set/get, but the parameter is mostly broken. Once we have these bulk cache functions, we may want to remove (or otherwise fix) cache_results.

This ticket was mentioned in Slack in #core by boone. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

#31 follow-up: @johnjamesjacoby
3 years ago

This ticket is relevant to my interests.

I followed Memcached's approach in WP Spider Cache, and it feels the most WordPress-like in it's signature to me.

#32 in reply to: ↑ 31 @boonebgorges
3 years ago

Replying to johnjamesjacoby:

This ticket is relevant to my interests.

I followed Memcached's approach in WP Spider Cache, and it feels the most WordPress-like in it's signature to me.

IMO, it feels WordPress-like in all the worst ways (doing one magic thing with a string $groups, and a different magic thing with an array), but as noted in 16, my feelings aren't particularly strong one direction or another.

To move this forward, someone needs to make the call about syntax and function naming. Authors of existing drop-ins (excepting @johnjamesjacoby) don't seem eager to wade into this discussion, so I think we need an executive decision. I nominate @dd32 to make that decision, unless he wants to delegate his authority to me ;)

#33 @dd32
3 years ago

To move this forward, someone needs to make the call about syntax and function naming. Authors of existing drop-ins (excepting @johnjamesjacoby) don't seem eager to wade into this discussion, so I think we need an executive decision. I nominate dd32 to make that decision,

I think we just need to choose the most sane function signature/return values and go with it - but with the caveat that we can't use an existing name if a common dropin has a conflicting method signature.

20875.4.diff seems sane to me, to recap it:

  • wp_cache_get_multiple( $groups ) which doesn't exist in the wild that I can see - it's within the wp_cache_ namespace, so if something private has used it, good luck to them.
  • Signature of $groups as array( group1 => [ key1, key2 ], group2 => [ key3 ] ); IMHO looks sane and balances the complexity of fetching from multiple groups while at the same time catering to it.
  • Return signature of array( group1 => [ key1 => value1, key2 => value2 ], group2 => [ key3 => value3 ] ) matches the input signature, which makes usability far more consistent.

Looking at real world use-cases:

  • $posts = wp_cache_get_multiple( [ 'posts' => [ 1, 2, 3 ] ] )['posts'];
  • $cache_data = wp_cache_get_multiple( [ 'posts' => [ 1 ], 'postmeta' => [ 1 ] ); $post = $cache_data['posts'][1]; $post_meta = $cache_data['postmeta'][1];
  • $option_names = [ 'option_1', 'option_2' ]; $options = wp_cache_get_multiple( [ 'options' => $option_names ] );

Moving forward on this ticket:

  • I'd like to see the authors of the various object caches to be consulted, with 20875.4.diff as the suggested core patch (I don't see a compiled list of object caches and their authors here)
  • A quick yay-nay from each and maybe examples of how the cache would implement it, to ensure that there's no unexpected surprises for authors. (Such as the APC one in 20875-implementation-apc.php). This could be done by others as patches or PRs to projects too (Best to ask no cache to implement the signature until core has a commit though!)
  • A few examples of cases in WordPress where it can be utilised immediately (through the compat wrapper) which shows it's beneficial and actually offers improvements when implemented. (Such as done in 20875-example-usage.diff

From there, I think we're surprisingly in a good position to move forward here.

#34 @spacedmonkey
3 years ago

Good to see progress on this ticket. Regarding getting feedback for drop-in authors, we can reach out to some of them on slack and see what they say. I know that many of the most interested parties have already set this ticket to watched anyway. But I am worried that if this is left hanging anymore it will get held up. Maybe we could set a hard deadline for feedback of say, 4 weeks. If we don't hear back from them in this time, just go a head. We can keep backwards compatibility by using function_exists in the right places.

A reviewed the core for good places to implement a multi get and found these functions.

update_meta_cache
update_object_term_cache
_get_non_cached_ids

All of these a simple for each loops from one group, so shouldn't be too painful to implement.

Last edited 3 years ago by spacedmonkey (previous) (diff)

#35 @spacedmonkey
22 months ago

20875.5.diff is an update to the patch.

A small change to the compat fix.

I would really like to see this in core. It has been ages, the object-cache drop-in plugin developers, have not feedback, let's move forward wp_get_cache_multiple, as it is not a breaking change.

#36 @spacedmonkey
22 months ago

  • Focuses performance added

See attached to this ticket, three example implementation.

When this gones in, there should become there own tickets.

Note: See TracTickets for help on using tickets.