perf(weee): cache parent lookups in ConfigurableVariationAttributePriority (#40642)#40836
Open
lbajsarowicz wants to merge 2 commits into
Open
Conversation
…ority The plugin called by Tax::getProductWeeeAttributes iterated every configurable parent of every child product and re-loaded each parent via the product repository on every invocation. On category and search listing pages this fanned out into a tight N+1: the same configurable parent was loaded N times for its N visible variants, and the loop overwrote `$result` on every iteration instead of returning the first non-empty match. Profiling on 2.4.8-p4 attributes ~30% of getAmount wall time to this path. Add per-request memoisation keyed by: - child id -> parent ids (skip getParentIdsByChild on repeats), - parent id + scope signature (shipping/billing/website/calculate/round) -> weee attributes (skip repeated parent product loads and tax computations across variants of the same configurable). Bail out as soon as a non-empty result is found and avoid the loop entirely when the original result already has data. Implement ResetAfterRequestInterface so the caches do not leak across web requests in single-process containers. Fixes magento#40642
|
Hi @lbajsarowicz. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
Contributor
Author
|
@magento run all tests |
Contributor
Author
|
@magento run all tests |
This file contains hidden or 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
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.
Description
The plugin invoked by
Tax::getProductWeeeAttributesiterated everyconfigurable parent of every child product and re-loaded each parent
via the product repository on every invocation. On category and search
listing pages this fanned out into a tight N+1 — the same configurable
parent was loaded N times for its N visible variants — and the loop
overwrote
\$resulton every iteration instead of returning the firstnon-empty match. The reporter's profiling on 2.4.8-p4 attributes ~30%
of
Calculator::getAmountwall time to this path.Added per-request memoisation keyed by:
getParentIdsByChildon repeats),-> weee attributes (skip repeated parent product loads across variants
of the same configurable).
Bail out as soon as a non-empty result is found, and skip the loop
entirely when the original result already has data. Implement
ResetAfterRequestInterfaceso the caches do not leak across webrequests in single-process containers.
Fixes #40642
Fixed Issues
Performance Impact
Reproduced on a catalog of configurables with 6 child variants each.
With Weee tax enabled and no override on children:
getProductWeeeAttributestriggersproductRepository::getByIdcache reuses across variants -> O(unique parents).
Manual testing scenarios
attribute populated only on the parent.
Blackfire / Xdebug / SPX:
getProductWeeeAttributescalls collapseto one per unique parent for the request.
Contribution checklist