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 (+4% to speed in Magento 2.3.2) #14

Merged
merged 6 commits into from
Mar 19, 2020
Merged

Performance improvements (+4% to speed in Magento 2.3.2) #14

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8812fd2
Performance improvements (+4% to speed in Magento2)
andrey-legayev Sep 29, 2019
7c96c52
Merge branch 'master' into perf-fixes
jdforrester Nov 15, 2019
35c64d0
Update Extend.php
Krinkle Mar 19, 2020
46168f6
Update processExtends.php
Krinkle Mar 19, 2020
5bfa347
Update index.php
Krinkle Mar 19, 2020
0ebab41
Merge branch 'master' into perf-fixes
Krinkle Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions lib/Less/Tree/Extend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,13 @@ public function __construct($selector, $option, $index){
break;
}

$this->object_id = $i++;
$this->parent_ids = array($this->object_id);
// This must use a string (instead of int) so that array_merge()
// preserves keys on arrays that use IDs in their keys.
$this->object_id = 'id_' . $i++;

$this->parent_ids = array(
$this->object_id => true
);
}

public function accept( $visitor ){
Expand Down Expand Up @@ -74,4 +79,4 @@ public function findSelfSelectors( $selectors ){
$this->selfSelectors = array(new Less_Tree_Selector($selfElements));
}

}
}
5 changes: 5 additions & 0 deletions lib/Less/Tree/Selector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class Less_Tree_Selector extends Less_Tree{
public $elements_len = 0;

public $_oelements;
public $_oelements_assoc;
public $_oelements_len;
public $cacheable = true;

Expand Down Expand Up @@ -83,6 +84,8 @@ public function match( $other ){
public function CacheElements(){

$this->_oelements = array();
$this->_oelements_assoc = array();

$css = '';

foreach($this->elements as $v){
Expand All @@ -108,6 +111,8 @@ public function CacheElements(){
array_shift($this->_oelements);
$this->_oelements_len--;
}

$this->_oelements_assoc = array_fill_keys($this->_oelements, true);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to analyze algorithm to replace _oelements with associative array, but it's quite complicated and I've decided not to do that.

Copy link
Member

Choose a reason for hiding this comment

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

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).

}
}

Expand Down
10 changes: 6 additions & 4 deletions lib/Less/Visitor/processExtends.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,9 @@ private function doExtendChaining( $extendsList, $extendsListTarget, $iterationC
$extend = $extendsList[$extendIndex];
$targetExtend = $extendsListTarget[$targetExtendIndex];

// look for circular references
if( in_array($targetExtend->object_id, $extend->parent_ids,true) ){
// Optimisation: Explicit reference, <https://github.com/wikimedia/less.php/pull/14>
if( \array_key_exists($targetExtend->object_id, $extend->parent_ids) ){
// ignore circular references
continue;
}

Expand Down Expand Up @@ -269,7 +270,8 @@ private function HasMatches($extend, $haystackSelectorPath){
return true;
}

if( in_array($first_el, $hackstackSelector->_oelements) ){
// Optimisation: Explicit reference, <https://github.com/wikimedia/less.php/pull/14>
if( \array_key_exists($first_el, $hackstackSelector->_oelements_assoc) ){
return true;
}
}
Expand Down Expand Up @@ -466,4 +468,4 @@ protected function visitDirectiveOut(){
array_pop($this->allExtendsStack);
}

}
}
2 changes: 1 addition & 1 deletion test/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ function obj($mixed, $objects = array() ) {


$exclude_keys = array('originalRuleset','currentFileInfo','lookups','index','ruleset_id','type','_rulesets','_variables','allowImports','_css','cache_string','elements_len',
'_oelements','first_oelements','_oelements_len','cacheable', ); //'variable','combinator'
'_oelements','_oelements_assoc','first_oelements','_oelements_len','cacheable', ); //'variable','combinator'
//$exclude_keys = array();

$type = gettype($mixed);
Expand Down