-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Move the notoptions lookup before the options cache lookup #8030
base: trunk
Are you sure you want to change the base?
Move the notoptions lookup before the options cache lookup #8030
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jonnynews. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
if ( isset( $alloptions[ $option ] ) ) { | ||
$value = $alloptions[ $option ]; | ||
} else { | ||
// Check for non-existent options first to avoid unnecessary object cache lookups and DB hits. | ||
$notoptions = wp_cache_get( 'notoptions', 'options' ); |
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.
Would it make sense to use wp_cache_get_multiple here?
$options = wp_cache_get_multiple( array( 'notoptions', $option ), 'options' );
$notoptions = $options['notoptions'] ?? array();
$value = $options[$option] ?? 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.
I don't think so, because the goal is to avoid the call to the object cache for the option at all if it doesn't exist. If we could get the alloptions and notoptions cache values in one request, that would be an improvement, but not a very meaningful one.
One thing to consider about the call to wp_cache_get( 'notoptions', 'options' );
is that the first time it is called, it will do check the external object cache if available, but then additional calls in the same request will already have that value in memory.
// Prevent non-existent `notoptions` key from triggering multiple key lookups. | ||
if ( ! is_array( $notoptions ) ) { | ||
$notoptions = array(); | ||
wp_cache_set( 'notoptions', $notoptions, 'options' ); |
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.
It appears that this line (initializing notoptions
in the cache to an empty array) is removed entirely in this PR? Is that intentional? Is this line no longer needed?
I wrote some rough tests for this and while it fixes the problem for a non-existent option, it appears to introduce the same problem to an option that exists but is not autoloaded. These tests pass with this patch but as you can see, the cache hits for The test I was running follows. It will almost certainly break on some of the third party tests so is not ready for commit. /**
* Test that get_option() does not hit the external cache multiple times for the same option.
*
* @ticket 62692
*
* @dataProvider data_get_option_does_not_hit_the_external_cache_multiple_times_for_the_same_option
*
* @param int $expected_connections Expected number of connections to the memcached server.
* @param bool $option_exists Whether the option should be set. Default true.
* @param string $autoload Whether the option should be auto loaded. Default true.
*/
public function test_get_option_does_not_hit_the_external_cache_multiple_times_for_the_same_option( $expected_connections, $option_exists = true, $autoload = true ) {
if ( ! wp_using_ext_object_cache() ) {
$this->markTestSkipped( 'This test requires an external object cache.' );
}
if ( ! function_exists( 'wp_cache_get_stats' ) ) {
$this->markTestSkipped( 'This test requires the Memcached PECL extension.' );
}
if ( $option_exists ) {
add_option( 'ticket-62692', 'value', '', $autoload );
}
wp_cache_delete_multiple( array( 'ticket-62692', 'notoptions', 'alloptions' ), 'options' );
$stats = wp_cache_get_stats();
$connections_start = array_shift( $stats )['cmd_get'];
$call_getter = 10;
while ( $call_getter-- ) {
get_option( 'ticket-62692' );
}
$stats = wp_cache_get_stats();
$connections_end = array_shift( $stats )['cmd_get'];
$this->assertSame( $expected_connections, $connections_end - $connections_start );
}
/**
* Data provider.
*
* @return array[]
*/
public function data_get_option_does_not_hit_the_external_cache_multiple_times_for_the_same_option() {
return array(
'exists, autoload' => array( 1, true, true ), // 1 on trunk.
'exists, not autoloaded' => array( 12, true, false ), // 3 on trunk.
'does not exist' => array( 3, false ), // 12 on trunk.
);
} |
Trac ticket: https://core.trac.wordpress.org/ticket/62692
This is an alternate approach to #8004 which moves the check of 'notoptions' before checking the 'options' cache while leaving the 'alloptions' check first.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.