Support for wp_cache_get_multiple() #65

netsuso · 2020-08-24T21:33:28Z

Add support for the new wp_cache_get_multiple() method added in WordPress 5.5

Part of the code has been reused from legacy wp_cache_get_multi(), although the new implementation uses actual memcache multiget requests instead of individual gets in a loop. This means it could be used efficiently for code that requires obtaining many cached objects at once.

It returns an array with this format:

wp_cache_get_multiple( array( 'key_id1', 'key_id2' ), 'group' );
array(
   'key_id1' => 'value1',
   'key_id2' => 'value2',
);

Where key_id1 and key_id2 are exactly the same keys as requested, following the format that is returned by the reference wp_cache_get_multiple() in WordPress 5.5.

The legacy method wp_cache_get_multi() is kept for backwards compatibility, although it has changed the response format. So far it returned the full memcached key (including the salt and prefix), and now it just returns the group and key identifier:

wp_cache_get_multi( array( 'group1' => array( 'key_id1' ), 'group2' => array( 'key_id2' ) ) );
array (
   'group1:key_id1' => 'value1',
   'group2:key_id2' => 'value2',
);

The expectation of this breaking any code is low, as the original response format invited to just ignore the keys (as they didn't match the requested keys) and use the returned values.

Fixes #63

- Moved most code from wp_cache_get_multi() to the new method - Using true memcached multiget instead of single gets in a loop - Kept wp_cache_get_multi() as a wrapper of the new method. The return value is slightly different, as it's not a map of the full memcache keys, but keys are just '<group>:<id>'

netsuso · 2020-08-24T21:40:21Z

tests/test-object-cache.php

-			$this->object_cache->key( 'foo', 'default') => 'data',
-			$this->object_cache->key( 'foo', 'another-group') => 'data',
+			'default:foo' => 'data',
+			'another-group:foo' => 'data',


For the moment I've just done the basic adaptations to the previous tests to validate that everything works the same apart from the slight response format. I'll add more tests (directly to wp_cache_get_multiple()) later

mjangda · 2020-08-24T21:42:43Z

To be on the safe side, I would suggest that we don't change wp_cache_get_multi. Instead, I would leave it as-is and mark as deprecated (https://developer.wordpress.org/reference/functions/_deprecated_function/)

dd32 · 2020-08-25T02:17:11Z

My initial feeling was to leave get_multi() as-is and deprecate it, or have get_multiple() wrap the existing get_multi(), however, upon reading the diff I don't think it makes that much of a difference realistically.

A lot of the differences in the diff are due to indentation changes since get_mutliple() requiring one less loop depth (since it only deals with a single group), and that the get_multi() implementation evidently didn't use the getMulti memcache implementation at all anyway (so there's another loop depth removal since it doesn't loop through keys).

I think I'd support the direction this diff has gone in - get_multiple() is the correct method, and get_multi() just wrapping it for legacy purposes, but also gaining a bit of a performance boost thanks to the getMulti calls being implemented right.

My only real concern here, is ensuring that false values are accounted for correctly (and on subsequent reads that uses $this->return_cache) since that looks like an easy failure point in the existing and new code. Unit tests verifying those scenario's would be appreciated (I assume there might be some existing UTs for it, I haven't checked)

aidvu · 2020-08-27T12:01:47Z

object-cache.php

+
+		foreach ( $groups as $group => $ids ) {
+			$this->timer_start();
+			$result = $this->get_multiple( $ids, $group );


We have some cache-key parsing in group_ops_stats. Reusing get_multiple() here would do this:

1 get_multiple ["wp_:posts:3061","wp_:posts:3060"] (0.1 ms) 2 get_multi ["061","060"] (0.1 ms) 3 get_multi ["061","060"] (0.0 ms)

david-binda

Thanks for working on this @netsuso ! Really appreciate it 🙇

I like the complex approach you took for introducing the proper support of the new core's wp_cache_get_multiple function, by deprecating the WP_Object_Cache::get_multi in favour of a brand new get_multiple method. Thanks for being that thorough!

I have left some, mostly minor, comments inline and have tested the code. It works well, even with the false value @dd32 mentions.

I'm marking this as "Requests changes" since, there is some minor stuff (see the inline comments) which might be worth addressing, but otherwise the implementation feels solid.

Please, let me know if you have any questions! I'm happy to help to get this shipped.

david-binda · 2020-10-06T16:26:01Z

object-cache.php

+		$no_mc = in_array( $group, $this->no_mc_groups );
+
+		$uncached_keys = array();
+
 		$return = array();
 		$return_cache = array(
 			'value' => false,


I know that this is not new nor subject of this PR, but it's unfortunate that we are actually adding the keys value and found with values set to false to the in-memory cache every time this method, or the wp_cache_get_multi is being called. IMHO, the $return_cache should be set to an empty array, unless I'm missing something...

david-binda · 2020-10-06T16:27:34Z

object-cache.php

-				} else if ( in_array( $group, $this->no_mc_groups ) ) {
-					$return[ $key ] = false;
+			if ( isset( $this->cache[ $key ] ) && ( ! $force || $no_mc ) ) {
+				if ( is_object( $this->cache[ $key ][ 'value' ] ) ) {


minor, accordingly to the WordPress coding standards, there should be no space after the [ and before ] in case the key is a string. Thus, should read $this->cache[ $key ]['value']. This comment applies to other occurrences in the PR as well.

david-binda · 2020-10-06T16:29:24Z

object-cache.php

+			$this->group_ops_stats( 'get_multiple', $uncached_keys_list, $group, null, $elapsed );
+
+			foreach ( $uncached_keys as $id => $key ) {
+				$value = isset( $values[$key] ) ? $values[$key] : false;


minor: accordingly to the WordPress coding standards, there should be a space in between [ and ] in $values[$key].

david-binda · 2020-10-06T16:32:02Z

object-cache.php

+		$return = array();
+
+		foreach ( $groups as $group => $ids ) {
+			$this->timer_start();


This is kinda unfair for the get_multi method, that it records the whole time, including reading the values from in-memory cache, while the get_multiple only counts the memcache get (which is on par with other methods, so kudos for fixing that!).

Despite this properly preserves the current behaviour, I can imagine that it could be misleading. Do we even need a separate timing for the get_multi, especially as it's being actually written twice, per @aidvu 's comment? IMHO, leaving the stats only inside the get_multiple method might be enough.

If we would remove the get_multi from stats, then we should also update all stats related code referring to the get_multi string.

david-binda · 2020-10-06T16:36:02Z

object-cache.php

+			$elapsed = $this->timer_stop();
+
+			foreach ( $result as $id => $value ) {
+				$id_with_group = $group . ':' . $id;


I now that this has been flagged in the PR's description, but it feels like keeping the function fully backward compatible should be as easy as replacing the $id_with_group = $group . ':' . $id; by $id_with_group = $this->key( $id, $group ); I wonder, what's the reasoning behind changing the key?

david-binda · 2020-10-06T16:40:41Z

object-cache.php

 					];
 				}
+
+				continue;


the continue statement does not make much sense here. I know it's a residuum from the original code of the get_multi, but perhaps it's a good time to remove it? The comment applies to other occurrences as well.

david-binda · 2020-10-06T16:42:58Z

object-cache.php

 					];
 				}
+
+				continue;
+			} else if ( $no_mc ) {


minor: WordPress coding standards prefer elseif over else if

david-binda · 2020-10-07T09:40:50Z

object-cache.php

+	function get_multiple( $ids, $group = 'default', $force = false ) {
+		$mc =& $this->get_mc( $group );
+
+		$no_mc = in_array( $group, $this->no_mc_groups );


I like the approach of storing the result of the expression in a variable, as it makes the code more readable and also likely more performant 👍

sonarcloud · 2022-06-26T07:06:48Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

No Coverage information
0.0% Duplication

dd32 · 2022-07-08T02:05:27Z

Implemented via #88

netsuso requested review from mjangda and nickdaugherty August 24, 2020 21:33

mjangda requested review from aidvu and dd32 August 24, 2020 21:38

netsuso commented Aug 24, 2020

View reviewed changes

mjangda added the needs review label Aug 24, 2020

aidvu reviewed Aug 27, 2020

View reviewed changes

david-binda requested changes Oct 7, 2020

View reviewed changes

nickdaugherty removed their request for review August 9, 2021 14:47

dd32 closed this Jul 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for wp_cache_get_multiple() #65

Support for wp_cache_get_multiple() #65

netsuso commented Aug 24, 2020 •

edited by mjangda

Loading

netsuso Aug 24, 2020

mjangda commented Aug 24, 2020

dd32 commented Aug 25, 2020

aidvu Aug 27, 2020

david-binda left a comment

david-binda Oct 6, 2020

david-binda Oct 6, 2020

david-binda Oct 6, 2020

david-binda Oct 6, 2020

david-binda Oct 6, 2020

david-binda Oct 6, 2020

david-binda Oct 6, 2020

david-binda Oct 7, 2020

sonarcloud bot commented Jun 26, 2022

dd32 commented Jul 8, 2022

Support for wp_cache_get_multiple() #65

Support for wp_cache_get_multiple() #65

Conversation

netsuso commented Aug 24, 2020 • edited by mjangda Loading

Choose a reason for hiding this comment

mjangda commented Aug 24, 2020

dd32 commented Aug 25, 2020

Choose a reason for hiding this comment

david-binda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 26, 2022

dd32 commented Jul 8, 2022

netsuso commented Aug 24, 2020 •

edited by mjangda

Loading