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.

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

Cache match elements to speed up compiler #1831

Merged
merged 2 commits into from
Feb 1, 2014
Merged

Conversation

oyejorge
Copy link
Contributor

No description provided.

@seven-phases-max
Copy link
Member

Btw, won't the caching break matching of interpolated selectors? I guess the "element expansion" should only be cached when selector contains no @{variable}s.

continue;
}

if( typeof v.value.value !== "string" ){
Copy link
Member

Choose a reason for hiding this comment

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

This make sense. But unfortunately there're too many possible value types beyond a string (e.g. an array, an expression etc.) so it still remains broken for "complex" variable values... Ideally we should use element.genCSS() instead of simple element.value expansion for interpolated things everywhere.


I was also thinking of something like that (i.e. caching + proper @{var} expansion) but I thought it would make sense to fix #1196 first which means this whole thing to be completely rewritten from scratch (because we will need to switch from the "element by element" to a "string/substring" matching method or a sort of).

@oyejorge
Copy link
Contributor Author

But unfortunately there're too many possible value types beyond a string ... Ideally we should use element.genCSS()

I looked into that as well but decided to go with this for a two reasons.

  1. The code this would replace doesn't factor in the different types of v.value.value:
oelements = other.elements.map( function(v) {
   return v.combinator.value + (v.value.value || v.value);
}).join("").match(/[,&#\.\w-]([\w-]|(\\.))*/g); 
  1. If v.value.value isn't a string, then it would be for something like the following and matching would be impossible anyways.
.class[a=1]{
   ...
}

Btw, won't the caching break matching of interpolated selectors?

Just checked and this compiles correctly:

@var: one;
.mixin-@{var}{
    width:10;
}
.test{
    .mixin-one;
}

@seven-phases-max
Copy link
Member

  1. The code this would replace doesn't factor in the different types of v.value.value:

Yes.

I mean situations like this:

@v1: .a;
@v2: .b .c;
@v3: @v1 @v2; 
@v4: ".d";

@{v1} {1: 1}
@{v2} {2: 2}
@{v3} {3: 3}
.e {@{v4} {4: 4}}

test {
    .a;       // OK
    .b .c;    // OK
    .a .b .c; // Err., no match (but ideally it should)
    .e .d;    // matches .e ".d", which is wrong of course,
              // and it still will match after these changes
}

I.e. the test for "string" type does not really improve anything.

Btw, won't the caching break matching of interpolated selectors?

I meant more tricky situations where the value of a variable used in selector changes in between mixin calls. Well, I did some testing and it seems that for the moment there's no way to achive anything like that, so indeed caching can't break anything. A code to illustrate what I mean:

.m(@v) {
    @{v} {value: @v}
}

.x {
    .m(~'.a');
    .m(~'.b');
}

test {
    .x .a;
    .x .b;
}

There .x .a and .x .b are independent rulesets cached separately so I guess caching is just fine there (I was afraid that the caching breaks .x .b).

I thought it would make sense to fix #1196 first

Sorry, wrong issue number, I meant #1270 and similar, i.e.:

@c: c;
.a {
    &-b    {}
    &-@{c} {}
}

test {
    .a-b; // error, undefined
    .a-c; // error, undefined
}

@seven-phases-max
Copy link
Member

Either way the performance improvement seems to be quite noticeable (it's 10-15% improvement for the less.benchmark which does not use too many mixin calls, so it should be even faster for things like Bootstrap). So personally I sympathize merging this.

if( !this._elements ){

len = this.elements.length;
for(i = 0; i < len; i++){
Copy link
Member

Choose a reason for hiding this comment

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

Btw, var i is not declared here (so grunt test fails).

@lukeapage lukeapage merged commit c425541 into less:master Feb 1, 2014
@lukeapage
Copy link
Member

okay then, merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants