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)
@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 |
@qsolutions-pl Could you explain those 100M? I'm wondering why it's that much I would expect less memory usage. I read your post on linkedin and you are talking about env.php and config.php files, but those files are clearly not that big. |
Hi @qsolutions-pl I did some tests. For a single store with a lot of configurations I see Magento being busy for 8ms or so when having the cache warmed and fpc and block cache disabled (which happens the most times), on a fast laptop CPU, so this could be a bit more gain on a server cpu. I added hrtime timers on the specific code pieces to measure how much time they were really costing. Is the gain mainly coming with config cache gone or disabled? |
@magento run Database Compare |
@qsolutions-pl looks like with your changes, magento requires DB connection while running any CLI command and because of that, magento can't be installed. |
@ihor-sviziev What makes you say its @qsolutions-pl changes that causes that? Does the code changes you see suggest a change to a reliance on the DB? (Thats not me being cherky, genuinely curious) When I pull down a fully clean version of latest, it seems to present the exact same issue, though its not all commands for example the consumer list and cache status/flush commands still function on clean and this pr. |
@magento create issue |
Just a small update on my end: |
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: