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

Move the notoptions lookup before the options cache lookup #8030

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
+87 −25
Open

Move the notoptions lookup before the options cache lookup #8030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56f63ff
Move the notoptions lookup before the options cache lookup
joemcgill Dec 20, 2024
e3c900e
Cache empty notoptions
joemcgill Dec 23, 2024
c3c6b56
Add additional unit test
joemcgill Dec 23, 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
59 changes: 34 additions & 25 deletions src/wp-includes/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,37 +162,46 @@ function get_option( $option, $default_value = false ) {

if ( ! wp_installing() ) {
$alloptions = wp_load_alloptions();

/*
* When getting an option value, we check in the following order for performance:
*
* 1. Check the 'alloptions' cache first to prioritize existing loaded options.
* 2. Check the 'notoptions' cache before a cache lookup or DB hit.
* 3. Check the 'options' cache prior to a DB hit.
* 4. Check the DB for the option and cache it in either the 'options' or 'notoptions' cache.
*/
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' );
Copy link

@jonnynews jonnynews Dec 20, 2024 •
edited
Loading

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;

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


if ( ! is_array( $notoptions ) ) {
$notoptions = array();
wp_cache_set( 'notoptions', $notoptions, 'options' );
}

if ( isset( $notoptions[ $option ] ) ) {
/**
* Filters the default value for an option.
*
* The dynamic portion of the hook name, `$option`, refers to the option name.
*
* @since 3.4.0
* @since 4.4.0 The `$option` parameter was added.
* @since 4.7.0 The `$passed_default` parameter was added to distinguish between a `false` value and the default parameter value.
*
* @param mixed $default_value The default value to return if the option does not exist
* in the database.
* @param string $option Option name.
* @param bool $passed_default Was `get_option()` passed a default value?
*/
return apply_filters( "default_option_{$option}", $default_value, $option, $passed_default );
}

$value = wp_cache_get( $option, 'options' );

if ( false === $value ) {
// Prevent non-existent options from triggering multiple queries.
$notoptions = wp_cache_get( 'notoptions', 'options' );

// Prevent non-existent `notoptions` key from triggering multiple key lookups.
if ( ! is_array( $notoptions ) ) {
$notoptions = array();
wp_cache_set( 'notoptions', $notoptions, 'options' );
Copy link

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot! I was thinking that there was no real purpose in caching an empty value here. However, as @peterwilsoncc's tests demonstrate, without that line each additional time we check for 'notoptions' a call will be made to the external object cache rather than returning the empty array from memory.

I've added that back in e3c900e and it looks like that fixes the problem:

  • exists, autoload: 1
  • exists, not autoloaded: 3
  • does not exist: 3

} elseif ( isset( $notoptions[ $option ] ) ) {
/**
* Filters the default value for an option.
*
* The dynamic portion of the hook name, `$option`, refers to the option name.
*
* @since 3.4.0
* @since 4.4.0 The `$option` parameter was added.
* @since 4.7.0 The `$passed_default` parameter was added to distinguish between a `false` value and the default parameter value.
*
* @param mixed $default_value The default value to return if the option does not exist
* in the database.
* @param string $option Option name.
* @param bool $passed_default Was `get_option()` passed a default value?
*/
return apply_filters( "default_option_{$option}", $default_value, $option, $passed_default );
}

$row = $wpdb->get_row( $wpdb->prepare( "SELECT option_value FROM $wpdb->options WHERE option_name = %s LIMIT 1", $option ) );

Expand Down
53 changes: 53 additions & 0 deletions tests/phpunit/tests/option/option.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,4 +548,57 @@ public function test_add_option_clears_the_notoptions_cache() {
$updated_notoptions = wp_cache_get( 'notoptions', 'options' );
$this->assertArrayNotHasKey( $option_name, $updated_notoptions, 'The "foobar" option should not be in the notoptions cache after adding it.' );
}

/**
* 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 ),
'exists, not autoloaded' => array( 3, true, false ),
'does not exist' => array( 3, false ),
);
}
}
Loading