Fix variant price performance regressions by mamhoff · Pull Request #4639 · solidusio/solidus

mamhoff · 2022-09-23T15:57:22Z

Summary

This PR fixes a performance regression originating from #4076 and #4073. These two PRs fixed some inconsistencies by converting variant.default_price and variant.currently_valid_prices from associations to methods. Unfortunately, they also introduced n+1 queries by attaching scopes to the prices association, essentially rendering any preloading of the prices association futile.

What this PR does is address both scope usages.

The first commit addresses the default price accessing prices.with_discarded so that a discarded variant keeps its default price. We can get around this scope usage by simply not discarding prices when a variant is discarded. Because we only really load prices through variants and not the other way around, this should have no real-world impact (and will make the unlikely case of someone actually un-deleting a variant more predictable).

The second commit addresses the currently_valid_prices method. It sorts the prices association by priority in the database. By handling the sorting in ruby, we can get rid of the scope usage here.

The third commit realizes that the default_price is the first price of the currently_valid_prices that fits the default_price_attributes and refactors some duplication out of the Spree::DefaultPrice concern.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed (~~cross them out~~ if they are not):

I have added automated tests to cover my changes.
~~I have attached screenshots to demo visual changes.~~
~~I have opened a PR to update the guides.~~
~~I have updated the readme to account for my changes.~~

codecov · 2022-09-24T09:39:06Z

Codecov Report

Merging #4639 (3f4578a) into master (20a47c8) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4639      +/-   ##
==========================================
- Coverage   86.02%   86.01%   -0.01%     
==========================================
  Files         571      571              
  Lines       14611    14605       -6     
==========================================
- Hits        12569    12563       -6     
  Misses       2042     2042

Impacted Files	Coverage Δ
core/app/models/concerns/spree/default_price.rb	`100.00% <100.00%> (ø)`
core/app/models/spree/variant.rb	`100.00% <100.00%> (ø)`
core/app/models/spree/variant/price_selector.rb	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mamhoff · 2022-09-26T08:21:13Z

Waiting on #4642 to fix the "coverage regression" :D

tvdeyen

As someone having several hundreds prices per product in the database (welcome to Europe!) I like this very much.

For the "breaking" change: I cannot imagine a single scenario where I would want to keep prices for deleted variants anyway. And with just keeping them I am fine because as you said they will not appear anywhere anyway.

Thanks 👍🏻

waiting-for-dev · 2022-09-27T12:26:09Z

@mamhoff, you can now rebase from master to fix the codecov issue 🙂

mamhoff · 2022-09-28T07:27:09Z

Rebased!

waiting-for-dev

Thanks!!

jarednorman

I begrudgingly admit this is a good change, even if it's going to break one of my apps. 😅

waiting-for-dev · 2022-09-29T04:25:59Z

I begrudgingly admit this is a good change, even if it's going to break one of my apps. 😅

Hey @jarednorman, in which way will your store be broken?

jarednorman · 2022-09-29T16:14:20Z

We have custom pricing logic and have overridden the currently_valid scope on prices, but this changes the currently_valid_prices method to no longer delegate to currently_valid.

mamhoff · 2022-09-29T16:49:38Z

Maybe the whole thing belongs into the variant_price_selector_class.

jarednorman · 2022-09-29T16:55:06Z

Yeah, there's still too many touch points for doing custom pricing right now.

When we soft-delete a variant, we also soft-delete the variant's prices. In my opinion, this is not necessary, as the association between prices and variants includes discarded prices, and harmful, as it leads to price inconsistencies between a variant before discarding and after undiscarding (if the variant has a newer, but discarded, price, we have no way of knowing that that price should not be un-discarded along with the variant).

Using the `currently_valid` scope in this instance method leads to an "n+1" issue when getting prices for more than one variant. By sorting these in memory rather than in the database, we can save a lot of database round-trips.

This uses the redefined public method from the previous commit to detect the default price. Note that through using `reverse_each.detect` rather than `min` with a block we should be able to save some iterations.

kennyadsl · 2022-10-17T14:48:10Z

I changed the label of this PR from Enhancement to Bug to reflect its content. @mamhoff going to merge, should we update the backport PRs as well?

mamhoff · 2022-10-17T15:09:29Z

I can update them on Wednesday, if you want to Backport before that, go ahead! Thank you!

kennyadsl · 2022-10-24T07:20:31Z

@mamhoff we could try using #4678 for this. What do you think?

mamhoff · 2022-10-24T08:11:15Z

Go ahead!

github-actions · 2022-10-24T10:37:43Z

💚 All backports created successfully

Status	Branch	Result
✅	v3.2
✅	v3.1

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

waiting-for-dev · 2022-10-24T10:40:52Z

Done! Please, @mamhoff, please, take a look to be sure nothing is off.

[v3.1] Merge pull request #4639 from mamhoff/do-not-discard-variant-prices-on-variant-discard

[v3.2] Merge pull request #4639 from mamhoff/do-not-discard-variant-prices-on-variant-discard

Until v3.2.3 duplicating a product duplicated the price. Since v3.2.4 the product duplication does not duplicate the price. Looks like it comes from the PR solidusio#4639 :/ To fix it we duplicate all of the master prices on the new_product.

Until v3.2.3 duplicating a product duplicated the price. Since v3.2.4 the product duplication does not duplicate the price. Looks like it comes from the PR #4639 :/ To fix it we duplicate all of the master prices on the new_product. (cherry picked from commit 00cac06)

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

github-actions Bot added the changelog:solidus_core Changes to the solidus_core gem label Sep 23, 2022

tvdeyen added the Needs Backport label Sep 23, 2022

mamhoff marked this pull request as draft September 23, 2022 16:25

mamhoff force-pushed the do-not-discard-variant-prices-on-variant-discard branch 2 times, most recently from f556d91 to 772735d Compare September 23, 2022 17:13

github-actions Bot added the changelog:solidus_backend Changes to the solidus_backend gem label Sep 24, 2022

mamhoff force-pushed the do-not-discard-variant-prices-on-variant-discard branch from d284db4 to ed2e43b Compare September 24, 2022 09:24

mamhoff marked this pull request as ready for review September 26, 2022 08:21

tvdeyen approved these changes Sep 26, 2022

View reviewed changes

tvdeyen requested a review from a team September 26, 2022 14:14

jarednorman reviewed Sep 26, 2022

View reviewed changes

Comment thread core/app/models/concerns/spree/default_price.rb Outdated

waiting-for-dev reviewed Sep 27, 2022

View reviewed changes

Comment thread core/app/models/concerns/spree/default_price.rb Outdated

waiting-for-dev added the type:enhancement Proposed or newly added feature label Sep 27, 2022

mamhoff force-pushed the do-not-discard-variant-prices-on-variant-discard branch from ed2e43b to d42b0a0 Compare September 28, 2022 07:26

mamhoff force-pushed the do-not-discard-variant-prices-on-variant-discard branch from d42b0a0 to 996e312 Compare September 28, 2022 09:24

waiting-for-dev approved these changes Sep 28, 2022

View reviewed changes

jarednorman approved these changes Sep 28, 2022

View reviewed changes

This was referenced Oct 4, 2022

[v3.1] Fix variant price performance regressions #4656

Closed

[v3.2] Fix variant price performance regressions #4657

Closed

mamhoff added 3 commits October 4, 2022 11:04

Sort currently_valid_prices in Ruby

7611923

Using the `currently_valid` scope in this instance method leads to an "n+1" issue when getting prices for more than one variant. By sorting these in memory rather than in the database, we can save a lot of database round-trips.

Use currently_valid_prices method for detecting default price

cb89fc4

This uses the redefined public method from the previous commit to detect the default price. Note that through using `reverse_each.detect` rather than `min` with a block we should be able to save some iterations.

kennyadsl added type:bug Error, flaw or fault and removed type:enhancement Proposed or newly added feature labels Oct 17, 2022

kennyadsl merged commit 425c82f into solidusio:master Oct 17, 2022

waiting-for-dev added Needs Backport-v3.2 and removed Needs Backport labels Oct 24, 2022

This was referenced Oct 24, 2022

[v3.2] Merge pull request #4639 from mamhoff/do-not-discard-variant-prices-on-variant-discard #4690

Merged

[v3.1] Merge pull request #4639 from mamhoff/do-not-discard-variant-prices-on-variant-discard #4691

Merged

waiting-for-dev added a commit that referenced this pull request Nov 8, 2022

Merge pull request #4691 from solidusio/backport/v3.1/pr-4639

36ca660

[v3.1] Merge pull request #4639 from mamhoff/do-not-discard-variant-prices-on-variant-discard

waiting-for-dev added a commit that referenced this pull request Nov 8, 2022

Merge pull request #4690 from solidusio/backport/v3.2/pr-4639

2eaa072

[v3.2] Merge pull request #4639 from mamhoff/do-not-discard-variant-prices-on-variant-discard

Roddoric mentioned this pull request Mar 8, 2023

Spree::ProductDuplicator does not duplicate the product price. #4970

Closed

Roddoric mentioned this pull request Mar 9, 2023

Spree::ProductDuplicator bug on price #4971

Merged

3 tasks

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 17, 2023

Remove deprecated currently_valid_prices

9e20094

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 17, 2023

Remove deprecated Spree::DefaultPrice#currently_valid_prices

e4d5102

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 21, 2023

Remove deprecated Spree::DefaultPrice#currently_valid_prices

22af592

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 12, 2023

Remove deprecated Spree::DefaultPrice#currently_valid_prices

3a98580

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 14, 2023

Remove deprecated Spree::DefaultPrice#currently_valid_prices

25beb19

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023

Remove deprecated Spree::DefaultPrice#currently_valid_prices

2f43672

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 24, 2023

Remove deprecated Spree::DefaultPrice#currently_valid_prices

1419cac

Please, use Spree::Variant#default_price instead. Ref solidusio#4639.

Uh oh!

Conversation

mamhoff commented Sep 23, 2022

Summary

Checklist

Uh oh!

codecov Bot commented Sep 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mamhoff commented Sep 26, 2022

Uh oh!

tvdeyen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

waiting-for-dev commented Sep 27, 2022

Uh oh!

mamhoff commented Sep 28, 2022

Uh oh!

waiting-for-dev left a comment

Choose a reason for hiding this comment

Uh oh!

jarednorman left a comment

Choose a reason for hiding this comment

Uh oh!

waiting-for-dev commented Sep 29, 2022

Uh oh!

jarednorman commented Sep 29, 2022

Uh oh!

mamhoff commented Sep 29, 2022

Uh oh!

jarednorman commented Sep 29, 2022

Uh oh!

kennyadsl commented Oct 17, 2022

Uh oh!

mamhoff commented Oct 17, 2022

Uh oh!

kennyadsl commented Oct 24, 2022

Uh oh!

mamhoff commented Oct 24, 2022

Uh oh!

github-actions Bot commented Oct 24, 2022

💚 All backports created successfully

Questions ?

Uh oh!

waiting-for-dev commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Sep 24, 2022 •

edited

Loading

waiting-for-dev commented Oct 24, 2022 •

edited

Loading