CSS selector JIT compilation support for :lang()

• Component: New Bugs
• Status: RESOLVED FIXED

[Dhi Aurrahman]

CSS selector JIT compilation support for :lang()

[Dhi Aurrahman]

Created attachment 245214 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 245214 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:786
> +            if (argumentList->size() == 1 && argumentList[0].isEmpty())

I'm still trying to figure out other conditions to reject the input range when the argumentList has only one language range.

[Benjamin Poulain]

Comment on attachment 245214 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests, no behavior changed.

There are no new behavior, but there are new things to test:

First, combining multiple language selector. For example:
     :lang(fr, es):lang(fr, en):lang(fr)

Then there is testing that the generator does not leak registers or stack when chaining many values. For example:
     :lang(fr, es):lang(fr, es):lang(fr, es):lang(fr, es):lang(fr, es):lang(fr, es):lang(fr, es) (times 30 for example).
With that kind of test, if there is a leak, it will crash.

> Source/WebCore/cssjit/SelectorCompiler.cpp:788
> +            fragment.langArgumentList = argumentList;

I don't think that's quite correct.

Something like:
    :lang(fr):lang(en)
should not match anything, while here it would match the last :lang(). (We should have tests for that).

> Source/WebCore/cssjit/SelectorCompiler.cpp:3222
> +#if ENABLE(CSS_SELECTORS_LEVEL4)
> +void SelectorCodeGenerator::generateElementIsInLanguage(Assembler::JumpList& failureCases, const Vector<LanguageArgument>* langArgumentList)

Feel free to duplicate the whole code to have less #ifdef.

We'll get rid of the old code eventually.

[Dhi Aurrahman]

Comment on attachment 245214 [details]
Patch

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

>> Source/WebCore/cssjit/SelectorCompiler.cpp:788
>> +            fragment.langArgumentList = argumentList;
> 
> I don't think that's quite correct.
> 
> Something like:
>     :lang(fr):lang(en)
> should not match anything, while here it would match the last :lang(). (We should have tests for that).

I still don't get this one. Do you mean we could have something like querySelectorAll(':lang(fr):lang(en)') ? Is it the same as: :lang('en', 'fr')?

[Benjamin Poulain]

(In reply to comment #4)
> Comment on attachment 245214 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245214&action=review
> 
> >> Source/WebCore/cssjit/SelectorCompiler.cpp:788
> >> +            fragment.langArgumentList = argumentList;
> > 
> > I don't think that's quite correct.
> > 
> > Something like:
> >     :lang(fr):lang(en)
> > should not match anything, while here it would match the last :lang(). (We should have tests for that).
> 
> I still don't get this one. Do you mean we could have something like
> querySelectorAll(':lang(fr):lang(en)') ? Is it the same as: :lang('en',
> 'fr')?

It is a valid selector, but it would never match since no element can have both languages simultaneously.

You could have ':lang(fr):lang(fr-be)'. That is valid and effectively matches 'fr-be'.

[Dhi Aurrahman]

Created attachment 246248 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 246248 [details]
Patch

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

After a couple of failed attempts on supporting multiple arguments inside the selector compiler, I decided to push the single-argument support first.

> LayoutTests/fast/selectors/lang-multiple-chained.html:26
> +      shouldBe('document.querySelectorAll(":lang(en, de):lang(en, es):lang(en, fr)").length', '1');

I'm not sure this one is right. Should it be 3?

[Benjamin Poulain]

Comment on attachment 246248 [details]
Patch

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

Let's work this differently: first let's compile everything, then add the optimizations one by one.

> Source/WebCore/cssjit/SelectorCompiler.cpp:173
> +    const Vector<LanguageArgument>* languageArguments = nullptr;

The simplest way to match multiple :lang() would be to store them as Vector<Vector<LanguageArgument>*>. When generating, you can simply go over each one and generate the assembly for that Vector<LanguageArgument>.

There will be overlap, but we can deal with that separately. We might not even be able to optimize for that anymore.

> Source/WebCore/cssjit/SelectorCompiler.cpp:786
> +                const AtomicString& argument = selectorLangArgumentList->at(0).languageRange;
> +                const AtomicString& fragmentLanguageFilter = fragment.languageArguments->at(0).languageRange;
> +                if (fragmentLanguageFilter != argument) {
> +                    if (argument.startsWith(fragmentLanguageFilter, false))
> +                        fragment.languageArguments = selectorLangArgumentList;
> +                    else if (fragmentLanguageFilter.startsWith(argument, false))
> +                        { }
> +                    else
> +                        return FunctionType::CannotMatchAnything;
> +                }

Let's deal with that later.

Even without any optimization, just doing a function call in the CSS JIT helps a great deal. If any part of a selector is not handle by the CSS JIT, the whole selector is dropped to slow path. By adding the JIT support for :lang(), you ensure that :lang() no longer force the engine to the slow path.

We'll work on the optimizations in follow ups, let's first get rid of the CannotCompile.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3247
> +void SelectorCodeGenerator::generateElementIsInLanguage(Assembler::JumpList& failureCases, const Vector<LanguageArgument>* languageArguments)

In addition to this, you would have SelectorCodeGenerator::generateElementIsInLanguage(Assembler::JumpList& failureCases, const Vector<Vector<LanguageArgument>*> languageArguments) that would generate the function call for each Vector<LanguageArgument>*.

> LayoutTests/ChangeLog:9
> +        * fast/selectors/lang-multiple-chained-expected.txt: Added.
> +        * fast/selectors/lang-multiple-chained.html: Added.

This kind of test is great to find problems in the compiler, typically using too many registers or blowing up the stack.

One other kind of test you can do for :lang() is chain 500 language ranges.

>> LayoutTests/fast/selectors/lang-multiple-chained.html:26
>> +      shouldBe('document.querySelectorAll(":lang(en, de):lang(en, es):lang(en, fr)").length', '1');
> 
> I'm not sure this one is right. Should it be 3?

1 is right.

The only way to satisfy ":lang(en, de)" and ":lang(en, es)" and ":lang(en, fr)" is to have "en" or start with "en-".

[Dhi Aurrahman]

Created attachment 246379 [details]
Patch

[Dhi Aurrahman]

Created attachment 246380 [details]
Patch

[Dhi Aurrahman]

Created attachment 246391 [details]
Patch

[Benjamin Poulain]

Comment on attachment 246391 [details]
Patch

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

Great patch, simple and effective.

> LayoutTests/fast/selectors/lang-chained-multiple.html:11
> +        description('Verify chained multiple :lang() selectors will not blowing up the stack');

blowing->blow
(don't forget to update the test results accordingly).

> LayoutTests/fast/selectors/lang-chained-multiple.html:53
> +        shouldBe('document.querySelectorAll(":lang(' + createMultipleArguments('en-gb', 500) + ')").length', '1'); 
> +        shouldBe('document.querySelectorAll(":lang(' + createMultipleArguments('en-gb', 1000) + ')").length', '1'); 
> +        shouldBe('document.querySelectorAll("' + createChainedSelector(':lang(' + createMultipleArguments('en-gb', 10) + ')', 500) + '").length', '1');
> +        shouldBe('document.querySelectorAll("' + createChainedSelector(':lang(' + createMultipleArguments('en-gb', 100) + ')', 500) + '").length', '1');
> +        shouldBe('document.querySelectorAll("' + createChainedSelector(':lang(' + createMultipleArguments('en-gb', 200) + ')', 500) + '").length', '1');

Ouch, the results is a bit too big. The patch is 1mb. :(

Please try to drop the number until the result is in the ~100kb range.

[Dhi Aurrahman]

Created attachment 246420 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 246420 [details]
Patch for landing

Clearing flags on attachment: 246420

Committed r179978: <http://trac.webkit.org/changeset/179978>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.