-
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
Changes from 2 commits
4582164
620f8b0
1b301f6
1246ab6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,9 @@ class DeploymentConfig | |
*/ | ||
private $readerLoad = []; | ||
|
||
/** @var array|null */ | ||
private ?array $flattenParams = null; | ||
|
||
/** | ||
* Constructor | ||
* | ||
|
@@ -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 ?? '')); | ||
|
||
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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be improved with |
||
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] ?? []; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears that the ̶I̶t̶ ̶a̶p̶p̶e̶a̶r̶s̶ ̶t̶h̶a̶t̶ ̶t̶h̶e̶ ̶ |
||
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; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be beneficial if all namespaces were imported: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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:
|
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 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.
thanks to this, we get 10% improvement
Great tip, thanks!
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.