Performance - Remove unnecessary get_time() calls to reduce redundant get_last_order() requests on admin Subscriptions list table #789
Conversation
|
|
||
| $date_type = wcs_normalise_date_type_key( $date_type, true ); | ||
| return $this->format_date_to_display( $timestamp_gmt, $date_type ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This change introduces a new function format_date_to_display(). Prior to this to display a date we'd call get_date_to_display( $date_type ); and it would need to fetch the timestamp.
In cases where we already have the timestamp, this would result in another get_time() call. This new function simply refactors out the formatting function of the original get_date_to_display() so if we have the timestamp, we can call format_date_to_display() instead.
| @@ -1316,7 +1327,7 @@ public function get_date_to_display( $date_type = 'next_payment' ) { | |||
| // translators: placeholder is human time diff (e.g. "3 weeks") | |||
| $date_to_display = sprintf( __( '%s ago', 'woocommerce-subscriptions' ), human_time_diff( current_time( 'timestamp', true ), $timestamp_gmt ) ); | |||
| } else { | |||
| $date_to_display = date_i18n( wc_date_format(), $this->get_time( $date_type, 'site' ) ); | |||
| $date_to_display = date_i18n( wc_date_format(), $timestamp_gmt + wc_timezone_offset() ); | |||
There was a problem hiding this comment.
This change removes one of the get_time() calls. Given we have the timestamp in GMT, and we want the time in the site's timezone, we offset the GMT time by the timezone offset instead.
There was a problem hiding this comment.
Thanks @james-allan for these improvements!
Old
116 duplicate queries
New
68 duplicate queries
Code changes look good. I tested my site in different +/- timezones mainly just to test the $timestamp_gmt + wc_timezone_offset() change introduced in this PR.
I also left a small whitespace commit and condition ordering for tiny performance, but apart from that this PR is good to go :)
| $tooltip_message .= __( 'This date should be treated as an estimate only. The payment gateway for this subscription controls when payments are processed.</br>', 'woocommerce-subscriptions' ); | ||
| $tooltip_classes .= ' wcs-offsite-renewal'; | ||
| } | ||
| if ( $subscription->payment_method_supports( 'gateway_scheduled_payments' ) && ! $subscription->is_manual() && $subscription_is_active ) { |
There was a problem hiding this comment.
Should we move the $subscription_is_active check first in this condition since it's already an established variable and may avoid us instantiating payment gateways and running gateway support checks.
There was a problem hiding this comment.
Yeah good idea. While fixing this I actually noticed that both conditions start with $subscription_is_active can they could be consolidated. Then I realised that that the $tooltip_message only changes if the subscription is active and so the "active" check can be made a top level condition.
I've fixed that in 759e7e9
Co-authored-by: Matt Allan <[email protected]>
…-subscription-list-table
There was a problem hiding this comment.
Thanks @james-allan, I've tested the latest changes and improvements and happy to get this merged!
Duplicate queries halved on my WooCommerce > Subscriptions with these changes.
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 82. This PR focuses on one key function:
get_date_column_content(). A subsequent PR will make further improvements.get_time()multiple times (up to 4 times). When rendering the Last Order Date column, each call triggered aget_related_orders()query, resulting in redundant database reads.get_time()calls when generating date columns.How to test this PR
get_related_order_metadata()137 duplicate queries.
Product impact