Performance improvements (+4% to speed in Magento 2.3.2) #14
Conversation
andrey-legayev
force-pushed
the
perf-fixes
branch
from
a557c88
to
8812fd2
Compare
| @@ -108,6 +111,8 @@ public function CacheElements(){ | |||
| array_shift($this->_oelements); | |||
| $this->_oelements_len--; | |||
| } | |||
|
|
|||
| $this->_oelements_assoc = array_fill_keys($this->_oelements, true); | |||
There was a problem hiding this comment.
Will need to review where and how _oelements_len and _oelements are used. If they are not used anywhere else, and (especially) not cached or filled in by caches anywhere, then we might be be able to instead make this the normal form of _oelements itself.
There was a problem hiding this comment.
I tried to analyze algorithm to replace _oelements with associative array, but it's quite complicated and I've decided not to do that.
There was a problem hiding this comment.
Thanks. I've had a more in-depth review today, and I better understnad what you mean now.
I mistakenly thought that the array was generated elsewhere, perhaps by pushing into the array in a loop or in repsonse to other signals. But, that is not the case. It is generated right here a few lines up from preg_match_all. This means even if we did not keep a sequential list, we would still need to iterate the list (directly or via array_fill_keys). It is inevitable (without much more changes).
lib/Less/Tree/Extend.php
Outdated
| @@ -42,8 +42,10 @@ public function __construct($selector, $option, $index){ | |||
| break; | |||
| } | |||
|
|
|||
| $this->object_id = $i++; | |||
| $this->parent_ids = array($this->object_id); | |||
| // string value is needed to make sure array_merge() processes keys as strings, not as numerics | |||
There was a problem hiding this comment.
Will need to look for where this is used. Might make sense to migrate to the + operator which can at times be faster to do this kind of operation for associative arrays.
lib/Less/Visitor/processExtends.php
Outdated
| @@ -54,7 +54,7 @@ private function doExtendChaining( $extendsList, $extendsListTarget, $iterationC | |||
| $targetExtend = $extendsListTarget[$targetExtendIndex]; | |||
|
|
|||
| // look for circular references | |||
| if( in_array($targetExtend->object_id, $extend->parent_ids,true) ){ | |||
| if (isset($extend->parent_ids[$targetExtend->object_id])) { | |||
There was a problem hiding this comment.
Worth comparing to array_key_exists. Although counterintuitively, it seems isset() despite doing more work (exists + non-null), has historically been faster. Not sure if that's still the case in PHP 7.2+.
There was a problem hiding this comment.
With PHP 7.4, array_key_exists is now faster: php/php-src#3360, though only when statically resolved: php/php-src@f1c0e67.
|
Thanks @andrey-legayev. Much appreciated. I've filed https://phabricator.wikimedia.org/T234270 internally to track review of this. This Less.php fork exists mainly to keep the ship afloat to host any fixes for incompatibilities or warnings we found during Wikipedia's migration to PHP 7.2. It doesn't currently have a long-term steward and is still marked as an "upstream" package, however that may need to change at some point - although we're short on resources in this area so that might take a while to get through. Thanks again! |
|
thanks for processing it! |
Hi,
I've run xdebug profiler for Magento2 static content deploy and found huge amount of calls to in_array() inside Less processor.
I've tried to optimize it and got ~4% performance improvement.
Pls review my changes.
Andrey