Bug 98829 – Minimize the recent template explosion in SelectorChecker.

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

[Dimitri Glazkov (Google)]

Minimize the recent template explosion in SelectorChecker.

[Dimitri Glazkov (Google)]

Created attachment 167867 [details]
WIP

[Dimitri Glazkov (Google)]

Created attachment 168048 [details]
Patch

[Antti Koivisto]

Comment on attachment 168048 [details]
Patch

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

> Source/WebCore/css/SelectorChecker.h:100
> -    template<typename CheckingContext>
> -    SelectorMatch checkSelector(const CheckingContext&, PseudoId&, bool& hasUnknownPseudoElements) const;
> -    template<typename CheckingContext>
> -    bool checkOneSelector(const CheckingContext&) const;
> +    SelectorMatch checkSelector(const SelectorCheckingContext&, PseudoId&, bool& hasUnknownPseudoElements) const;
> +    template<typename SiblingTraversalStrategy>
> +    bool checkOneSelector(const SelectorCheckingContext&, const SiblingTraversalStrategy&) const;

Since only checkOneSelector is now templated I suppose you expect to be calling it directly with a different strategy?

You are now calling SiblingTraversalStrategy with object that contains no data. Why? Do you expect that some other strategy will need data?  Are you sure the object and this pointer gets optimized away?

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> (From update of attachment 168048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168048&action=review
> 
> > Source/WebCore/css/SelectorChecker.h:100
> > -    template<typename CheckingContext>
> > -    SelectorMatch checkSelector(const CheckingContext&, PseudoId&, bool& hasUnknownPseudoElements) const;
> > -    template<typename CheckingContext>
> > -    bool checkOneSelector(const CheckingContext&) const;
> > +    SelectorMatch checkSelector(const SelectorCheckingContext&, PseudoId&, bool& hasUnknownPseudoElements) const;
> > +    template<typename SiblingTraversalStrategy>
> > +    bool checkOneSelector(const SelectorCheckingContext&, const SiblingTraversalStrategy&) const;
> 
> Since only checkOneSelector is now templated I suppose you expect to be calling it directly with a different strategy?

Right. Here's a larger context of this patch: https://bugs.webkit.org/attachment.cgi?id=167976&action=review

> 
> You are now calling SiblingTraversalStrategy with object that contains no data. Why? Do you expect that some other strategy will need data?

Yes, the ShadowDOMSiblingTraversalStrategy will contain a list of nodes that are considered siblings in that context.

>  Are you sure the object and this pointer gets optimized away?

I spent a bit of time trying to discern this in assembly, but my disas-foo is weak :-\ I couldn't see much difference between the callsites. The PerformanceTests/CSS/PseudoClassSelectors.html shows no difference in performance.

[Antti Koivisto]

Comment on attachment 168048 [details]
Patch

Ok, r=me

[WebKit Review Bot]

Comment on attachment 168048 [details]
Patch

Rejecting attachment 168048 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14248724

[Dimitri Glazkov (Google)]

Created attachment 168127 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168127 [details]
Patch for landing

Clearing flags on attachment: 168127

Committed r131002: <http://trac.webkit.org/changeset/131002>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.