CSS JIT: Implement :matches
Created attachment 240256 [details] Patch Added initial patch. I'll add the tests for heavy backtrackings
Created attachment 240261 [details] Patch Added backtracking tests derived from Benjamin's not-backtracking test. Now it's ready for review.
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.
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.
(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.
Created attachment 240321 [details] Patch
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.
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.
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)." :)
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 :)
Committed r175097: <http://trac.webkit.org/changeset/175097>