Build cache tag id lists in one array_merge call (#41098) - #41095
Open
lbajsarowicz wants to merge 1 commit into
Open
Build cache tag id lists in one array_merge call (#41098)#41095lbajsarowicz wants to merge 1 commit into
lbajsarowicz wants to merge 1 commit into
Conversation
FilesystemTagAdapter::getIdsMatchingAnyTags() and ::getAllIds() and RedisTagAdapter::getIdsMatchingAnyTags() merged each tag's id list into an accumulator on every iteration, so every iteration copied the whole result built so far. Cleaning the cache by tags walks one iteration per tag, which makes the cost grow with the square of the number of tags. Collect the per-tag lists and merge them once with argument unpacking. The resulting arrays and their order are identical, and the phpcs:ignore annotations for Magento2.Performance.ForeachArrayMerge are no longer needed.
|
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 |
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.
Fixed Issues (if relevant)
That issue covers three modules, so this pull request is one of three and does not close it on its own.
Description (*)
Cleaning the cache by tags goes through
Symfony::clean(), which callsTagAdapterInterface::getIdsMatchingAnyTags()(CLEANING_MODE_MATCHING_ANY_TAG) or::getIdsNotMatchingTags()(CLEANING_MODE_NOT_MATCHING_TAG). Three of those loops mergedeach tag's id list into an accumulator on every iteration:
array_merge()allocates a new array and copies both operands, so a loop of this shapecopies the whole partial result once per iteration — the total work grows with the square of
the number of tags, not linearly.
FilesystemTagAdapter::getAllIds()does it once per tagfile in the cache directory, and a store that invalidates by entity tags accumulates a large
number of those.
The three loops now collect the per-tag lists and merge them once with argument unpacking:
Same arrays, same order, same de-duplication — only the number of copies changes. The
// phpcs:ignore Magento2.Performance.ForeachArrayMergeannotations that were needed to keepthese lines quiet are gone.
RedisTagAdapter::getIdsMatchingAnyTags()gets the same treatment for its chunkedSUNIONpath (chunks of 500 tags, so the loop runs once per 500 tags).
Measured effect
FilesystemTagAdapteragainst a synthetic tag directory (20 ids per tag), PHP 8.5, median of5 runs for 5 000 tags, single run for 20 000:
getIdsMatchingAnyTags()getIdsNotMatchingTags()getIdsMatchingAnyTags()getIdsNotMatchingTags()The remaining time is file reads and
array_unique(), which this change does not touch. Thebenchmark script is a plain PHP file that instantiates the adapter with a stub
CacheItemPoolInterfaceand a temporary tag directory — happy to add it to the descriptionif that helps review.
Related
The sniff that is supposed to catch this shape,
Magento2.Performance.ForeachArrayMerge,warns on every
array_mergeinside afor/foreachbody — including calls that copy aconstant amount of data — and never looks inside
while/dobodies at all. That is why2.4-develop carries 77
phpcs:ignore Magento2.Performance.ForeachArrayMergeannotations,and why these three loops were annotated instead of fixed.
magento/magento-coding-standard#503 narrows the sniff to the accumulating shape and adds
while/docoverage (126 raw detections in 2.4-develop → 74, one new true finding). Reviewingand merging it would make the remaining suppressions in this repository meaningful again — the
core team's input there is very welcome.
Manual testing scenarios (*)
holds a large number of tag files.
bin/magento cache:clean <type>, or save a product socatalog_product_*tags are cleaned) and confirm exactly the same cache entries are removed as before.
vendor/bin/phpunit -c dev/tests/unit/phpunit.xml.dist lib/internal/Magento/Framework/Cache/Test/Unit/— 336 tests green, including
FilesystemTagAdapterTestandRedisTagAdapterTest.Contribution checklist (*)