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.

Sign up for GitHub

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

Jump to bottom

Performance improvements #38642

Open
wants to merge 4 commits into
base: 2.4-develop
Choose a base branch
from
Open

Performance improvements #38642

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4582164
performance updates - array merge
qsolutions-pl Apr 19, 2024
620f8b0
refactor 2.4.7, removing duplicated code
qsolutions-pl Apr 19, 2024
1b301f6
Merge branch '2.4-develop' into performance-improvements
qsolutions-pl Apr 20, 2024
1246ab6
Merge branch '2.4-develop' into performance-improvements
engcom-Hotel May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 32 additions & 23 deletions lib/internal/Magento/Framework/App/DeploymentConfig.php
View file
Open in desktop
Expand Up @@ -61,6 +61,9 @@ class DeploymentConfig
*/
private $readerLoad = [];

/** @var array|null */
private ?array $flattenParams = null;

/**
* Constructor
*
Expand Down Expand Up @@ -247,37 +250,43 @@ private function getAllEnvOverrides(): array
*/
private function flattenParams(array $params, ?string $path = null, array &$flattenResult = null): array
{
if (null === $flattenResult) {
$flattenResult = [];
}
$paramsKey = CRC32(json_encode($params ?? ''));
Copy link
Contributor

@kandy kandy Apr 19, 2024 •
edited

Choose a reason for hiding this comment

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

Suggested change
$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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
thanks to this, we get 10% improvement

Great tip, thanks!

Copy link
Member

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)

Copy link
Contributor

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 ?? [];
}


foreach ($params as $key => $param) {
if ($path) {
$newPath = $path . '/' . $key;
} else {
$newPath = $key;
}
if (isset($flattenResult[$newPath])) {
//phpcs:ignore Magento2.Exceptions.DirectThrow
throw new RuntimeException(new Phrase("Key collision '%1' is already defined.", [$newPath]));
if (!isset($this->flattenParams[$paramsKey])) {
if (null === $flattenResult) {
$flattenResult = [];
}

if (is_array($param)) {
$flattenResult[$newPath] = $param;
$this->flattenParams($param, $newPath, $flattenResult);
} else {
// allow reading values from env variables
// value need to be specified in %env(NAME, "default value")% format
// like #env(DB_PASSWORD), #env(DB_NAME, "test")
if ($param !== null && preg_match(self::ENV_NAME_PATTERN, $param, $matches)) {
$param = getenv($matches['name']) ?: ($matches['default'] ?? null);
foreach ($params as $key => $param) {
if ($path) {
$newPath = $path . '/' . $key;
} else {
$newPath = $key;
}
Copy link
Member

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;

if (isset($flattenResult[$newPath])) {
//phpcs:ignore Magento2.Exceptions.DirectThrow
throw new RuntimeException(new Phrase("Key collision '%1' is already defined.", [$newPath]));
}

$flattenResult[$newPath] = $param;
if (is_array($param)) {
$flattenResult[$newPath] = $param;
$this->flattenParams($param, $newPath, $flattenResult);
} else {
// allow reading values from env variables
// value need to be specified in %env(NAME, "default value")% format
// like #env(DB_PASSWORD), #env(DB_NAME, "test")
if ($param !== null && preg_match(self::ENV_NAME_PATTERN, $param, $matches)) {
$param = getenv($matches['name']) ?: ($matches['default'] ?? null);
}

$flattenResult[$newPath] = $param;
}
}

$this->flattenParams[$paramsKey] = $flattenResult;
}

return $flattenResult;
return $this->flattenParams[$paramsKey] ?? [];
}

/**
Expand Down
62 changes: 36 additions & 26 deletions lib/internal/Magento/Framework/App/DeploymentConfig/Reader.php
View file
Open in desktop
Expand Up @@ -81,47 +81,57 @@ public function getFiles()
}

/**
* Method loads merged configuration within all configuration files.
* To retrieve specific file configuration, use FileReader.
* $fileKey option is deprecated since version 2.2.0.
*
* @param string $fileKey The file key (deprecated)
* @return array
* @throws FileSystemException If file can not be read
* @throws RuntimeException If file is invalid
* @throws \Exception If file key is not correct
* @see FileReader
* @param $fileKey
* @return array|mixed
* @throws \Magento\Framework\Exception\FileSystemException
* @throws \Magento\Framework\Exception\RuntimeException
*/
public function load($fileKey = null)
{
static $cache = [];

$path = $this->dirList->getPath(DirectoryList::CONFIG);
$fileDriver = $this->driverPool->getDriver(DriverPool::FILE);
$result = [];

if ($fileKey) {
$filePath = $path . '/' . $this->configFilePool->getPath($fileKey);
if ($fileDriver->isExists($filePath)) {
$result = include $filePath;
if (!is_array($result)) {
throw new RuntimeException(new Phrase("Invalid configuration file: '%1'", [$filePath]));
}
}
$cache = $this->getFileCache($cache, $filePath, $fileDriver);
$result = $cache[$filePath] ?: [];
} else {
$configFiles = $this->getFiles();
foreach ($configFiles as $file) {
$configFile = $path . '/' . $file;
if ($fileDriver->isExists($configFile)) {
$fileData = include $configFile;
if (!is_array($fileData)) {
throw new RuntimeException(new Phrase("Invalid configuration file: '%1'", [$configFile]));
}
} else {
continue;
}
if ($fileData) {
$result = array_replace_recursive($result, $fileData);
$cache = $this->getFileCache($cache, $configFile, $fileDriver);
if ($cache[$configFile]) {
$result = array_replace_recursive($result, $cache[$configFile]);
}
}
}
return $result ?: [];
}

/**
* @param array $cache
* @param string $filePath
* @param \Magento\Framework\Filesystem\DriverInterface $fileDriver
* @return array
* @throws \Magento\Framework\Exception\FileSystemException
* @throws \Magento\Framework\Exception\RuntimeException
*/
public function getFileCache(array $cache, string $filePath, \Magento\Framework\Filesystem\DriverInterface $fileDriver): array
{
Copy link
Member

@TuVanDev TuVanDev Apr 19, 2024 •
edited

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.

if (!isset($cache[$filePath])) {
if ($fileDriver->isExists($filePath)) {
$fileData = include $filePath;
if (!is_array($fileData)) {
throw new RuntimeException(new Phrase("Invalid configuration file: '%1'", [$filePath]));
}
$cache[$filePath] = $fileData;
} else {
$cache[$filePath] = null;
}
}
return $cache;
}
}
Copy link
Member

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

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Member

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

9 changes: 3 additions & 6 deletions lib/internal/Magento/Framework/Console/Cli.php
View file
Open in desktop
Expand Up @@ -148,19 +148,16 @@ protected function getApplicationCommands()
try {
if (class_exists(\Magento\Setup\Console\CommandList::class)) {
$setupCommandList = new \Magento\Setup\Console\CommandList($this->serviceManager);
$commands = array_merge($commands, $setupCommandList->getCommands());
$commands = [...$commands, ...$setupCommandList->getCommands()];
}

if ($this->objectManager->get(DeploymentConfig::class)->isAvailable()) {
/** @var CommandListInterface */
$commandList = $this->objectManager->create(CommandListInterface::class);
$commands = array_merge($commands, $commandList->getCommands());
$commands = [...$commands, ...$commandList->getCommands()];
}

$commands = array_merge(
$commands,
$this->getVendorCommands($this->objectManager)
);
$commands = [...$commands, ...$this->getVendorCommands($this->objectManager)];
} catch (\Exception $e) {
$this->initException = $e;
}
Expand Down