[webkit-reviews] review denied: [Bug 132057] CSS JIT: Backtracking with current matching element support for Child fragment. : [Attachment 230071] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 132057: CSS JIT: Backtracking with current matching element support for
Child fragment.
https://bugs.webkit.org/show_bug.cgi?id=132057

Attachment 230071: Patch
https://bugs.webkit.org/attachment.cgi?id=230071&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=230071&action=review


Amazing start. This is a great patch. Some comments bellow:

> Source/WebCore/cssjit/SelectorCompiler.cpp:114
> +static const unsigned InvalidHeight = std::numeric_limits<unsigned>::max();

We do not capitalize the first letter of constant in WebKit.

> Source/WebCore/cssjit/SelectorCompiler.cpp:120
> +	   ,
matchingOtherBacktrackingAction(BacktrackingAction::NoBacktracking)

maybe matchingOtherBacktrackingAction->matchingPostTagNameBacktrackingAction?
To make it more explicit that tag name must go first.

> Source/WebCore/cssjit/SelectorCompiler.cpp:177
> +    void
generateWalkToParentElementWithoutElementCheck(Assembler::RegisterID
targetRegister);

Without the element check, that would simply be generateWalkToParentNode().

> Source/WebCore/cssjit/SelectorCompiler.cpp:680
> +enum TagNameEquality {

Let's use a typed enum with "enum class".

> Source/WebCore/cssjit/SelectorCompiler.cpp:702
> +	   // Since rhs & lhs have actual LocalName,
> +	   // inverted lhs never matches on rhs.

This comment could be on a single line.

I believe this comment is useful but it should be in equalTagNamePatterns(). In
equalTagNames(), we do not have the context of inverted/non-inverted.

> Source/WebCore/cssjit/SelectorCompiler.cpp:712
> +	   // Since rhs & lhs have actual NamespaceURI,
> +	   // inverted lhs never matches on rhs.

Ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:731
> +static inline unsigned computeBacktrackingStartHeightFromDescendant(const
TagNameList& tagNames)

This function could have a comment explaining it finds the largest matching
prefix.

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

You can use RELEASE_ASSERT here.

I try to be very aggressive with release assertion in this compiler to avoid
any security issue. We can afford it because we compile very little compared to
how many time the compiled code is executed.

> Source/WebCore/cssjit/SelectorCompiler.cpp:735
> +    unsigned size = tagNames.size();

size -> subsetSize?
largestPrefixSize?

> Source/WebCore/cssjit/SelectorCompiler.cpp:744
> +	   unsigned amount = tagNames.size() - size;
> +	   bool matched = true;
> +	   for (unsigned i = 0; i < size; ++i) {
> +	       if (!equalTagNamePatterns(tagNames[(tagNames.size() - 1 - i)],
tagNames[(tagNames.size() - 1 - i) - amount].tagName)) {
> +		   matched = false;
> +		   break;
> +	       }
> +	   }

It would be nice to simplify this loop somehow.

Maybe rename amount to "offsetToLargestPrefix"?
Put "tagNames.size() - 1" in a temporary "lastIndex"?

It would be nice to put "tagNames.size() - 1 - i" and "(tagNames.size() - 1 -
i) - amount" in named temporary too. Not sure what would be a good name.

> Source/WebCore/cssjit/SelectorCompiler.cpp:752
> +static inline void computeBacktrackingHeightFromDescendant(SelectorFragment&
fragment, TagNameList& tagNames, bool hasDescendantRelationOnTheRight, const
SelectorFragment*& previousChildFragment)
> +{

I think this function could start with

if (!hasDescendantRelationOnTheRight)
    return;

The descendant backtracking height is only relevant inside backtracking chains,
and a chain can only start with a descendant.


Maybe rename previousChildFragment to previousChildFragmentInBacktrackingChain
or previousChildFragmentInDescendantBacktrackingChain?

> Source/WebCore/cssjit/SelectorCompiler.cpp:753
> +    TagNamePattern pattern;

Let's make 2 local declaration in the block where it is used.

> Source/WebCore/cssjit/SelectorCompiler.cpp:765
> +	       // tagName is not matched

WebKit comments should be sentences, you need the period at the end.

> Source/WebCore/cssjit/SelectorCompiler.cpp:770
> +	   // tagName is matched

Ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:838
> -	       fragment.backtrackingFlags |=
BacktrackingFlag::SaveDescendantBacktrackingStart;
> +	       saveDescendantBacktrackingStartFragmentIndex = i;

Here we could have an ASSERT(saveDescendantBacktrackingStartFragmentIndex ==
std::numeric_limits<unsigned>::max()).

> Source/WebCore/cssjit/SelectorCompiler.cpp:1008
>      Assembler::JumpList tagMatchingLocalFailureCases;
> -    generateElementMatching(tagMatchingLocalFailureCases, fragment);
> +    generateElementMatching(tagMatchingLocalFailureCases,
tagMatchingLocalFailureCases, fragment);
>      tagMatchingLocalFailureCases.linkTo(loopStart, &m_assembler);

Let's rename tagMatchingLocalFailureCases to matchingFailureCases in this case.