-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 improvements #38642
base: 2.4-develop
Are you sure you want to change the base?
Performance improvements #38642
Conversation
Hi @qsolutions-pl. Thank you for your contribution! Add the comment under your pull request to deploy test or vanilla Magento instance:
❗ Automated tests can be triggered manually with an appropriate comment:
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
$newPath = $path . '/' . $key; | ||
} else { | ||
$newPath = $key; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be improved with
$newPath = $path ? $path . '/' . $key : $key;
* @throws \Magento\Framework\Exception\RuntimeException | ||
*/ | ||
public function getFileCache(array $cache, string $filePath, \Magento\Framework\Filesystem\DriverInterface $fileDriver): array | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that the getFileCache
method is only used within the Reader class, it should be declared as a private scope.
̶I̶t̶ ̶a̶p̶p̶e̶a̶r̶s̶ ̶t̶h̶a̶t̶ ̶t̶h̶e̶ ̶̶g̶e̶t̶F̶i̶l̶e̶C̶a̶c̶h̶e̶
̶ ̶d̶o̶e̶s̶ ̶n̶o̶t̶ ̶t̶h̶r̶o̶w̶ ̶\̶M̶a̶g̶e̶n̶t̶o̶\̶F̶r̶a̶m̶e̶w̶o̶r̶k̶\̶E̶x̶c̶e̶p̶t̶i̶o̶n̶\̶F̶i̶l̶e̶S̶y̶s̶t̶e̶m̶E̶x̶c̶e̶p̶t̶i̶o̶n̶.̶ ̶C̶a̶n̶ ̶y̶o̶u̶ ̶c̶o̶n̶f̶i̶r̶m̶ ̶t̶h̶i̶s̶?̶ Please ignore this since the $fileDriver->isExists()
function call may throw this exception.
} | ||
} | ||
return $cache; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be beneficial if all namespaces were imported:
\Magento\Framework\Filesystem\DriverInterface
\Magento\Framework\Exception\FileSystemException
\Magento\Framework\Exception\RuntimeException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the benefit, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bgorski It is not related to performance, but rather improving readability and consistency in the code. As you can see, the codebase was recently refactored to import namespaces. Specifically, all namespaces in this class were imported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, it also avoids the line length rule since the following line exceeds 120 characters:
public function getFileCache(array $cache, string $filePath, \Magento\Framework\Filesystem\DriverInterface $fileDriver): array
@magento run all tests |
if (null === $flattenResult) { | ||
$flattenResult = []; | ||
} | ||
$paramsKey = CRC32(json_encode($params ?? '')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$paramsKey = CRC32(json_encode($params ?? '')); | |
$paramsKey = empty($params) ? '2d06800538d394c2' : hash('xxh3', json_encode($params)); |
it shold be even better from perfromance point of view. see https://php.watch/versions/8.1/xxHash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because $params is expected to be an array, we could avoid to use the empty function and compare to [] or use !$params instead (it's fine tunning, it won't bring incredible improvement)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if #params is empty, you could return the result directly also, without hashing.
if (!$params) {
return $flattenResult ?? [];
}
@magento run all tests |
Could you please provide some absolute numbers? Is that like 50% of 10ms, or 50% of 200ms? |
@qsolutions-pl Is it true that it will add 100+MB's of memory? That's a lot of memory for config cache? How did you measure it? Also, since we are caching deployment config, does changing env.php require a config cache flush. Right now changes made in env.php work immidietly. "reduced the configuration files being read twice by the framework" Thank you in advance for contributing this. |
@Jakhotiya
|
@ihor-sviziev do you know why tests are failing? |
@magento run Database Compare, Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests, Static Tests, WebAPI Tests |
@magento run all tests |
Hi @qsolutions-pl, Thank you for your contribution! Did you get time to work on the changes. Thank you! |
I did not have time to make the changes I I want to rewrite the entire process the way DeploymentConfig is handled. |
Maybe best to change this to a draft PR until it's ready |
The problem is that the larger the config.php file, the bigger the gain in performance, but truth to be told, this code needs to be rewritten from scratch - imho. |
Hi @qsolutions-pl, Thank you for the reply. As per this information, let's close this PR for now, once you will ready with new implementation you can reopen the same or let us know we will reopen it again. Thank you! |
Hi @engcom-Charlie, wouldn't it be better to keep this open but mark it as a draft PR? This PR is highly upvoted and clearly many are interested in this. Chances are a newly created PR might not get the needed attention again. |
There is no need to create new PR for new implementation. @qsolutions-pl can simply continue on this PR in his branch and reopen whenever needed. Moving this to On hold since its already reopen. |
Description (*)
This PR reduces config load time by 50%,
but it adds +100MB to memory as I've added the file cache
What was completed in this PR:
Contribution checklist (*)
Resolved issues: