Fix duplicate spec evaluation in `evaluate_specs()` (#51166)
* feat: Add memoization to EvaluateSuggestion class This commit adds memoization to the `EvaluateSuggestion` class in order to improve performance by caching the results of the `evaluate_specs` method. The memoization is implemented using an associative array called `$memo`, which stores the results of previous evaluations based on the input specifications and logger arguments. The memoization logic checks if the results for a given set of specifications and logger arguments already exist in the `$memo` array, and if so, returns the cached results instead of re-evaluating the specs. This helps to avoid redundant computations and improves the overall efficiency of the `evaluate_specs` method. * Add changelog * Only use specs as key * Move shipping test to correct folder * Fix tests * Fix tests * Fix lint * Address PR feedback
This commit is contained in:
parent
007bd21a35
commit
b728f53f29
|
@ -0,0 +1,4 @@
|
|||
Significance: patch
|
||||
Type: fix
|
||||
|
||||
Fix duplicate spec evaluation in evaluate_specs()
|
|
@ -13,6 +13,13 @@ use Automattic\WooCommerce\Admin\RemoteSpecs\RuleProcessors\RuleEvaluator;
|
|||
* Evaluates the spec and returns the evaluated suggestion.
|
||||
*/
|
||||
class EvaluateSuggestion {
|
||||
/**
|
||||
* Stores memoized results of evaluate_specs.
|
||||
*
|
||||
* @var array
|
||||
*/
|
||||
protected static $memo = array();
|
||||
|
||||
/**
|
||||
* Evaluates the spec and returns the suggestion.
|
||||
*
|
||||
|
@ -58,6 +65,12 @@ class EvaluateSuggestion {
|
|||
* @return array The visible suggestions and errors.
|
||||
*/
|
||||
public static function evaluate_specs( $specs, $logger_args = array() ) {
|
||||
$specs_key = self::get_memo_key( $specs );
|
||||
|
||||
if ( isset( self::$memo[ $specs_key ] ) ) {
|
||||
return self::$memo[ $specs_key ];
|
||||
}
|
||||
|
||||
$suggestions = array();
|
||||
$errors = array();
|
||||
|
||||
|
@ -72,9 +85,43 @@ class EvaluateSuggestion {
|
|||
}
|
||||
}
|
||||
|
||||
return array(
|
||||
$result = array(
|
||||
'suggestions' => $suggestions,
|
||||
'errors' => $errors,
|
||||
);
|
||||
|
||||
// Memoize results, with a fail safe to prevent unbounded memory growth.
|
||||
// This limit is unlikely to be reached under normal circumstances.
|
||||
if ( count( self::$memo ) > 50 ) {
|
||||
self::reset_memo();
|
||||
}
|
||||
self::$memo[ $specs_key ] = $result;
|
||||
|
||||
return $result;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resets the memoized results. Useful for testing.
|
||||
*/
|
||||
public static function reset_memo() {
|
||||
self::$memo = array();
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns a memoization key for the given specs.
|
||||
*
|
||||
* @param array $specs The specs to generate a key for.
|
||||
*
|
||||
* @return string The memoization key.
|
||||
*/
|
||||
private static function get_memo_key( $specs ) {
|
||||
$data = wp_json_encode( $specs );
|
||||
|
||||
if ( function_exists( 'hash' ) && in_array( 'xxh3', hash_algos(), true ) ) {
|
||||
// Use xxHash (xxh3) if available.
|
||||
return hash( 'xxh3', $data );
|
||||
}
|
||||
// Fall back to CRC32.
|
||||
return (string) crc32( $data );
|
||||
}
|
||||
}
|
||||
|
|
|
@ -323,6 +323,31 @@ class WC_Admin_Tests_PaymentGatewaySuggestions_EvaluateSuggestion extends WC_Uni
|
|||
remove_filter( 'woocommerce_admin_remote_specs_evaluator_should_log', '__return_true' );
|
||||
}
|
||||
|
||||
/**
|
||||
* Test that the memo is set correctly.
|
||||
*/
|
||||
public function test_memo_set_correctly() {
|
||||
$specs = array(
|
||||
array(
|
||||
'id' => 'test-gateway-1',
|
||||
'is_visible' => true,
|
||||
),
|
||||
array(
|
||||
'id' => 'test-gateway-2',
|
||||
'is_visible' => false,
|
||||
),
|
||||
);
|
||||
|
||||
$result = TestableEvaluateSuggestion::evaluate_specs( $specs );
|
||||
$memo = TestableEvaluateSuggestion::get_memo_for_tests();
|
||||
|
||||
$this->assertCount( 1, $memo );
|
||||
$memo_key = array_keys( $memo )[0];
|
||||
$this->assertEquals( $result, $memo[ $memo_key ] );
|
||||
$this->assertCount( 1, $result['suggestions'] );
|
||||
$this->assertEquals( 'test-gateway-1', $result['suggestions'][0]->id );
|
||||
}
|
||||
|
||||
/**
|
||||
* Overrides the WC logger.
|
||||
*
|
||||
|
@ -359,3 +384,19 @@ class WC_Admin_Tests_PaymentGatewaySuggestions_EvaluateSuggestion extends WC_Uni
|
|||
);
|
||||
}
|
||||
}
|
||||
|
||||
//phpcs:disable Generic.Files.OneObjectStructurePerFile.MultipleFound, Squiz.Classes.ClassFileName.NoMatch, Suin.Classes.PSR4.IncorrectClassName
|
||||
/**
|
||||
* TestableEvaluateSuggestion class.
|
||||
*/
|
||||
class TestableEvaluateSuggestion extends EvaluateSuggestion {
|
||||
/**
|
||||
* Get the memo for testing.
|
||||
*
|
||||
* @return array
|
||||
*/
|
||||
public static function get_memo_for_tests() {
|
||||
return self::$memo;
|
||||
}
|
||||
}
|
||||
//phpcs:enable Generic.Files.OneObjectStructurePerFile.MultipleFound, Squiz.Classes.ClassFileName.NoMatch, Suin.Classes.PSR4.IncorrectClassName
|
||||
|
|
|
@ -25,7 +25,7 @@ class WC_Admin_Tests_PaymentGatewaySuggestions_Init extends WC_Unit_Test_Case {
|
|||
delete_option( 'woocommerce_show_marketplace_suggestions' );
|
||||
add_filter(
|
||||
'transient_woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs',
|
||||
function( $value ) {
|
||||
function ( $value ) {
|
||||
if ( $value ) {
|
||||
return $value;
|
||||
}
|
||||
|
@ -37,6 +37,8 @@ class WC_Admin_Tests_PaymentGatewaySuggestions_Init extends WC_Unit_Test_Case {
|
|||
);
|
||||
}
|
||||
);
|
||||
|
||||
EvaluateSuggestion::reset_memo();
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -57,7 +59,7 @@ class WC_Admin_Tests_PaymentGatewaySuggestions_Init extends WC_Unit_Test_Case {
|
|||
remove_all_filters( 'transient_woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs' );
|
||||
add_filter(
|
||||
DataSourcePoller::FILTER_NAME,
|
||||
function() {
|
||||
function () {
|
||||
return array();
|
||||
}
|
||||
);
|
||||
|
@ -242,7 +244,7 @@ class WC_Admin_Tests_PaymentGatewaySuggestions_Init extends WC_Unit_Test_Case {
|
|||
|
||||
add_filter(
|
||||
'locale',
|
||||
function( $_locale ) {
|
||||
function () {
|
||||
return 'zh_TW';
|
||||
}
|
||||
);
|
||||
|
@ -364,5 +366,4 @@ class WC_Admin_Tests_PaymentGatewaySuggestions_Init extends WC_Unit_Test_Case {
|
|||
// Clean up.
|
||||
delete_option( PaymentGatewaySuggestions::RECOMMENDED_PAYMENT_PLUGINS_DISMISS_OPTION );
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -1,7 +1,7 @@
|
|||
<?php
|
||||
declare( strict_types = 1 );
|
||||
|
||||
namespace Automattic\WooCommerce\Tests\Admin\ShippingPartnerSuggestions;
|
||||
namespace Automattic\WooCommerce\Tests\Admin\Features\ShippingPartnerSuggestions;
|
||||
|
||||
use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\EvaluateSuggestion;
|
||||
use Automattic\WooCommerce\Admin\Features\ShippingPartnerSuggestions\DefaultShippingPartners;
|
||||
|
@ -31,6 +31,8 @@ class DefaultShippingPartnersTest extends WC_Unit_Test_Case {
|
|||
update_option( 'woocommerce_store_address', 'foo' );
|
||||
|
||||
update_option( 'active_plugins', array( 'foo/foo.php' ) );
|
||||
|
||||
EvaluateSuggestion::reset_memo();
|
||||
}
|
||||
|
||||
/**
|
|
@ -7,6 +7,7 @@ use Automattic\WooCommerce\Admin\Features\OnboardingTasks\Tasks\Shipping;
|
|||
use Automattic\WooCommerce\Admin\Features\ShippingPartnerSuggestions\DefaultShippingPartners;
|
||||
use Automattic\WooCommerce\Admin\Features\ShippingPartnerSuggestions\ShippingPartnerSuggestions;
|
||||
use Automattic\WooCommerce\Admin\Features\ShippingPartnerSuggestions\ShippingPartnerSuggestionsDataSourcePoller;
|
||||
use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\EvaluateSuggestion;
|
||||
use WC_Unit_Test_Case;
|
||||
|
||||
/**
|
||||
|
@ -37,7 +38,7 @@ class ShippingPartnerSuggestionsTest extends WC_Unit_Test_Case {
|
|||
delete_option( 'woocommerce_show_marketplace_suggestions' );
|
||||
add_filter(
|
||||
'transient_woocommerce_admin_' . ShippingPartnerSuggestionsDataSourcePoller::ID . '_specs',
|
||||
function( $value ) {
|
||||
function ( $value ) {
|
||||
if ( $value ) {
|
||||
return $value;
|
||||
}
|
||||
|
@ -70,6 +71,8 @@ class ShippingPartnerSuggestionsTest extends WC_Unit_Test_Case {
|
|||
// Have a mock logger used by the suggestions rule evaluator.
|
||||
$this->mock_logger = $this->getMockBuilder( 'WC_Logger_Interface' )->getMock();
|
||||
add_filter( 'woocommerce_logging_class', array( $this, 'override_wc_logger' ) );
|
||||
|
||||
EvaluateSuggestion::reset_memo();
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -91,7 +94,7 @@ class ShippingPartnerSuggestionsTest extends WC_Unit_Test_Case {
|
|||
remove_all_filters( 'transient_woocommerce_admin_' . ShippingPartnerSuggestionsDataSourcePoller::ID . '_specs' );
|
||||
add_filter(
|
||||
DataSourcePoller::FILTER_NAME,
|
||||
function() {
|
||||
function () {
|
||||
return array();
|
||||
}
|
||||
);
|
||||
|
|
|
@ -29,6 +29,8 @@ class DefaultPromotionsTest extends WC_Unit_Test_Case {
|
|||
update_option( 'woocommerce_store_address', 'foo' );
|
||||
|
||||
update_option( 'active_plugins', array( 'foo/foo.php' ) );
|
||||
|
||||
EvaluateSuggestion::reset_memo();
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -26,7 +26,7 @@ class InitTest extends WC_Unit_Test_Case {
|
|||
delete_option( 'woocommerce_show_marketplace_suggestions' );
|
||||
add_filter(
|
||||
'transient_woocommerce_admin_' . WCPayPromotionDataSourcePoller::ID . '_specs',
|
||||
function( $value ) {
|
||||
function ( $value ) {
|
||||
if ( $value ) {
|
||||
return $value;
|
||||
}
|
||||
|
@ -38,6 +38,8 @@ class InitTest extends WC_Unit_Test_Case {
|
|||
);
|
||||
}
|
||||
);
|
||||
|
||||
EvaluateSuggestion::reset_memo();
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -59,7 +61,7 @@ class InitTest extends WC_Unit_Test_Case {
|
|||
remove_all_filters( 'transient_woocommerce_admin_' . WCPayPromotionDataSourcePoller::ID . '_specs' );
|
||||
add_filter(
|
||||
DataSourcePoller::FILTER_NAME,
|
||||
function() {
|
||||
function () {
|
||||
return array();
|
||||
}
|
||||
);
|
||||
|
|
Loading…
Reference in New Issue