-
Notifications
You must be signed in to change notification settings - Fork 34
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
Performance - Reduce unnecessary database reads in get_related_orders() when fetching multiple order types #790
base: trunk
Are you sure you want to change the base?
Conversation
…get_related_order-types
} | ||
|
||
return $related_orders; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗒️ Note
Just noting that this function is just a stub in the abstract class. Because I'm adding a new function and using it via WCS_Related_Order_Store
, if anyone has a custom related order store, without this stub it would cause a fatal error.
This function is just a simple implementation of returning a list of related orders from an array of order types.
This restores the previous format of this returned value. Prior to my earlier commit the get_related_order_ids() call returned results in the [ 0 => $id ] format. This commit restores that.
@mattallan after our discussion in Slack, I've just updated this PR (efda0d8) to remove the When working on this PR I noticed that parent orders returned in the format The old version of this function had inconsistent returned formats: wcs_get_subscription( 143 )->get_related_order_ids( 'parent' )
[ [142] => 142 ]
-------
wcs_get_subscription( 143 )->get_related_order_ids( 'renewal' | 'any' )
[
[0] => 158
[1] => 146
[2] => 145
[3] => 144
] This is because when querying for 'any' or a specific type, the In any case, this function is protected so we don't have to worry too much about the inconsistent return format. I chose to return in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @james-allan for working on this!
The $is_batch_processing
approach kind of felt a bit messy to me because it requires you to set and unset $batch_processing_related_orders
$subscription_meta_cache
variables. That said, I attempted to explore another approach like updating get_related_order_ids()
to allow an array of relation types to be passed, but that got even messier!!
Coming back and reviewing your changes makes a lot of sense why you decided to go with this approach 😅
Testing performed:
- Load WooCommerce Subscriptions list table and confirm related orders column is populating
- Note the reduction in queries performed on the page
- Deleted related orders cache and refreshed the WooCommerce > Subscriptions page to regenerate related orders.
- Processed a renewal order & deleted a renewal order to confirm the changes
get_related_order_metadata()
hasn't impacted regular relation adding/deleting.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a minor finding I found while testing/reviewing
foreach ( $relation_types as $relation_type ) { | ||
$related_order_ids = array_merge( $related_order_ids, WCS_Related_Order_Store::instance()->get_related_order_ids( $this, $relation_type ) ); | ||
if ( $parent_id ) { | ||
$related_order_ids['parent'] = [ $parent_id ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another minor thing I noticed but this function doesn't return an empty 'parent' array if there's no parent order, but it does for empty 'renewal', 'switch' etc.
For example, if you create an New Subscription via WP admin with no parent or renewals and call:
wcs_get_subscription( 123 )->get_related_orders( 'all', [ 'parent', 'renewal', 'switch' ] );
The get_related_order_ids()
returns an array like:
Array
(
[renewal] => Array
(
)
[switch] => Array
(
)
)
Not sure if we should add an empty array for parent if the calling function is requesting it?
Part of https://github.com/woocommerce/woocommerce-subscriptions/issues/4795
Description
This PR reduces the number of duplicate queries running on the WooCommerce → Subscriptions list table, decreasing them from 137 to 117 (-20). It focuses on the
$subscription->get_related_orders()
flow and improvements to theWCS_Related_Order_Store
. It therefore has broader impacts to the overall performance - more than just the list table.Previously, calling
$subscription->get_related_orders()
with multiple order types would lead to multipleWCS_Related_Order_Store::instance()->get_related_order_ids()
calls.Whenever you fetch a subscription's related order cache, you do a direct read on the database.
So for example, this code snippet would lead to three full subscription metadata reads -- One for each order type (renewal, switch, resubscribe).
This PR reduces that to just one read.
To achieve that, this PR makes the following changes:
WCS_Related_Order_Store::get_related_order_ids_by_types()
function, designed to fetch multiple related order lists from the cache in a single read.WC_Subscription::get_related_order_ids()
so it can fetch multiple order types - not just one at a time.WC_Subscription::get_related_orders()
to make use of this new function.How to test this PR
get_related_order_metadata()
have in the list table.
Product impact