Improve performance of Taxon promotion rule #1409
Conversation
|
These changes look good and make sense to me 👍 However I'm a little confused about the existing behaviour of this class. |
|
That's right. I believe it's a big oversight on |
|
I've run into other inconsistencies with how |
1) Before #actionable? was bringing all product ids of the rule's taxons
from the database. On a particular case in a store with 1mil
products, the promotion was bringing 0.6mil of them, with a query ~ 5sec
and 5mil ids in Ruby.
After, the exists? query is a milliseconds one.
2) Before #eligible? was fetching all parent taxons of each taxon on a
product, which is not required for the match process.
So, given an order with 10 line items, with each line item having a
product with 3 taxons, each of them having 2 parent taxons
would mean 30 queries to the db to fetch the ancestors, bringing a
total of 60 taxons in Ruby memory.
All of them unnecessary, as the matching processes can be done solely
based on the rule's taxons, which if 5 would mean 5 queries to the DB
(self_and_descendants)
3) Fix incorrect test on #eligible?#all for taxon child of a taxon rule
case.
* there were no children tested in the case
4) On #eligible?#all, take only the IDs from the DB to
reduce memory overhead
|
Hello, I have resurrected this PR with improved variable names, and a few more details polished. I still believe it's a good addition. I think the review focus should really be on the only test, which I have modified - I believe that it was incorrectly written in first place, and wasn't actually testing what we wanted. Other than that, given that the code is not too convoluted, the other tests should server their role to ensure that I haven't broken anything with those changes. |
|
Thanks for the work on this Martin. Sorry for the slow response, but we agree that we like this code and I have rebased it with intent to merge in #2258 👍 |
|
Hey, no problem at all, glad you took the time to look at it as it can be quite confusing what is going on there. Thanks! |
Before
#actionable?was bringing all product ids of the rule's taxonsfrom the database. On a particular case in a store with 1mil
products, the promotion was bringing 0.6mil of them, with a query ~ 5sec
and 5mil ids in Ruby.
After, the
exists?query is a milliseconds one.Before
#eligible?was fetching all parent taxons of each taxon on aproduct, which is not required for the match process.
So, given an order with 10 line items, with each line item having a
product with 3 taxons, each of them having 2 parent taxons
would mean 30 queries to the db to fetch the ancestors, bringing a
total of 60 taxons in Ruby memory.
All of them unnecessary, as the matching processes can be done solely
based on the rule's taxons, which if 5 would mean 5 queries to the DB
(self_and_descendants)
Fix incorrect test on
#eligible?#allfor taxon child of a taxon rulecase.
On
#eligible?#all, take only the IDs from the DB toreduce memory overhead
On a next PR, I will see to fix
#actionable?to perform matching with the rule's descendant taxons.