Clone all ExchangeVariant objects in bulk #10832
Merged
Conversation
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
An ExchangeVariant is a simple representation of a join table between Exchange and Variant. Previously this code was triggering an additional INSERT query for every variant added to the newly cloned exchange. Some exchanges have ~3000 variants! The code now creates them in bulk in a single INSERT statement. When cloning large order cycles this can improve performance by ~1000% or so.
mkllnk
approved these changes
May 12, 2023
mkllnk
added a commit
to mkllnk/ofn-install
that referenced
this pull request
May 12, 2023
This should be reset after the cloning has been made more efficient: openfoodfoundation/openfoodnetwork#10832
|
The build fail seems to be in master. |
abdellani
approved these changes
May 13, 2023
drummer83
added
pr-staged-fr
|
Hi @Matt-Yorkley, I worked with two order cycles:
Also I have verified that
I think this one is ready for merging. I will do that! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What? Why?
Fixes server-melting performance issues when cloning large order cycles 👉 openfoodfoundation/ofn-install#873 (comment)
Review Notes
An
ExchangeVariantis a simple representation of a join table betweenExchangeandVariant. Previously this code was triggering an additionalINSERTquery for every variant associated to the newly cloned exchange. Some exchanges have ~3000 variants! The code now associates them in bulk in a singleINSERTstatement. When cloning large order cycles this can radically improve the performance.Hot tip: using the
ActiveRecord#insert_allmethod is not advised on any objects that have callbacks, as it skips them.ExchangeVarianthas no callbacks 👍Before, production data and cloning an order cycle with ~180 variants and multiple exchanges:
2.8 seconds, 1176 queries, ~2 billion memory allocations!
After, cloning the same order cycle with the same data:
0.2 seconds, 56 queries, ~127 thousand memory allocations.
What should we test?
Cloning order cycles. Make sure all the variants are there in the exchanges for the newly cloned OC as expected.
Release notes
Changelog Category: Technical changes
The title of the pull request will be included in the release notes.