[Webkit-unassigned] [Bug 132057] CSS JIT: Backtracking with current matching element support for Child fragment.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 24 16:38:42 PDT 2014


https://bugs.webkit.org/show_bug.cgi?id=132057


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #230071|review?                     |review-
               Flag|                            |




--- Comment #15 from Benjamin Poulain <benjamin at webkit.org>  2014-04-24 16:39:01 PST ---
(From update of attachment 230071)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list