Skip to content

Adding OrderTableDatastore and Meta caching integration #46023

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

Merged
merged 74 commits into from
Dec 5, 2024

Conversation

prettyboymp
Copy link
Contributor

@prettyboymp prettyboymp commented Mar 27, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

Closes #45550.

Background:
The current OrderCaching works by attempting to cache the Order objects. However, the current implementation of WC_Data::__sleep()/__wakeup() only allow the ID to be serialized. The reason for this was to avoid serializing the datastore with a data object, but also prevents issues where complex hierarchies of objects can end up getting cached with the order object. The end result means that any unserialized WC_Data object has to be reinitialized completely, requiring DB queries to retrieve the data for re-hydration.

This PR introduces the following that are disabled by default:

  1. Caching of raw order data is added to the HPOS DataStore layer reducing the queries needed to initialize order objects.
  2. OrderCache (which is used to cache instantiated WC_Order class objects) now uses an order_object cache group that is set as non-persistent. We don't want to save the order objects in persistent storage and the time spent Instantiating a WC_Order object is small, but can add up. So non-persistent cache is used to avoid re-instantiating the same object over and over again during a single request.
  3. Introduces a new WOOCOMMERCE_ENABLE_ALPHA_FEATURE_TESTING constant that can be used during initial alpha integration of new features/enhancements. When defined, the option to enable order data caching is available through the advanced features settings.

Benefits:

  1. An entity doesn't need to care where it is stored (e.g. which datastore) or about it's own caching. So we can remove some of the work-arounds added to deal with cache when syncing.
  2. Caching a set of Orders continues to only cache the set of IDs instead of the entire object. This ensures that the orders restored from the set are the latest versions of those orders as provided in the database.

Alternative Considered:
I looked at improving the serialization handling of WC_Data and it's child classes so that we could serialize all the relevant data with the entities without connected services, like the data stores. There were a couple issues with this:

First, PHP limitations meant that the serialization/deserialization would need to be pretty complex: PHP doesn't have a simple way to use the default serialization and just exclude certain properties. To properly restore an object, each class in the hierarchy would need to potentially deal with any private properties it defines. With extended classes from other plugins, those child classes would need to be properly modified for this to work or compatibility would break.

Second, there is already a bit of juggling going on around caching orders because the data stores have accessible public API's for making updates. Shifting the caching to them allows them to directly control the cache and update it when needed.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Unit tests have been updated to validate proper cache invalidation.

The following can be used to test the functionality of the OrdersTableDataStore caching. :

  1. Create a file named PR46023Tests.php somewhere within the plugins/woocommerce/tests/php/src diretory.
<?php

/**
 * Test for https://github.com/woocommerce/woocommerce/pull/46023 to verify that the caching actually makes
 * performance improvements over the current cache handling by caching the order data not currently preserved
 * with the WC_Data objects.  It works by forcing cached objects/arrays to be serialized like they would be with a 
 * persistent cache solution. 
 */
class PR46023Tests extends WC_Unit_Test_Case {

	use \Automattic\WooCommerce\RestApi\UnitTests\HPOSToggleTrait;

	public function setUp(): void {
		parent::setUp();
		add_filter( 'wc_allow_changing_orders_storage_while_sync_is_pending', '__return_true' );
		$this->setup_cot();
		$GLOBALS['wp_object_cache'] = new Serializing_Cache_Proxy( $GLOBALS['wp_object_cache'] );
	}

	public function tearDown(): void {
		$this->clean_up_cot_setup();
		remove_filter( 'wc_allow_changing_orders_storage_while_sync_is_pending', '__return_true' );
		parent::tearDown();
	}

	public function test_order_cache_perfmance() {
		global $wpdb;
		$order_1 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_2 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_3 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_4 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_5 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();
		$order_6 = \Automattic\WooCommerce\RestApi\UnitTests\Helpers\OrderHelper::create_order();

		wc_get_orders( [
			$order_1->get_id(),
			$order_2->get_id(),
			$order_3->get_id(),
			$order_4->get_id(),
			$order_5->get_id(),
			$order_6->get_id(),
		] );

		$initial_queries = $wpdb->num_queries;
		wc_get_order( $order_1->get_id() );
		wc_get_order( $order_2->get_id() );
		wc_get_order( $order_3->get_id() );
		wc_get_order( $order_4->get_id() );
		wc_get_order( $order_5->get_id() );
		wc_get_order( $order_6->get_id() );

		$final_queries = $wpdb->num_queries;
		$this->assertEquals( 0, $final_queries - $initial_queries, 'Expected no queries to occur retrieving previously retrieved Orders' );
	}
}

/**
 * Proxy class used to mimic objects
 */
class Serializing_Cache_Proxy {
	public $original_cache_instance;

	public function __construct( $original_cache_instance ) {
		$this->original_cache_instance = $original_cache_instance;
	}

	public function __call( $function, $args ) {
		return $this->original_cache_instance->$function( ...$args );
	}

	public function get( $key, $group = 'default', $force = false, &$found = null ) {
		$data = $this->original_cache_instance->get( $key, $group, $found, $found );
		if ( is_object( $data ) || is_array( $data ) ) {
			return unserialize( serialize( $data ) );
		}

		return $data;
	}
}
  1. Run the test unit tests, filtering to the new test with --filter=PR46023Tests

Manual Testing

  1. Setup an environment with persistent object caching drop-in setup.
  2. Under wp-admin/admin.php?page=wc-settings&tab=advanced&section=features, verify that the HPOS Data Caching Feature is not shown.
  3. Add the following to the wp-config.php: define( 'WOOCOMMERCE_ENABLE_ALPHA_FEATURE_TESTING', true );
  4. Reload the advanced->features and verify that the option now shows.
    Screenshot 2024-08-15 at 2 50 55 PM
  5. Disable persistent cache by temporarily removing the drop-in.
  6. Reload the advanced->features and verify that the option is visible, but disabled noting that it is only suggested with external object caching.
    Screenshot 2024-08-15 at 2 52 38 PM
  7. Re-enable external object caching.
  8. Enable HPOS and the HPOS Data Caching.
  9. Place an order through the site
  10. Within the admin, validate that the data is as expected.
  11. Update the order within the admin, including any meta data.
  12. Verify that the data is updated as expected.
  13. Revising the order details as a customer and make sure the data reflects the latest data.

Additional Automated testing:

  1. Modify OrderUtil:: custom_orders_table_datastore_cache_enabled() to always return true;
  2. Run the woocommerce core testsuites.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 27, 2024
@prettyboymp prettyboymp marked this pull request as ready for review April 3, 2024 19:28
@prettyboymp prettyboymp requested review from a team and lsinger and removed request for a team April 3, 2024 21:36
Copy link
Contributor

github-actions bot commented Apr 3, 2024

Hi @naman03malhotra,

Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@prettyboymp prettyboymp removed the request for review from lsinger April 4, 2024 08:42
@prettyboymp
Copy link
Contributor Author

Pausing review on this until I can come up with a better testing process.

@prettyboymp prettyboymp requested a review from a team April 6, 2024 09:27
…actions into their own methods.

Breaking these out should simplify future refactoring to possibly move DB and Cache layer interactions into their own classes and let the datastore delegate to those.
@prettyboymp
Copy link
Contributor Author

  • I understand that we are yet tinkering here. Still, the complexity of the changes is high, and I suggest refactoring things a bit before rolling out the feature as alfa: the fast route would be to aggregate/extend the classes heavily affected by caching mechanisms (where possible) and place new bits in there.

Do you think we could do something about this?

@kalessil, I pushed some refactoring around the datastore methods that were both accessing cache and the db. I broke these out into db specific methods and cache specific methods and left the merging/management of the data in the original method. Think this simplifies those and better defines where the abstraction/separation should go for future iterations.

Because the current abstractions of the DB layer within the datastores uses inheritance rather than composition, the future iterations won't be easy to implement without further compatibility concerns and likely temporary additional complexity, which is why I'd like to address those later.

Copy link
Contributor

@kalessil kalessil left a comment

Choose a reason for hiding this comment

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

@prettyboymp: with the latest updates, it's much easier to grasp, thanks for the updates!

Also leaving some implementation-related hints, feel free to ignore them or let me know if I should turn geeky mode and do deeper implementation review.

// phpcs:enable

foreach ( $meta_rows as $meta_row ) {
if ( ! isset( $meta_data[ $meta_row->object_id ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we should probably array fill instead before the loop - that'd reduce cognitive load abit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this. After re-reviewing this, filling the array ahead of time for the $object_ids works out better programmatically here as well. This made me notice that, though unlikely, an order with no meta data would result in no value set for the object_id. This would both mean it would always be a cache miss and cause some warnings in OrdersTableDataStore::init_order_record().

Copy link
Contributor

@kalessil kalessil left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for updating the PR!

@prettyboymp, what'd be our next steps to push the caching forward?

* The use of datastore caching moves persistent data caching to the datastore. Order object caching then only
* acts as request level caching as the `order_objects` cache group is set as non-persistent.
*/
return 'order_objects';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is a valid concern or not, but adding it here just in case. It appears that this object_type value is used for more than just a cache key. In particular, it's also used for hook names. It seems like there could be backcompat issues here if we conditionally change this. But I'm not sure what the alternative would be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to update this docblock to remove the reference to hooks/options. The only built in CacheEngine implementation is WPCacheEngine.php which doesn't have any underlying hooks or option usage.

I think the compatibility concern is limited. It would require that an extension both overwrote the cache engine implementation in a way where it depended on the object type returned from the default cache implementation for some behavior but hard coded the previous default in hook usage, or vice versa.

@@ -1,17 +1,30 @@
<?php
/**
* OrdersTableDataStoreMeta class file.
*
* @phpcs:disable Universal.NamingConventions.NoReservedKeywordParameterNames.objectFound
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be re-enabled sometime after the offending code lines.

@naman03malhotra naman03malhotra removed their request for review December 5, 2024 15:49
@prettyboymp prettyboymp merged commit a4a055d into trunk Dec 5, 2024
27 checks passed
@prettyboymp prettyboymp deleted the fix/45550-cache-orders-at-datastore-layer branch December 5, 2024 18:54
@github-actions github-actions bot added this to the 9.6.0 milestone Dec 5, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Dec 5, 2024
@Stojdza Stojdza added status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Dec 6, 2024
PavloBorysenko pushed a commit to PavloBorysenko/woocommerce that referenced this pull request Jan 3, 2025
…46023)

Adding a feature for caching of raw order data at the OrderDataStore level.

Closes woocommerce#45550

With this feature enabled:
* The cache group used by OrderCache is moved to `order_objects` and made non-persistent
* The OrderTableDataStore and OrderTableDataStoreMeta now cache retrieved data.
* Cache for individual order data is purged automatically after a write operation to the order.

This is all feature gated behind the `woocommerce_hpos_datastore_caching_enabled` option and is currently under alpha testing - meaning that the option to enable this feature is only available if `WOOCOMMERCE_ENABLE_ALPHA_FEATURE_TESTING` is defined as true.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wc_get_order cache being ignored Current Orders Cache Query implementation results in more queries than when disabled.
5 participants