Skip to content
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

Performance optimizations #25125

Merged
merged 10 commits into from
Dec 30, 2019
Merged

Performance optimizations #25125

merged 10 commits into from
Dec 30, 2019

Conversation

andrey-legayev
Copy link
Contributor

Description (*)

I've prepared several performance optimizations based on my XDebug profiling and debugging of add to cart process and shopping cart page.

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

General regression testing

Questions or comments

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 are green)

@m2-assistant
Copy link

m2-assistant bot commented Oct 17, 2019

Hi @andrey-legayev. 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 Guide documentation.

@andrey-legayev
Copy link
Contributor Author

Can someone help me to fix following findings of Static Tests?

  1. I didn't change this part and I don't think I should:
FILE: ...ml/lib/internal/Magento/Framework/View/Element/AbstractBlock.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
  19 | WARNING | Abstract classes MUST NOT be marked as public @api.
 248 | WARNING | Empty FUNCTION statement detected
  1. There is {inheritdoc} instruction - isn't it valid?
FILE: ...al/Magento/Framework/App/ObjectManager/ConfigLoader/Compiled.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 29 | ERROR | [x] Missing @param for argument in method annotation

@@ -50,8 +53,7 @@ public function render(array $source, array $arguments)
{
$text = end($source);
/* If phrase contains escaped quotes then use translation for phrase with non-escaped quote */
$text = str_replace('\"', '"', $text);
$text = str_replace("\\'", "'", $text);
$text = strtr($text, ['\"' => '"', "\\'" => "'"]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested: str_replace() is slower than strtr() in this case

@ihor-sviziev ihor-sviziev self-assigned this Oct 18, 2019
@andrey-legayev
Copy link
Contributor Author

Static tests have been fixed.
@ihor-sviziev - thank you!

@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Oct 18, 2019
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6126 has been created to process this Pull Request

@andrey-legayev
Copy link
Contributor Author

I've run performance tests for string concat vs. array join:
https://gist.github.com/andrey-legayev/24ed101a34d4775bf1b6f9879581f51a

No real performance improvement (which surprised me).
But anyway - I'm rolling back string concatenation improvements, because it doesn't make any sense.

I've run performance tests for string concat vs. array join:
https://gist.github.com/andrey-legayev/24ed101a34d4775bf1b6f9879581f51a

No real performance improvement (which surprised me).
But anyway - I'm rolling back string concatenation improvements, because it doesn't make any sense.
@slavvka
Copy link
Member

slavvka commented Dec 9, 2019

@engcom-Foxtrot please take a look at static tests and seems like some EE and B2B are failing as well

@engcom-Golf
Copy link
Contributor

Hi @andrey-legayev Please fix failed static test and Functional Test, as we have no access to your repository, thank you!
DeepinScreenshot_select-area_20191210100527

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6126 has been created to process this Pull Request

@magento-engcom-team magento-engcom-team merged commit 4deec6b into magento:2.4-develop Dec 30, 2019
@m2-assistant
Copy link

m2-assistant bot commented Dec 30, 2019

Hi @andrey-legayev, 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.

This pull request was closed.
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.

7 participants