Bug 126199 – Use the Selector Code Generator for resolving style

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Use the Selector Code Generator for resolving style

[Benjamin Poulain]

Created attachment 219952 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219952 [details]
Patch

Attachment 219952 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/50458029

[EFL EWS Bot]

Comment on attachment 219952 [details]
Patch

Attachment 219952 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/45698122

[kov's GTK+ EWS bot]

Comment on attachment 219952 [details]
Patch

Attachment 219952 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/51868073

[Antti Koivisto]

Comment on attachment 219952 [details]
Patch

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

> Source/WebCore/css/ElementRuleCollector.cpp:301
>  inline bool ElementRuleCollector::ruleMatches(const RuleData& ruleData, PseudoId& dynamicPseudo)
>  {
>      const StyleResolver::State& state = m_state;
> +#if ENABLE(CSS_SELECTOR_JIT)
> +    void* compiledSelectorChecker = ruleData.compiledSelectorCodeRef.code().executableAddress();
> +    if (!compiledSelectorChecker && ruleData.compilationStatus == SelectorCompilationStatus::NotCompiled) {
> +        JSC::VM* vm = document().scriptExecutionContext()->vm();
> +        ruleData.compilationStatus = SelectorCompiler::compileSelector(ruleData.selector(), vm, ruleData.compiledSelectorCodeRef);
> +        compiledSelectorChecker = ruleData.compiledSelectorCodeRef.code().executableAddress();
> +    }

This compiles too eagerly. If you look below in ruleData.hasFastCheckableSelector() branch you see there are number of cases we resolve without calling into SelectorCheckerFastPath at all. Those cases will use more memory and may get slower by activating the JIT. We should probably not compile selectors that are in this category.

Does the infrastructure support compiling off the main thread? I think optimally we would kick off the compiling during RuleData construction and use the compiled versions when ready.

> Source/WebCore/css/ElementRuleCollector.cpp:312
> +    if (compiledSelectorChecker) {
> +        if (ruleData.compilationStatus == SelectorCompilationStatus::SimpleSelectorChecker) {
> +            if (m_pseudoStyleRequest.pseudoId != NOPSEUDO)
> +                return false;
> +            SelectorCompiler::SimpleSelectorChecker selectorChecker = SelectorCompiler::simpleSelectorCheckerFunction(compiledSelectorChecker, ruleData.compilationStatus);
> +            return selectorChecker(state.element());
> +        }
> +        ASSERT(ruleData.compilationStatus == SelectorCompilationStatus::SelectorCheckerWithCheckingContext);
> +
> +        if (m_pseudoStyleRequest.pseudoId != NOPSEUDO)
> +            return false;

NOPSEUDO tests can move before the compilationStatus check and combined.

> Source/WebCore/css/RuleSet.h:84
> +#if ENABLE(CSS_SELECTOR_JIT)
> +    mutable SelectorCompilationStatus compilationStatus;
> +    mutable JSC::MacroAssemblerCodeRef compiledSelectorCodeRef;
> +#endif // ENABLE(CSS_SELECTOR_JIT)

These fields should be optimized for memory at some point (I think we can manage with 64bits or less for all this). We can have large numbers of selectors and so RuleDatas. We also shouldn't generate code for simplest selectors that are resolved by hash look-ups and don't need overhead of even having these fields.

[Benjamin Poulain]

Created attachment 220991 [details]
Patch

[EFL EWS Bot]

Comment on attachment 220991 [details]
Patch

Attachment 220991 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6029259655086080

[kov's GTK+ EWS bot]

Comment on attachment 220991 [details]
Patch

Attachment 220991 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/6717223087374336

[EFL EWS Bot]

Comment on attachment 220991 [details]
Patch

Attachment 220991 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6353650481889280

[Benjamin Poulain]

Created attachment 221004 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 221004 [details]
Patch

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

> Source/WebCore/css/ElementRuleCollector.cpp:312
> +        JSC::VM* vm = document().scriptExecutionContext()->vm();

We should be using a reference instead in all these places (probably should be done in a separate patch though).

> Source/WebCore/css/ElementRuleCollector.cpp:321
> +            SelectorCompiler::SimpleSelectorChecker selectorChecker = SelectorCompiler::simpleSelectorCheckerFunction(compiledSelectorChecker, ruleData.compilationStatus);

Do we really need this temporary variable?  Also, isn't this the case where auto will be useful?
Since this is a function pointer into a JIT'ed code, type doesn't give us any extra information anyway.

> Source/WebCore/css/RuleSet.h:84
> +#if ENABLE(CSS_SELECTOR_JIT)
> +    mutable SelectorCompilationStatus compilationStatus;
> +    mutable JSC::MacroAssemblerCodeRef compiledSelectorCodeRef;
> +#endif // ENABLE(CSS_SELECTOR_JIT)
> +

These should really be private and have accessors instead.

[Benjamin Poulain]

Committed r161917: <http://trac.webkit.org/changeset/161917>