Skip to content

Support for wp_cache_get_multiple() #65

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

Closed
wants to merge 1 commit into from
Closed

Support for wp_cache_get_multiple() #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
125 changes: 83 additions & 42 deletions object-cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,15 @@ function wp_cache_get( $key, $group = '', $force = false, &$found = null ) {

/**
* Retrieve multiple cache entries
*
* @param array $groups Array of arrays, of groups and keys to retrieve
* @return mixed
*/
function wp_cache_get_multiple( $keys, $group = '', $force = false ) {
global $wp_object_cache;

return $wp_object_cache->get_multiple( $keys, $group, $force );
}

/**
* Legacy method signature for wp_cache_get_multiple()
*/
function wp_cache_get_multi( $groups ) {
global $wp_object_cache;
Expand Down Expand Up @@ -354,68 +360,103 @@ function get( $id, $group = 'default', $force = false, &$found = null ) {
return $value;
}

function get_multi( $groups ) {
/*
format: $get['group-name'] = array( 'key1', 'key2' );
*/
function get_multiple( $ids, $group = 'default', $force = false ) {
$mc =& $this->get_mc( $group );

$no_mc = in_array( $group, $this->no_mc_groups );
Copy link

@david-binda david-binda Oct 7, 2020

Choose a reason for hiding this comment

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

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 👍


$uncached_keys = array();

$return = array();
$return_cache = array(
'value' => false,
Copy link

@david-binda david-binda Oct 6, 2020

Choose a reason for hiding this comment

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

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...

'found' => false,
);

foreach ( $groups as $group => $ids ) {
$mc =& $this->get_mc( $group );
$keys = array();
$this->timer_start();
foreach ( $ids as $id ) {
$key = $this->key( $id, $group );

foreach ( $ids as $id ) {
$key = $this->key( $id, $group );
$keys[] = $key;

if ( isset( $this->cache[ $key ] ) ) {
if ( is_object( $this->cache[ $key ][ 'value'] ) ) {
$return[ $key ] = clone $this->cache[ $key ][ 'value'];
$return_cache[ $key ] = [
'value' => clone $this->cache[ $key ][ 'value'],
'found' => $this->cache[ $key ][ 'found'],
];
} else {
$return[ $key ] = $this->cache[ $key ][ 'value'];
$return_cache[ $key ] = [
'value' => $this->cache[ $key ][ 'value' ],
'found' => $this->cache[ $key ][ 'found' ],
];
}

continue;
} 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' ] ) ) {
Copy link

@david-binda david-binda Oct 6, 2020

Choose a reason for hiding this comment

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

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.

$return[ $id ] = clone $this->cache[ $key ][ 'value' ];
$return_cache[ $key ] = [
'value' => false,
'found' => false,
'value' => clone $this->cache[ $key ][ 'value' ],
'found' => $this->cache[ $key ][ 'found' ],
];

continue;
} else {
$fresh_get = $mc->get( $key );
$return[ $key ] = $fresh_get;
$return[ $id ] = $this->cache[ $key ][ 'value' ];
$return_cache[ $key ] = [
'value' => $fresh_get,
'found' => false !== $fresh_get,
'value' => $this->cache[ $key ][ 'value' ],
'found' => $this->cache[ $key ][ 'found' ],
];
}

continue;
Copy link

@david-binda david-binda Oct 6, 2020

Choose a reason for hiding this comment

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

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.

} else if ( $no_mc ) {
Copy link

@david-binda david-binda Oct 6, 2020

Choose a reason for hiding this comment

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

minor: WordPress coding standards prefer elseif over else if

$return[ $id ] = false;
$return_cache[ $key ] = [
'value' => false,
'found' => false,
];

continue;
} else {
$uncached_keys[ $id ] = $key;
}
}

if ( $uncached_keys ) {
$this->timer_start();

$uncached_keys_list = array_values( $uncached_keys );
$values = $mc->get( $uncached_keys_list );

$elapsed = $this->timer_stop();
$this->group_ops_stats( 'get_multi', $keys, $group, null, $elapsed );
$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;
Copy link

@david-binda david-binda Oct 6, 2020

Choose a reason for hiding this comment

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

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


$return[ $id ] = $value;
$return_cache[ $key ] = [
'value' => $value,
'found' => false !== $value,
];
}
}

$this->cache = array_merge( $this->cache, $return_cache );

return $return;
}

/**
* Legacy implementation for get_multiple
*
* Kept here for backwards compatibility
*/
function get_multi( $groups ) {
/*
format: $get['group-name'] = array( 'key1', 'key2' );
*/
$return = array();

foreach ( $groups as $group => $ids ) {
$this->timer_start();
Copy link

@david-binda david-binda Oct 6, 2020

Choose a reason for hiding this comment

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

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.

$result = $this->get_multiple( $ids, $group );
Copy link
Contributor

@aidvu aidvu Aug 27, 2020

Choose a reason for hiding this comment

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

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)

$elapsed = $this->timer_stop();

foreach ( $result as $id => $value ) {
$id_with_group = $group . ':' . $id;
Copy link

@david-binda david-binda Oct 6, 2020

Choose a reason for hiding this comment

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

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?

$return[ $id_with_group ] = $value;
}

$this->group_ops_stats( 'get_multi', $ids, $group, null, $elapsed );
}

return $return;
}

function flush_prefix( $group ) {
if ( 'WP_Object_Cache' === $group || 'WP_Object_Cache_global' === $group ) {
// Never flush the flush numbers.
Expand Down
20 changes: 10 additions & 10 deletions tests/test-object-cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,8 +509,8 @@ public function test_get_multi_returns_array_of_values_from_memcache(): void {
);

$expected = [
$this->object_cache->key( 'foo', 'default') => 'data',
$this->object_cache->key( 'foo', 'another-group') => 'data',
'default:foo' => 'data',
'another-group:foo' => 'data',
Copy link
Author

@netsuso netsuso Aug 24, 2020

Choose a reason for hiding this comment

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

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

];

$this->assertEquals( $expected, $values );
Expand Down Expand Up @@ -581,9 +581,9 @@ public function test_get_multi_returns_data_from_local_cache(): void {
);

$expected = [
$this->object_cache->key( 'foo', 'default') => 'data-updated',
$this->object_cache->key( 'foo', 'another-group') => 'data',
$this->object_cache->key( 'foo', 'and-another-group') => 'data',
'default:foo' => 'data-updated',
'another-group:foo' => 'data',
'and-another-group:foo' => 'data',
];

$this->assertEquals( $expected, $values );
Expand Down Expand Up @@ -618,9 +618,9 @@ public function test_get_multi_returns_data_not_from_local_cache(): void {
);

$expected = [
$this->object_cache->key( 'foo', 'default') => 'data',
$this->object_cache->key( 'foo', 'another-group') => 'data',
$this->object_cache->key( 'foo', 'and-another-group') => 'data',
'default:foo' => 'data',
'another-group:foo' => 'data',
'and-another-group:foo' => 'data',
];

$this->assertEquals( $expected, $values );
Expand All @@ -633,7 +633,7 @@ public function test_get_multi_only_uses_local_cache_when_using_non_persistent_g
$this->object_cache->add( 'foo', 'data', $group );

$values = $this->object_cache->get_multi( [ $group => [ 'foo' ] ] );
$expected = [ $this->object_cache->key( 'foo', $group ) => 'data' ];
$expected = [ $group . ':foo' => 'data' ];
$this->assertEquals( $expected, $values );

$expected_cache = [
Expand All @@ -654,7 +654,7 @@ public function test_get_multi_returns_false_if_no_local_cache_when_using_non_pe
unset( $this->object_cache->cache[ $key ] );

$values = $this->object_cache->get_multi( [ $group => [ 'foo' ] ] );
$expected = [ $this->object_cache->key( 'foo', $group ) => false ];
$expected = [ $group . ':foo' => false ];

$this->assertEquals( $expected, $values );
$this->assertFalse( $this->object_cache->cache[ $key ][ 'value' ] );
Expand Down