You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Metric: cart completion with 4/5 items took 11 seconds when it takes less than 2 seconds now, it can vary depending on the machines. As part of those 2 sec there is 1 sec for stripe (called 3 times) and few hundreds milliseconds for external tax api call
Cart/Order performance improvement, more specifically during cart completion
The purpose of that Pr is to drastically reduce the response time of the cart completion strategy.
The improvements made here have also some side effects which improve the performances of other part of the code but
it was not the main goal of it.
All improvements will be applicable in later PR's to also improve everything related to cart and order end point
and totals calculations.
Totals calculation
Some improvements have been made to the totals service, in order to not break the existing API right away, it has been
decided by myself (but can evolve) to create a new service TotalsNewService where all new methods are implemented and consumed from.
Generally speaking there is no mention of any specific entities such as the Cart or Order entity as the different methods
explicitly describe the expected arguments which makes it flexible and do not enforce to pass a specific entity
but only an object that contains the expected properties. The overall idea is to have a totals service
less coupled to specific entities but just do its job which is to compute some calculations based on the
expected arguments.
The overall changes are the following:
getLineItemsTotals
This method accept a collection of items as well as some options
includeTax which only enforce the tax_lines to exist either provided by the item itself or the ones that will be queried internally through the tax provider service if useExistingTaxLines is not set to true
calculationContext which is the usual calculation context
taxRate which is only there to support backward compatibility. I choose to keep it but the methods have been separated
useExistingTaxLines which enforce the usage of the tax lines from the items. Can be useful in cases where
the tax_lines already exists or have been already fetched and assigned by the consumer in order to not re fetch them if
it is not necessary
This method is of a higher rank as it will only take care of using the right tax lines and call another method
to compute the totals of each item.
To compute the totals of each item there is now two methods which are described bellow
getLineItemTotalsLegacy
As the name describe, this method calcul the totals for the legacy backward compatibility cases where the taxRate is
explicitly provided to be used. It is normally in the case of the order.
getLineItemTotals
Which is used to calculate the totals with the new approach using the tax lines.
In that method the options includeTax as been kept only to throw if no tax lines are provided by any of the
possible way, either from the item or explicitly provided
getLineItemRefund
This method take a partial item composed of the necessary properties needed to calculate the refund amount.
It is also possible to provide an optional taxRate to calculate it the legacy way where the method will call
the getLineItemRefundLegacy method for that.
getLineItemRefundLegacy
Now let's look at the gift card part of the totals below
getGiftCardTotals
This method take now a maximun amount that can be gifted, it is possible to pass either the giftCards or the giftCardTransactions
as part of the options. If the giftCardTransactions are provided then a separate method will be called getGiftCardTransactionsTotals
in order to provided the right calculation for that collection otherwise the giftCards will be used
to calculate the amount.
getGiftCardTransactionsTotals
This method take care of computing the totals based on the transactions and the region
getShippingMethodsTotals
This method accept a collection of shippingMethods as well as some options
includeTax which only enforce the tax_lines to exist either provided by the method itself or the ones that will be queried internally through the tax provider service if useExistingTaxLines is not set to true
discounts
calculationContext which is the usual calculation context
taxRate which is only there to support backward compatibility. I choose to keep it but the methods have been separated
useExistingTaxLines which enforce the usage of the tax lines from the method. Can be useful in cases where
the tax_lines already exists or have been already fetched and assigned by the consumer in order to not re fetch them if
it is not necessary
This method is of a higher rank as it will only take care of using the right tax lines and call another method
to compute the totals of each method.
To compute the totals of each method there is now two methods which are described bellow
getShippingMethodTotalsLegacy
As the name describe, this method calcul the totals for the legacy backward compatibility cases where the taxRate is
explicitly provided to be used. It is normally in the case of the order.
getShippingMethodTotals
Which is used to calculate the totals with the new approach using the tax lines.
In that method the options includeTax as been kept only to throw if no tax lines are provided by any of the
possible way, either from the method or explicitly provided
Cart completion
The main purpose of the previous improvements and refactoring was to improve the overall performances
of the cart completion flow. To achieve that, multiple changes needed to be made and will be described bellow.
Cart service and Order service
Most of the changes in the service are separated in two categories
Methods signature and refactoring
Improvements of the decorate totals method
Some of the method was always expecting a cart id and then was querying the expected cart. In some cases
we are already holding the cart and it make sense that we have the flexibility to provide this cart
instead of fetching it again, expect in those cases where we are not sure what to provide and therefore
we prefer giving the hand to the method to fetch if with the appropriate relations. Therefore for some of the
service methods it is now possible to either give and id or the entity directly. For the later case it
is much more for the people in need of performance improvement and in that case it is the consumer who must
be sure to pass the entity with the appropriate relations.
Finally, the decorateTotals had been reworked in order to provide a better readability, understandability and performances.
To do so, the hidden quadratic and cubic loops have been replaces by the usage of map or object and the overall
loops have been simplified or reduced. We have therefore remove O(n^(2 or 3)) + XO(n) by only XO(n) where X is inferior
to what it was before. Also, we now take advantage of being able to query the tax lines or use the existing one
before calculating the total which gives more flexibility and reduce the number of time we rely on an
external party. For the example, for a cart with 4 items we have now few calls to the external party instead of 64 for the overall flow.
Before those changes, the number a call was constantly increasing with the number of item in the cart, it is not
the case anymore.
The overall decorate totals of the order service have also been improved the same way the cart has been improved and therefore
the readability, understandability and performances have been improved.
Summary
Some other adjustments have been made along the way in order to improve the usability and simplify the usage of
all the changes and open the door to more refactoring (but simpler) in the entire code base where we are relying
on the totals, cart and order. Therefore, future PR's will be open to tackle those refactoring and align the code base
to move toward the new ways of calculating the totals and the usage of the new api's. It will increase the overall
performances of everything that is related to the cart or the order. It should be taken into consideration
that the future changes will probably also impact the payment provider if they rely on one of the entities
mention above. Also, the actual totals service could potentially be deprecated until a certain period of time
in favour of the new approached.
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Woooo, I'm excited to try this out tomorrow! Can you try adding a similar thing with the tax_lines for OrderService::decorateTotals also? I'm seeing some long non-database performance times listing orders as well. Thanks!
I ve added the new unit tests suite for the new totals service (this is why the PR look enormous now, a lot of tests 😹).
I have removed the notion of useExistingTaxLines in order to simplify the api usage. Since the getTaxLines is now properly manage it does not add a lot of overhead so it is a trade off.
I have re applied the changes made to the get-order end point and cleaned up the legacy decorate totals
various clean up
I did add some todo for the places where we are using entire entities in the creation process which can lead to errors with cascade insert if some data should not be inserted but are present (tax lines on the items for example)
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
adrien2p
force-pushed
the
feat/totals-improvements
branch
from
Cart/Order performance improvement, more specifically during cart completion
The purpose of that Pr is to drastically reduce the response time of the cart completion strategy.
The improvements made here have also some side effects which improve the performances of other part of the code but
it was not the main goal of it.
All improvements will be applicable in later PR's to also improve everything related to cart and order end point
and totals calculations.
Totals calculation
Some improvements have been made to the totals service, in order to not break the existing API right away, it has been
decided by myself (but can evolve) to create a new service
TotalsNewServicewhere all new methods are implemented and consumed from.Generally speaking there is no mention of any specific entities such as the Cart or Order entity as the different methods
explicitly describe the expected arguments which makes it flexible and do not enforce to pass a specific entity
but only an object that contains the expected properties. The overall idea is to have a totals service
less coupled to specific entities but just do its job which is to compute some calculations based on the
expected arguments.
The overall changes are the following:
This method accept a collection of
itemsas well as some optionsincludeTaxwhich only enforce the tax_lines to exist either provided by the item itself or the ones that will be queried internally through the tax provider service ifuseExistingTaxLinesis not set to truecalculationContextwhich is the usual calculation contexttaxRatewhich is only there to support backward compatibility. I choose to keep it but the methods have been separateduseExistingTaxLineswhich enforce the usage of the tax lines from the items. Can be useful in cases wherethe tax_lines already exists or have been already fetched and assigned by the consumer in order to not re fetch them if
it is not necessary
This method is of a higher rank as it will only take care of using the right tax lines and call another method
to compute the totals of each item.
To compute the totals of each item there is now two methods which are described bellow
As the name describe, this method calcul the totals for the legacy backward compatibility cases where the
taxRateisexplicitly provided to be used. It is normally in the case of the order.
Which is used to calculate the totals with the new approach using the tax lines.
In that method the options
includeTaxas been kept only to throw if no tax lines are provided by any of thepossible way, either from the item or explicitly provided
This method take a partial item composed of the necessary properties needed to calculate the refund amount.
It is also possible to provide an optional
taxRateto calculate it the legacy way where the method will callthe getLineItemRefundLegacy method for that.
Now let's look at the gift card part of the totals below
This method take now a maximun amount that can be gifted, it is possible to pass either the
giftCardsor thegiftCardTransactionsas part of the options. If the
giftCardTransactionsare provided then a separate method will be calledgetGiftCardTransactionsTotalsin order to provided the right calculation for that collection otherwise the
giftCardswill be usedto calculate the amount.
This method take care of computing the totals based on the transactions and the region
This method accept a collection of
shippingMethodsas well as some optionsincludeTaxwhich only enforce the tax_lines to exist either provided by the method itself or the ones that will be queried internally through the tax provider service ifuseExistingTaxLinesis not set to truediscountscalculationContextwhich is the usual calculation contexttaxRatewhich is only there to support backward compatibility. I choose to keep it but the methods have been separateduseExistingTaxLineswhich enforce the usage of the tax lines from the method. Can be useful in cases wherethe tax_lines already exists or have been already fetched and assigned by the consumer in order to not re fetch them if
it is not necessary
This method is of a higher rank as it will only take care of using the right tax lines and call another method
to compute the totals of each method.
To compute the totals of each method there is now two methods which are described bellow
As the name describe, this method calcul the totals for the legacy backward compatibility cases where the
taxRateisexplicitly provided to be used. It is normally in the case of the order.
Which is used to calculate the totals with the new approach using the tax lines.
In that method the options
includeTaxas been kept only to throw if no tax lines are provided by any of thepossible way, either from the method or explicitly provided
Cart completion
The main purpose of the previous improvements and refactoring was to improve the overall performances
of the cart completion flow. To achieve that, multiple changes needed to be made and will be described bellow.
Cart service and Order service
Most of the changes in the service are separated in two categories
Some of the method was always expecting a cart id and then was querying the expected cart. In some cases
we are already holding the cart and it make sense that we have the flexibility to provide this cart
instead of fetching it again, expect in those cases where we are not sure what to provide and therefore
we prefer giving the hand to the method to fetch if with the appropriate relations. Therefore for some of the
service methods it is now possible to either give and id or the entity directly. For the later case it
is much more for the people in need of performance improvement and in that case it is the consumer who must
be sure to pass the entity with the appropriate relations.
Finally, the
decorateTotalshad been reworked in order to provide a better readability, understandability and performances.To do so, the hidden quadratic and cubic loops have been replaces by the usage of map or object and the overall
loops have been simplified or reduced. We have therefore remove O(n^(2 or 3)) + XO(n) by only XO(n) where X is inferior
to what it was before. Also, we now take advantage of being able to query the tax lines or use the existing one
before calculating the total which gives more flexibility and reduce the number of time we rely on an
external party. For the example, for a cart with 4 items we have now few calls to the external party instead of 64 for the overall flow.
Before those changes, the number a call was constantly increasing with the number of item in the cart, it is not
the case anymore.
The overall decorate totals of the order service have also been improved the same way the cart has been improved and therefore
the readability, understandability and performances have been improved.
Summary
Some other adjustments have been made along the way in order to improve the usability and simplify the usage of
all the changes and open the door to more refactoring (but simpler) in the entire code base where we are relying
on the totals, cart and order. Therefore, future PR's will be open to tackle those refactoring and align the code base
to move toward the new ways of calculating the totals and the usage of the new api's. It will increase the overall
performances of everything that is related to the cart or the order. It should be taken into consideration
that the future changes will probably also impact the payment provider if they rely on one of the entities
mention above. Also, the actual totals service could potentially be deprecated until a certain period of time
in favour of the new approached.
FIXES CORE-327