Performance improvements #38642
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.
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.
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.
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.
What would be the benefit, though?
There was a problem hiding this comment.
@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.
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
m2-community-project
bot
moved this from Review in Progress
to Pending Review
in Pull Requests Dashboard
m2-community-project
bot
added
Progress: pending review
and removed
Progress: review
labels
|
@magento run all tests |
| if (null === $flattenResult) { | ||
| $flattenResult = []; | ||
| } | ||
| $paramsKey = CRC32(json_encode($params ?? '')); |
There was a problem hiding this comment.
| $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.
thanks to this, we get 10% improvement
Great tip, thanks!
There was a problem hiding this comment.
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)
m2-community-project
bot
moved this from Pending Review
to Review in Progress
in Pull Requests Dashboard
m2-community-project
bot
added
Progress: review
and removed
Progress: pending review
labels
|
@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 |
|
I've implemented this on 1 production site, no issues, and here are the results:
|
|
@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 |
engcom-Charlie
added
the
Priority: P2
|
Just a small update on my end: |
m2-community-project
bot
moved this from Review in Progress
to Changes Requested
in Pull Requests Dashboard
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: