Move the notoptions lookup before the options cache lookup

[joemcgill]

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.

[github-actions]

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 props-bot label.

Unlinked Accounts

The 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:

Props joemcgill, siliconforks, peterwilsoncc.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The 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

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[jonnynews]

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;

[joemcgill]

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.

[siliconforks]

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?

[joemcgill]

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

[peterwilsoncc]

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 exists, not autoloaded goes up from three to twelve. The numbers for trunk are in comments in the data provider.

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.
	);
}

[joemcgill]

As @siliconforks noticed, I had failed to cache the empty 'notoptions' array, which was causing extra hits to external cache when the option existed and was not autoloaded, rather than pulling the 'notoptions' value from memory. This has been addressed in e3c900e.

[joemcgill]

@peterwilsoncc I went ahead and committed your tests to this PR but noticed you mention this in Trac:

...they need work so as not to break host tests using a different persistent cache implementation

Can you share more details about what needs to be updated?

[peterwilsoncc]

I ran out of time to test yesterday but I think the test will need to ensure cmd_get exists as it’s only reported on memcached. Other caches use a different structure for the stats, Redis reports it as ‘get’. All this is from memory while checking my email over lunch.