Build store configuration report in one array_merge call (#41098) - #41097
Open
lbajsarowicz wants to merge 1 commit into
Open
Build store configuration report in one array_merge call (#41098)#41097lbajsarowicz wants to merge 1 commit into
lbajsarowicz wants to merge 1 commit into
Conversation
getReport() prepended each scope's rows to the report accumulated so far with array_merge, so every website and every store view copied the whole report built up to that point. The cost grows with the square of the number of scopes while the report itself grows linearly. Collect the per-scope reports and merge them once with argument unpacking. The collected list is reversed before merging, which keeps the existing row order - stores first, then websites, then the default scope - and the phpcs:ignore annotations for Magento2.Performance.ForeachArrayMerge are no longer needed.
Contributor
Author
|
@magento run all tests |
|
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. |
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 (*)
Analytics\Model\StoreConfigurationProvider::getReport()collects 39 configuration values (asdeclared in
Analytics/etc/di.xml) for the default scope, then for every website, then forevery store view. Each scope was prepended to the report accumulated so far:
array_merge()allocates a new array and copies both operands, so each website and each storeview copies the whole report built up to that point. The work grows with the square of the
number of scopes while the report grows linearly — the merchants who feel it are exactly the
ones with many store views.
The two loops now collect the per-scope reports and merge them once:
The list is reversed before merging, which reproduces the previous prepend order exactly —
store views first (last store view first), then websites, then the default scope. I verified
the two variants produce identical arrays for the shapes in the benchmark below, not just the
same row count.
Measured effect
Merge cost only (config reads excluded, so this isolates what the change touches), 39 config
paths, PHP 8.5, median of 5 runs:
Websites are counted separately, so an installation pays this for websites and store views
both.
Related
Magento2.Performance.ForeachArrayMergeis meant to catch this shape, but it warns on everyarray_mergeinside afor/foreachbody — including calls that copy a constant amount ofdata — and never inspects
while/dobodies. 2.4-develop carries 77phpcs:ignore Magento2.Performance.ForeachArrayMergeannotations as a result, the two removedhere among them.
magento/magento-coding-standard#503 narrows the sniff to the accumulating shape and adds
while/docoverage (126 raw detections in 2.4-develop → 74, plus one new true finding).Reviewing and merging it would make the remaining suppressions meaningful again — input from
the core team there is very welcome.
Manual testing scenarios (*)
(Stores > Configuration > General > Advanced Reporting).
bin/magento analytics:collect-data(or wait for theanalytics_collect_datacron) andinspect the generated
store_config.csvin the archive: same rows, same order as before thechange — store view rows first, then website rows, then the default scope.
vendor/bin/phpunit -c dev/tests/unit/phpunit.xml.dist app/code/Magento/Analytics/Test/Unit/— 175 tests green, including
StoreConfigurationProviderTest.Contribution checklist (*)