[RFC] Just touch taxons in Product#touch_taxons #4239
Conversation
tvdeyen
force-pushed
the
simplify-touch_taxons
branch
from
c89c881
to
ea53670
Compare
|
@tvdeyen Thanks for taking care of this! I'm seeing the test suite for this PR is taking ~1 hour compared to the ~10 minutes of the other builds. Even if the single spec added takes ~5 seconds, I'm concerned that this change adds a significant amount of time by getting performance worse on every single test that interacts with products and taxons. If that's the case, I don't think we should merge this as is. On a side note, did you consider providing this new behavior optional with a preference? Maybe we could have the past approach as default, with optimization in mind and the new one, more reliable in terms of deadlock as optional. I know of stores that have products constantly updated by external tools (for content or inventory management sync), in these cases we will trigger a lot of these touches and I'm afraid that this version would significantly slow down their operations (assuming the current version is barely acceptable for them, which I'm not sure). |
| @@ -33,5 +33,9 @@ | |||
| expect(product.taxons.count).to eq(50) | |||
| expect { subject }.to_not raise_error | |||
| end | |||
|
|
|||
| it "is fast" do | |||
| puts Benchmark.measure { subject } | |||
@kennyadsl The spec that I added took 5647.54 (1.5h) to run under postgresql on CircleCI. In mysql it finished in 587.86s (9 minutes). Most of the time is spent creating 1000 taxons, nested 10 levels deep. The actual spec of updating 50 taxons (and their ancestors) runs in 8ms on my machine. Disclaimer: I never intended to keep the spec. I just wanted to verify it does not cause any deadlocks even with unrealistically high numbers of taxons. Locally this spec runs with postgres in under 10 seconds. I am not sure why this takes so long on CircleCI.
I thought about this, yes. But is this actually necessary? See the note below.
My test shows that this should not have an impact on these stores, because even though we touch a lot of products, how many taxons are these products attached to? 1, 2, 10? My test assigns a product to 50 taxons (very unlikely for real life stores IMO) and it takes ~8ms to touch these 50 taxons (and their 10 ancestors). These numbers are from an 8 core Apple M1 Pro with 32GB of RAM. But even if they would be 10 x slower it still would only take 80ms to update 50 taxons. Most products I know of are assigned to 1-5 taxons. If we are still concerned (and honestly the CircleCI times concern me), we could add the new behavior as an opt-in. For my use-case this would be fine. But shouldn't we ship "reliable" as the default option then? And fast/unreliable as opt-in?
@jarednorman I agree, but does a call to |
|
I would assume it does, but we can check. |
|
I've confirmed that |
kennyadsl
added
release:major
|
Hey @tvdeyen! I think we can try to move this forward now. What about rebasing to try to have the test suite green? After we verified that there are no deadlocks, we can remove the slow "benchmark" specs and merge the PR. Sounds good? |
And make use of the `after_touch` callback of `Spree::Taxon`, that already handles touching ancestors and its taxonomy. This hopefully fixes solidusio#3931 as well, because it runs each `touch` callback in it's own transaction instead of one large transaction that might cause dead locks. This IS slower than the optimized approach before, but this should not be that much of an issue, since we are updating a product here and this will a) not happen that much and b) there will not be many taxons one single product will be attached to, right? Even in large stores this will most likely be in single to double digits, instead of hundreds or thousands. But I might be painfully wrong here. This also has the advantage that a `after_touch` callback a store or an extenstion might have will be triggered and caches will be invalidated correctly.
1000 taxons nested 10 levels deep. A product that is attached to 50 of it gets updated and causes each of its taxons to update, which will trigger all ancestors and the taxonomy to be updated as well. No dead locks.
We saw very slow run times on circle ci. In order to debug we want to know if the time is spent in creating the records or updating them.
tvdeyen
force-pushed
the
simplify-touch_taxons
branch
from
e67aabb
to
9fe8ea1
Compare
Description
And make use of the
after_touchcallback ofSpree::Taxon,that already handles touching ancestors and its taxonomy.
This hopefully fixes #3931 as well, because it runs each
touchcallback in it's own transaction instead of one large transaction
that might cause dead locks.
This IS slower than the optimized approach before, but this should not
be that much of an issue, since we are updating a product here
and this will a) not happen that much and b) there will not be many taxons
one single product will be attached to, right? Even in large stores this
will most likely be in single to double digits, instead of hundreds
or thousands. But I might be painfully wrong here.
This also has the advantage that a
after_touchcallback a store or anextension might have will be triggered and caches will be invalidated
correctly.
DISCLAIMER: I did not added any tests on purpose, because I want to discuss if this is an approach we want to try first.Actually I added a spec that creates a taxon tree of 1000 taxons nested 10 random levels deep. The product that then will be updated is attached to 50 random taxons. We expect no errors.
Checklist: