From 56f63ff7ad1e1cf0bdd01e8bf32b7169ffa0eb7e Mon Sep 17 00:00:00 2001 From: Joe McGill Date: Fri, 20 Dec 2024 10:51:18 -0600 Subject: [PATCH 1/5] Move the notoptions lookup before the options cache lookup --- src/wp-includes/option.php | 58 ++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index c84f1660f931b..638c77a3b33d3 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -162,37 +162,45 @@ 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' ); + + if ( ! is_array( $notoptions ) ) { + $notoptions = array(); + } + + 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' ); - } 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 ) ); From e3c900ed807c6cc92a848f31a0e4d4709bf97885 Mon Sep 17 00:00:00 2001 From: Joe McGill Date: Mon, 23 Dec 2024 08:08:21 -0600 Subject: [PATCH 2/5] Cache empty notoptions --- src/wp-includes/option.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wp-includes/option.php b/src/wp-includes/option.php index 638c77a3b33d3..4b26504b76356 100644 --- a/src/wp-includes/option.php +++ b/src/wp-includes/option.php @@ -178,6 +178,7 @@ function get_option( $option, $default_value = false ) { if ( ! is_array( $notoptions ) ) { $notoptions = array(); + wp_cache_set( 'notoptions', $notoptions, 'options' ); } if ( isset( $notoptions[ $option ] ) ) { From c3c6b56590e829e5de597649f804ac4db7b8cc7f Mon Sep 17 00:00:00 2001 From: Joe McGill Date: Mon, 23 Dec 2024 08:15:46 -0600 Subject: [PATCH 3/5] Add additional unit test --- tests/phpunit/tests/option/option.php | 53 +++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/phpunit/tests/option/option.php b/tests/phpunit/tests/option/option.php index 36a40d9a2f495..a8f92fdb02908 100644 --- a/tests/phpunit/tests/option/option.php +++ b/tests/phpunit/tests/option/option.php @@ -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 ), + ); + } } From edae41577d7bad750ddf57d235091dfdfdd4ea2b Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Thu, 2 Jan 2025 09:05:45 +1100 Subject: [PATCH 4/5] Ensure stats are of the correct shape. --- tests/phpunit/tests/option/option.php | 41 +++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/tests/phpunit/tests/option/option.php b/tests/phpunit/tests/option/option.php index a8f92fdb02908..a3d3684d74568 100644 --- a/tests/phpunit/tests/option/option.php +++ b/tests/phpunit/tests/option/option.php @@ -565,8 +565,8 @@ public function test_get_option_does_not_hit_the_external_cache_multiple_times_f $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 ( false === $this->helper_object_cache_stats_cmd_get() ) { + $this->markTestSkipped( 'This test requires access to the number of get requests to the external object cache.' ); } if ( $option_exists ) { @@ -575,16 +575,14 @@ public function test_get_option_does_not_hit_the_external_cache_multiple_times_f wp_cache_delete_multiple( array( 'ticket-62692', 'notoptions', 'alloptions' ), 'options' ); - $stats = wp_cache_get_stats(); - $connections_start = array_shift( $stats )['cmd_get']; + $connections_start = $this->helper_object_cache_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']; + $connections_end = $this->helper_object_cache_stats_cmd_get(); $this->assertSame( $expected_connections, $connections_end - $connections_start ); } @@ -601,4 +599,35 @@ public function data_get_option_does_not_hit_the_external_cache_multiple_times_f 'does not exist' => array( 3, false ), ); } + + /** + * Helper function to get the number of get commands from the external object cache. + * + * @return int|false Number of get command calls, false if unavailable. + */ + public function helper_object_cache_stats_cmd_get() { + if ( ! wp_using_ext_object_cache() || ! function_exists( 'wp_cache_get_stats' ) ) { + return false; + } + + $stats = wp_cache_get_stats(); + + // Check the shape of the stats. + if ( ! is_array( $stats ) ) { + return false; + } + + // Get the first server's stats. + $stats = array_shift( $stats ); + + if ( ! is_array( $stats ) ) { + return false; + } + + if ( ! array_key_exists( 'cmd_get', $stats ) ) { + return false; + } + + return $stats['cmd_get']; + } } From c8f38f42fdf80b421ad940fba448b1ea1d9974ac Mon Sep 17 00:00:00 2001 From: Joe McGill Date: Fri, 3 Jan 2025 14:24:21 -0600 Subject: [PATCH 5/5] Update unit tests --- tests/phpunit/tests/option/option.php | 29 ++++++--------------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/tests/phpunit/tests/option/option.php b/tests/phpunit/tests/option/option.php index a3d3684d74568..9a2f061aa369c 100644 --- a/tests/phpunit/tests/option/option.php +++ b/tests/phpunit/tests/option/option.php @@ -112,8 +112,8 @@ public function test_get_option_notoptions_cache() { wp_cache_set( 'notoptions', $notoptions, 'options' ); $before = get_num_queries(); - $value = get_option( 'invalid' ); - $after = get_num_queries(); + get_option( 'invalid' ); + $after = get_num_queries(); $this->assertSame( 0, $after - $before ); } @@ -127,8 +127,8 @@ public function test_get_option_notoptions_set_cache() { get_option( 'invalid' ); $before = get_num_queries(); - $value = get_option( 'invalid' ); - $after = get_num_queries(); + get_option( 'invalid' ); + $after = get_num_queries(); $notoptions = wp_cache_get( 'notoptions', 'options' ); @@ -137,25 +137,6 @@ public function test_get_option_notoptions_set_cache() { $this->assertArrayHasKey( 'invalid', $notoptions, 'The "invalid" option should be in the notoptions cache.' ); } - /** - * @ticket 58277 - * - * @covers ::get_option - */ - public function test_get_option_notoptions_do_not_load_cache() { - add_option( 'foo', 'bar', '', false ); - wp_cache_delete( 'notoptions', 'options' ); - - $before = get_num_queries(); - $value = get_option( 'foo' ); - $after = get_num_queries(); - - $notoptions = wp_cache_get( 'notoptions', 'options' ); - - $this->assertSame( 0, $after - $before, 'The options cache was not hit on the second call to `get_option()`.' ); - $this->assertFalse( $notoptions, 'The notoptions cache should not be set.' ); - } - /** * @covers ::get_option * @covers ::add_option @@ -554,6 +535,8 @@ public function test_add_option_clears_the_notoptions_cache() { * * @ticket 62692 * + * @covers ::get_option + * * @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.