Bug 137947 – CSS JIT: Implement :matches

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

CSS JIT: Implement :matches

[Yusuke Suzuki]

Created attachment 240256 [details]
Patch

Added initial patch. I'll add the tests for heavy backtrackings

[Yusuke Suzuki]

Created attachment 240261 [details]
Patch

Added backtracking tests derived from Benjamin's not-backtracking test. Now it's ready for review.

[Yusuke Suzuki]

Comment on attachment 240261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240261&action=review

Added comments for reviewing.

> Source/WebCore/cssjit/SelectorCompiler.cpp:681
> +                functionType = mostRestrictiveFunctionType(functionType, localFunctionType);

Use `mostRestrictiveFunctionType` :)

> Source/WebCore/cssjit/SelectorCompiler.cpp:1465
> +        dataLogF("%*sComputing fragment[%d] backtracking height %u. NotMatched %u / Matched %u | width %u. NotMatched %u / Matched %u\n", level * 4, "", i, fragment.heightFromDescendant, fragment.tagNameNotMatchedBacktrackingStartHeightFromDescendant, fragment.tagNameMatchedBacktrackingStartHeightFromDescendant, fragment.widthFromIndirectAdjacent, fragment.tagNameNotMatchedBacktrackingStartWidthFromIndirectAdjacent, fragment.tagNameMatchedBacktrackingStartWidthFromIndirectAdjacent);

Use `"%*s"` for indentation. Is it acceptable?

> Source/WebCore/cssjit/SelectorCompiler.cpp:3548
> +        generateElementMatchesSelectorList(failureCases, elementAddressRegister, matchesList);

Leveraging generalized backtracking, we can implement :matches with pretty clean code! Yay!

> LayoutTests/ChangeLog:9
> +        * fast/selectors/matches-backtracking.html: Added.

Based on not-backtracking tests with extensions.

[Benjamin Poulain]

Comment on attachment 240261 [details]
Patch

Quick comment:

I haven't looked at the code yet but I can think of two other kind of tests that are useful for the JIT:
-Chaining many :matches(). E.g. :matches(.a):matches(.b):matches(.c):matches(.d) etc. This catches problems with register allocation leaking registers.
-Nesting many :matches(). E.g. :matches(:matches(:matches(.a), :matches(.b)), :matches(:matches([c=d]), :matches([e=f][g=h]))), etc. When used with various register allocation pattern, it can find cases were we do not allocate enough registers in the prologue.

[Yusuke Suzuki]

(In reply to comment #4)
> Comment on attachment 240261 [details]
> Patch
> 
> Quick comment:
> 
> I haven't looked at the code yet but I can think of two other kind of tests
> that are useful for the JIT:
> -Chaining many :matches(). E.g.
> :matches(.a):matches(.b):matches(.c):matches(.d) etc. This catches problems
> with register allocation leaking registers.
> -Nesting many :matches(). E.g. :matches(:matches(:matches(.a),
> :matches(.b)), :matches(:matches([c=d]), :matches([e=f][g=h]))), etc. When
> used with various register allocation pattern, it can find cases were we do
> not allocate enough registers in the prologue.

Sounds nice! I'll add them.

[Yusuke Suzuki]

Created attachment 240321 [details]
Patch

[Benjamin Poulain]

Comment on attachment 240261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240261&action=review

The patch looks great. I r- just to get a chance to see the other tests before landing.

> Source/WebCore/ChangeLog:8
> +        Initial patch for supporting :matchs in CSS JIT.

typo: :matchs

>> Source/WebCore/cssjit/SelectorCompiler.cpp:681
>> +                functionType = mostRestrictiveFunctionType(functionType, localFunctionType);
> 
> Use `mostRestrictiveFunctionType` :)

Oooops, I messed up :)

> Source/WebCore/cssjit/SelectorCompiler.cpp:757
> +                // Since this fragment always unmatch against the element, don't insert it to matchesList.

"always unmatch" -> "never matches"

> Source/WebCore/cssjit/SelectorCompiler.cpp:769
> +            // Since all selector list in :matches() cannot match anything, the whole :matches() filter cannot match anything.
> +            if (matchesList.isEmpty())

Interesting case, do we have tests for it?

> Source/WebCore/cssjit/SelectorCompiler.cpp:772
> +            ASSERT(!matchesList.isEmpty());

I don't think this assertion has a lot of value here because the same branch exists above.

It could be interesting to move it inside generateElementMatchesMatchesPseudoClass().

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1465
>> +        dataLogF("%*sComputing fragment[%d] backtracking height %u. NotMatched %u / Matched %u | width %u. NotMatched %u / Matched %u\n", level * 4, "", i, fragment.heightFromDescendant, fragment.tagNameNotMatchedBacktrackingStartHeightFromDescendant, fragment.tagNameMatchedBacktrackingStartHeightFromDescendant, fragment.widthFromIndirectAdjacent, fragment.tagNameNotMatchedBacktrackingStartWidthFromIndirectAdjacent, fragment.tagNameMatchedBacktrackingStartWidthFromIndirectAdjacent);
> 
> Use `"%*s"` for indentation. Is it acceptable?

I am not sure that works with NSLog. Let's try, if that's a problem I'll repatch.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1522
>      for (SelectorFragment& fragment : selectorFragments) {

This is getting busy. We should probably put the 3 branches in the same for() loop.

Any opinion?

>> LayoutTests/ChangeLog:9
>> +        * fast/selectors/matches-backtracking.html: Added.
> 
> Based on not-backtracking tests with extensions.

I can think of one more boundary test: having *a lot* of selector in the selector list of :matches().

> LayoutTests/fast/selectors/matches-backtracking.html:109
> +description('Check the basic features of the ":not(selectorList)" pseudo class.');

Wrong description here.

[Benjamin Poulain]

Comment on attachment 240321 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240321&action=review

Let's do it! r+ with the minor comments from the previous version.

> LayoutTests/ChangeLog:12
> +        * fast/selectors/matches-compltex-expected.txt: Added.
> +        * fast/selectors/matches-compltex.html: Added.

Typo: compltex?

> LayoutTests/fast/selectors/matches-backtracking.html:109
> +description('Check the basic features of the ":matches(selectorList)" pseudo class.');

Probably a basic feature for you but the readers might disagree :)

Let's say: "Check backtracking with :matches(selectorList)" or something like that :)

> LayoutTests/fast/selectors/matches-compltex.html:171
> +// // Multiple nested :matches.

Double comments here.

[Yusuke Suzuki]

Comment on attachment 240261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240261&action=review

>> Source/WebCore/cssjit/SelectorCompiler.cpp:757
>> +                // Since this fragment always unmatch against the element, don't insert it to matchesList.
> 
> "always unmatch" -> "never matches"

Done.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:769
>> +            if (matchesList.isEmpty())
> 
> Interesting case, do we have tests for it?

Added test cases for it :) such as `:matches(:visited)` on `querySelector`.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:772
>> +            ASSERT(!matchesList.isEmpty());
> 
> I don't think this assertion has a lot of value here because the same branch exists above.
> 
> It could be interesting to move it inside generateElementMatchesMatchesPseudoClass().

Yeah, I've moved it.

>>> Source/WebCore/cssjit/SelectorCompiler.cpp:1465
>>> +        dataLogF("%*sComputing fragment[%d] backtracking height %u. NotMatched %u / Matched %u | width %u. NotMatched %u / Matched %u\n", level * 4, "", i, fragment.heightFromDescendant, fragment.tagNameNotMatchedBacktrackingStartHeightFromDescendant, fragment.tagNameMatchedBacktrackingStartHeightFromDescendant, fragment.widthFromIndirectAdjacent, fragment.tagNameNotMatchedBacktrackingStartWidthFromIndirectAdjacent, fragment.tagNameMatchedBacktrackingStartWidthFromIndirectAdjacent);
>> 
>> Use `"%*s"` for indentation. Is it acceptable?
> 
> I am not sure that works with NSLog. Let's try, if that's a problem I'll repatch.

OK. At least, I made sure that `printf` spec of C99 allows it.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1522
>>      for (SelectorFragment& fragment : selectorFragments) {
> 
> This is getting busy. We should probably put the 3 branches in the same for() loop.
> 
> Any opinion?

Agreed. I think combining these 3 branches into 1 doesn't hurt the readability. So I'll fix it :)

>>> LayoutTests/ChangeLog:9
>>> +        * fast/selectors/matches-backtracking.html: Added.
>> 
>> Based on not-backtracking tests with extensions.
> 
> I can think of one more boundary test: having *a lot* of selector in the selector list of :matches().

OK, I'll add it :)

>> LayoutTests/fast/selectors/matches-backtracking.html:109
>> +description('Check the basic features of the ":not(selectorList)" pseudo class.');
> 
> Wrong description here.

I've updated it with "Check backtracking with :matches(selectorList)." :)

[Yusuke Suzuki]

Comment on attachment 240321 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240321&action=review

>> LayoutTests/ChangeLog:12
>> +        * fast/selectors/matches-compltex.html: Added.
> 
> Typo: compltex?

Oops. Thank you for your pointing.

>> LayoutTests/fast/selectors/matches-compltex.html:171
>> +// // Multiple nested :matches.
> 
> Double comments here.

Oops, fixed :)

[Yusuke Suzuki]

Committed r175097: <http://trac.webkit.org/changeset/175097>