Bug 135733 – CSS JIT: support :scope

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yusuke Suzuki]

Supporting :scope. It makes CheckingContext more similar to SelectorChecker::SelectorCheckingContext.

[Yusuke Suzuki]

Created attachment 236240 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 236240 [details]
Patch

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

Added comments.

> Source/WebCore/cssjit/SelectorCompiler.cpp:528
> +        return FunctionType::SelectorCheckerWithCheckingContext;

When the context is QuerySelecor, it requires CheckingContext. Otherwise, :scope works as :root since context.scope is nullptr.

> Source/WebCore/cssjit/SelectorCompiler.cpp:-1629
> -    RELEASE_ASSERT(m_selectorContext == SelectorContext::RuleCollector);

Now, QueryingRules can accept CheckingContext.

> Source/WebCore/dom/SelectorQuery.cpp:384
> +    checkingContext.scope = rootNode.isDocumentNode() ? nullptr : &rootNode;

searchRootNode may be filtered with id. So taking rootNode additionally and set it to context.scope.

[Yusuke Suzuki]

Ah, since :scope process is very small, CSS JIT can inline it. I'll submit the patch.

[Yusuke Suzuki]

Created attachment 236252 [details]
Patch

[Benjamin Poulain]

Comment on attachment 236252 [details]
Patch

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

Great patch. Some nitpick below:

> Source/WebCore/ChangeLog:17
> +        * cssjit/SelectorCompiler.cpp:

addFlagsToElementStyleFromContext() should get an ASSERT(m_selectorContext == SelectorContext::QuerySelector).

Maybe shouldUseRenderStyleFromCheckingContext() could get one too. It is not really incorrect to call that for QuerySelector but that is suspicious if that happen. You would need to refactor some test functions.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2971
> +    RELEASE_ASSERT(m_selectorContext == SelectorContext::QuerySelector);

This could be a regular assertion. Your code below handle the null case correctly if that were to happen.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2985
> +    Assembler::Jump successCase;
> +    {
> +        LocalRegister checkingContext(m_registerAllocator);
> +        loadCheckingContext(checkingContext);
> +
> +        Assembler::Jump scopeIsNull = m_assembler.branchTestPtr(Assembler::Zero, Assembler::Address(checkingContext, OBJECT_OFFSETOF(CheckingContext, scope)));
> +        successCase = m_assembler.branchPtr(Assembler::Equal, Assembler::Address(checkingContext, OBJECT_OFFSETOF(CheckingContext, scope)), elementAddressRegister);
> +        failureCases.append(m_assembler.jump());
> +
> +        scopeIsNull.link(&m_assembler);
> +    }
> +    generateElementIsRoot(failureCases);
> +    successCase.link(&m_assembler);

Alternative idea to write this with fewer jumps:

1) Reserve register "scope".
2) Load "checkingContext, OBJECT_OFFSETOF(CheckingContext, scope)" into "scope".
3) Check for nullity, if null, jump to [5]
4) Load the root in scope with getDocument().
5) Compare scope and elementAddressRegister, fail if they are not equal.

Please put the update for review if you do this.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3002
>      // When fragment doesn't have a pseudo element, there's no need to mark the pseudo element style.
>      if (!fragment.pseudoElementSelector)
>          return;

Ooops, loadCheckingContext() should have been after that if() :)

> LayoutTests/ChangeLog:13
> +        * fast/selectors/querySelector-scope-filtered-root.html: Added.

Good idea to test that!

[Yusuke Suzuki]

Comment on attachment 236252 [details]
Patch

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

Thank you for your great review!

>> Source/WebCore/ChangeLog:17
>> +        * cssjit/SelectorCompiler.cpp:
> 
> addFlagsToElementStyleFromContext() should get an ASSERT(m_selectorContext == SelectorContext::QuerySelector).
> 
> Maybe shouldUseRenderStyleFromCheckingContext() could get one too. It is not really incorrect to call that for QuerySelector but that is suspicious if that happen. You would need to refactor some test functions.

Seeing the implementation, threre's a lot of `if (m_selectorContext == SelectorChecker::QuerySelector)` check on the function entry of the test functions. As the result, the content of the function is splitted into 2.
In this time, I've refactored them a little(1); Making the function's structure consistent like,

if (m_selectorContext == SelectorChecker::QuerySelector) {
   .... // content for QuerySelector
   return;    // return early
}
ASSERT(m_selectorContext == SelectorChecker::QuerySelector);
...  // content for RuleCollector


And inserting assertions.


But I think there's the more aggressive way(2) to do this;
Pushing selectorContext value into SelectorCodeGenerator's template variable (SelectorCodeGenerator<SelectorContext>) and split these test functions to `SelectorCodeGenerator<QuerySelector>::generateXXXTest` and `SelectorCodeGenerator<RuleCollector>::generateXXXTest`.
This can guarantee that SelectorContext is QuerySelector (vice versa) statically by the compiler.
Maybe this is a little bit radical change, in the following patch, I took (1). But if (2) is preferable, I'll open the new bug and fix it :)
What do you think about the plan (2)?

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2971
>> +    RELEASE_ASSERT(m_selectorContext == SelectorContext::QuerySelector);
> 
> This could be a regular assertion. Your code below handle the null case correctly if that were to happen.

Fixed!

>> Source/WebCore/cssjit/SelectorCompiler.cpp:2985
>> +    successCase.link(&m_assembler);
> 
> Alternative idea to write this with fewer jumps:
> 
> 1) Reserve register "scope".
> 2) Load "checkingContext, OBJECT_OFFSETOF(CheckingContext, scope)" into "scope".
> 3) Check for nullity, if null, jump to [5]
> 4) Load the root in scope with getDocument().
> 5) Compare scope and elementAddressRegister, fail if they are not equal.
> 
> Please put the update for review if you do this.

That's quite nice! I'll update and upload the patch for your review :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3002
>>          return;
> 
> Ooops, loadCheckingContext() should have been after that if() :)

Yeah!

[Yusuke Suzuki]

Created attachment 236323 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 236323 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:1345
> +    if (m_selectorContext != SelectorContext::QuerySelector && m_functionType == FunctionType::SelectorCheckerWithCheckingContext) {

Since it is clear that the pseudo element has no effect when the context is QuerySelector, I wrote this guard here.
Is it preferable to pushing this guard into `generateRequestedPseudoElementEqualsToSelectorPseudoElement`?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1592
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

Now `addFlagsToElementStyleFromContext` is called after ASSERT(m_selectorContext != SelectorContext::QuerySelector); assertion is passed in the caller side :)

[Yusuke Suzuki]

Comment on attachment 236323 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:3005
> +    failureCases.append(m_assembler.branchPtr(Assembler::NotEqual, scope, elementAddressRegister));

Quite simplified :)

[Benjamin Poulain]

(In reply to comment #6)
> But I think there's the more aggressive way(2) to do this;
> Pushing selectorContext value into SelectorCodeGenerator's template variable (SelectorCodeGenerator<SelectorContext>) and split these test functions to `SelectorCodeGenerator<QuerySelector>::generateXXXTest` and `SelectorCodeGenerator<RuleCollector>::generateXXXTest`.
> This can guarantee that SelectorContext is QuerySelector (vice versa) statically by the compiler.
> Maybe this is a little bit radical change, in the following patch, I took (1). But if (2) is preferable, I'll open the new bug and fix it :)
> What do you think about the plan (2)?

That seems reasonable but I am afraid this would hurt binary size if the whole compiler becomes a template. ARM CPUs have a tiny Instruction Cache and increasing the binary size tend to hurt a lot. In this code, branch performance is not a bottleneck, we cannot justify increases in binary size.

[Benjamin Poulain]

Comment on attachment 236323 [details]
Patch

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

Brilliant work as usual. Some minor comments below.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1345
>> +    if (m_selectorContext != SelectorContext::QuerySelector && m_functionType == FunctionType::SelectorCheckerWithCheckingContext) {
> 
> Since it is clear that the pseudo element has no effect when the context is QuerySelector, I wrote this guard here.
> Is it preferable to pushing this guard into `generateRequestedPseudoElementEqualsToSelectorPseudoElement`?

IMHO, this code is clear with this check...

> Source/WebCore/cssjit/SelectorCompiler.cpp:1398
> +    if (m_selectorContext != SelectorContext::QuerySelector && m_functionType == FunctionType::SelectorCheckerWithCheckingContext) {

...and it is consistent with the counterpart here.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1642
> +    ASSERT(m_functionType == FunctionType::SelectorCheckerWithCheckingContext);

A RELEASE_ASSERT() could be better here to guard any invalid access to m_checkingContextStackReference.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1827
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);
> +

In WebKit we generally avoid ASSERT that repeat a early return.

What you can do is use ASSERT_WITH_MESSAGE to explain the early return.
E.g.: ASSERT_WITH_MESSAGE(m_selectorContext != SelectorContext::QuerySelector, "We should never mark the tree from QuerySelector because that would cause useless style invalidation.")

...or you can remove this assertion.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1848
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);
> +

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2444
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2446
> +    if (shouldUseRenderStyleFromCheckingContext(m_selectorContext, fragment)) {

This actually looks quite a bit better with your early return.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2479
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

Same comment as above regarding the assertion.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2601
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2673
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2957
> +    ASSERT_WITH_MESSAGE(m_selectorContext != SelectorContext::QuerySelector, "When the fragment has pseudo element, the selector becomres CannotMatchAnything for QuerySelector and this test function is not called.");

Typo: "becomres"

[Yusuke Suzuki]

Comment on attachment 236323 [details]
Patch

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

Thank you for your review! So I'll land it manually with changes you pointed :)

>>> Source/WebCore/cssjit/SelectorCompiler.cpp:1345
>>> +    if (m_selectorContext != SelectorContext::QuerySelector && m_functionType == FunctionType::SelectorCheckerWithCheckingContext) {
>> 
>> Since it is clear that the pseudo element has no effect when the context is QuerySelector, I wrote this guard here.
>> Is it preferable to pushing this guard into `generateRequestedPseudoElementEqualsToSelectorPseudoElement`?
> 
> IMHO, this code is clear with this check...

OK, so I'll use this form :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1642
>> +    ASSERT(m_functionType == FunctionType::SelectorCheckerWithCheckingContext);
> 
> A RELEASE_ASSERT() could be better here to guard any invalid access to m_checkingContextStackReference.

Nice, I've changed it to RELEASE_ASSERT.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1827
>> +
> 
> In WebKit we generally avoid ASSERT that repeat a early return.
> 
> What you can do is use ASSERT_WITH_MESSAGE to explain the early return.
> E.g.: ASSERT_WITH_MESSAGE(m_selectorContext != SelectorContext::QuerySelector, "We should never mark the tree from QuerySelector because that would cause useless style invalidation.")
> 
> ...or you can remove this assertion.

Thank you for your pointing. So in this time, I've simply dropped it :)

[Yusuke Suzuki]

Committed r172408: <http://trac.webkit.org/changeset/172408>