Reduce per-product work in listing swatch renderer (#39073) - #41228
Reduce per-product work in listing swatch renderer (#39073)#41228lbajsarowicz wants to merge 1 commit into
Conversation
Resolve the configurable attribute list once per parent in getConfigurableOptionsIds() instead of once per child variation, and limit the fallback variation collection to a single row since only the first item is used. Both scale with the number of variations per configurable, which is where listings with large configurables spend their time.
|
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. |
|
@magento run all tests |
| foreach ($attributeCodes as $attributeCode) { | ||
| $attributeValue = $product->getData($attributeCode); | ||
| if ($attributeValue !== null) { | ||
| $ids[$attributeValue] = 1; |
There was a problem hiding this comment.
If only keys needed from $ids - the subsequent array_keys fcall can be also eliminated after changing $ids[$attributeValue] = 1; to $ids[] = $attributeValue;.
There was a problem hiding this comment.
Checked this against the data flow before changing anything, and I am going to keep array_keys($ids).
$attributeValue does repeat across the loop. It is nested per allowed product and per swatch attribute code, and sibling variations routinely share a value — ten children in "Red", five in size "L". $ids[$attributeValue] = 1 is a deliberate dedup, so array_keys($ids) returns one entry per distinct option, not one per product.
That dedup is essential downstream. The result goes to SwatchHelper::getSwatchesByOptionsId(), which decides whether the cache already holds everything with:
$swatches = $this->getCachedSwatches($optionIds);
if (count($swatches) !== count($optionIds)) {and getCachedSwatches() is array_intersect_key($this->swatchesCache, array_combine($optionIds, $optionIds)). Both array_combine and array_intersect_key key by option id, so count($swatches) can never exceed the number of distinct ids. Feed duplicates in and count($optionIds) is permanently larger, the check never passes even with a fully warm cache, and every call falls through to addFilterByOptionsIds() and a fresh collection query.
That would defeat swatch caching on any listing with repeated swatch values, which is the normal case, and it works against exactly what this PR is for. No test catches it today because the unit fixtures use small already-distinct value sets.
Separately, on the red Semantic Version Checker: it reports one MAJOR, M121 [protected] Method return typing changed on getSwatchProductImage. The method is protected, has no declared return type, and could already return null — the pre-patch code simply fell off the end when neither branch matched while the docblock claimed @return string. This PR adds the explicit return null; and corrects the docblock to @return string|null. Runtime behaviour is unchanged; SVC is flagging a documentation correction. I would rather leave the docblock accurate than restore a wrong one to silence the check, but say the word if you want it reverted.
There was a problem hiding this comment.
You’re right deduplication needs to be preserved, so $ids[] = $attributeValue isn’t equivalent.
It can instead use $ids[$attributeValue] = $attributeValue and return $ids directly.
The downstream code consumes the values and does not require sequential keys.
The gain is very small, though, so either implementation is fine.
engcom-Bravo
added
the
Priority: P3
|
|
||
| return null; |
There was a problem hiding this comment.
@lbajsarowicz, Do you think you could avoid changing the return type of the protected method so that the Semantic Version Check passes without errors? This would allow to include these changes in a minor release without requiring approval from the Architect.
4 participants
Description (*)
Two contained reductions of per-configurable work in the category listing swatch renderer. They do not remove the nine per-parent SQL statements the issue measures (each is intrinsically parent-scoped, and everything parent-independent is already memoized in EAV
Source\Table,Swatches\Helper\DataandSwatchAttributeCodes), so this is a partial improvement on the axis the reporter is on, variations per configurable:Renderer\Configurable::getConfigurableOptionsIds()calledhelper->getAllowAttributes($product)inside the per-child loop, resolving the parent's configurable attributes once per variation. With 250 variations and 12 listed products that is 3000 resolutions per page. It is now resolved once per parent; the option id set is unchanged.Helper\Data::loadVariationByFallback()loaded the whole matching child collection and usedgetFirstItem(). It now setssetPageSize(1). This path runs per parent fromListing\Configurable::_getAdditionalConfig()whenever a swatch attribute is in the layered navigation query.Also fixes the pre-existing Static Tests warnings in the touched block (
public const,@seeon deprecated members, explicitreturn null).Fixed Issues (if relevant)
Manual testing scenarios (*)
Swatches\Helper\Data::getAllowAttributescall count drops from children x parents to parents.Contribution checklist (*)