Skip to content

[Performance] Fix array merge in loop to speed up static content deploy #22248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

ihor-sviziev
Copy link
Contributor

Description (*)

This PR improving static content deploy speed.

For instance i tried to compile Magento/blank theme for 6 different locales (I have all these language packs installed).

php bin/magento setup:static-content:deploy -f --jobs=2 --area frontend en_US locale2 locale3 locale4 locale5 locale6 --theme Magento/blank

Before applying this patch: 109 seconds on my machine.
After applying this patch: 93 seconds on my machine.

If we'll execute for SCD for multiple theme - results should be bigger.

More info about array_merge in loop:
https://github.com/kalessil/phpinspectionsea/blob/master/docs/performance.md#slow-array-function-used-in-loop

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

N/A

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@m2-assistant
Copy link

m2-assistant bot commented Apr 9, 2019

Hi @ihor-sviziev. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ihor-sviziev if I remember correctly, we have a rule in recently released coding standard for array_merge in loop. Could you please check?

@orlangur
Copy link
Contributor

@ihor-sviziev ok, that's here I saw it: magento/magento-coding-standard#72 (comment)

So, standard is just evolving without any connection with reality :)

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-4723 has been created to process this Pull Request

@VasylShvorak
Copy link
Contributor

✔️ QA passed

foreach ($result as $path) {
$result = array_merge($result, $this->collectFileMap($path));
$result[] = $this->collectFileMap($path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An integration test is failing with provided changes: \Magento\Deploy\DeployTest
After the short investigation, we found that $path var can be the type of array and will break the check $this->map[$fileName].

@sidolov
Copy link
Contributor

sidolov commented May 17, 2019

Hi @ihor-sviziev , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this May 17, 2019
@m2-assistant
Copy link

m2-assistant bot commented May 17, 2019

Hi @ihor-sviziev, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@ihor-sviziev ihor-sviziev added the WIP Work In Progress label Jun 6, 2019
@ihor-sviziev ihor-sviziev reopened this Jun 6, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jun 6, 2019

Hi @ihor-sviziev. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@m2-assistant
Copy link

m2-assistant bot commented Jul 11, 2019

Hi @ihor-sviziev, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@orlangur
Copy link
Contributor

@ihor-sviziev wazzup?)

@ihor-sviziev
Copy link
Contributor Author

@ihor-sviziev wazzup?)

There is an issue that really hard to debug. I don't have free time for now.

@ihor-sviziev ihor-sviziev removed the WIP Work In Progress label Jul 18, 2019
@ihor-sviziev ihor-sviziev deleted the fix-array-merge-in-loop branch September 23, 2020 06:43
@ihor-sviziev
Copy link
Contributor Author

FYI I applied the same change (and fixed it) in the #30153, it already got merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants