Bug 140818 - CSS selector JIT compilation support for :lang()
Summary: CSS selector JIT compilation support for :lang()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dhi Aurrahman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-23 00:19 PST by Dhi Aurrahman
Modified: 2015-02-11 18:34 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.78 KB, patch)
2015-01-23 00:30 PST, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (37.54 KB, patch)
2015-02-08 13:54 PST, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (1.09 MB, patch)
2015-02-11 00:37 PST, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (1.09 MB, patch)
2015-02-11 00:46 PST, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch (1.09 MB, patch)
2015-02-11 07:28 PST, Dhi Aurrahman
no flags Details | Formatted Diff | Diff
Patch for landing (109.05 KB, patch)
2015-02-11 17:49 PST, Dhi Aurrahman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dhi Aurrahman 2015-01-23 00:19:05 PST
CSS selector JIT compilation support for :lang()
Comment 1 Dhi Aurrahman 2015-01-23 00:30:22 PST
Created attachment 245214 [details]
Patch
Comment 2 Dhi Aurrahman 2015-01-23 01:50:50 PST
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.
Comment 3 Benjamin Poulain 2015-01-26 13:52:18 PST
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.
Comment 4 Dhi Aurrahman 2015-01-31 20:41:51 PST
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')?
Comment 5 Benjamin Poulain 2015-01-31 22:19:42 PST
(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'.
Comment 6 Dhi Aurrahman 2015-02-08 13:54:07 PST
Created attachment 246248 [details]
Patch
Comment 7 Dhi Aurrahman 2015-02-08 13:56:30 PST
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?
Comment 8 Benjamin Poulain 2015-02-10 13:52:27 PST
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-".
Comment 9 Dhi Aurrahman 2015-02-11 00:37:04 PST
Created attachment 246379 [details]
Patch
Comment 10 Dhi Aurrahman 2015-02-11 00:46:36 PST
Created attachment 246380 [details]
Patch
Comment 11 Dhi Aurrahman 2015-02-11 07:28:25 PST
Created attachment 246391 [details]
Patch
Comment 12 Benjamin Poulain 2015-02-11 15:29:37 PST
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.
Comment 13 Dhi Aurrahman 2015-02-11 17:49:13 PST
Created attachment 246420 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2015-02-11 18:34:01 PST
Comment on attachment 246420 [details]
Patch for landing

Clearing flags on attachment: 246420

Committed r179978: <http://trac.webkit.org/changeset/179978>
Comment 15 WebKit Commit Bot 2015-02-11 18:34:05 PST
All reviewed patches have been landed.  Closing bug.