Summary: | CSS selector JIT compilation support for :lang() | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dhi Aurrahman <diorahman> | ||||||||||||||
Component: | New Bugs | Assignee: | Dhi Aurrahman <diorahman> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Dhi Aurrahman
2015-01-23 00:19:05 PST
Created attachment 245214 [details]
Patch
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 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 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')? (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'. Created attachment 246248 [details]
Patch
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 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-". Created attachment 246379 [details]
Patch
Created attachment 246380 [details]
Patch
Created attachment 246391 [details]
Patch
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. Created attachment 246420 [details]
Patch for landing
Comment on attachment 246420 [details] Patch for landing Clearing flags on attachment: 246420 Committed r179978: <http://trac.webkit.org/changeset/179978> All reviewed patches have been landed. Closing bug. |