Bug 87650 – Remove m_rootNode and m_selectorChecker from SelectorQuery.h

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kentaro Hara]

This is a refactoring patch for upcoming optimization. We can remove m_rootNode and m_selectorChecker from SelectorQuery.h

[Kentaro Hara]

Created attachment 144332 [details]
Patch

[Antti Koivisto]

Comment on attachment 144332 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This is a refactoring patch for upcoming optimization.
> +        We can remove m_rootNode and m_selectorChecker from SelectorQuery.h.

Where are are you heading with this? An obvious optimization direction would be caching SelectorQuery objects but this would make it less effective.

[Kentaro Hara]

Thanks anttik!

(In reply to comment #2)
> > Source/WebCore/ChangeLog:9
> > +        This is a refactoring patch for upcoming optimization.
> > +        We can remove m_rootNode and m_selectorChecker from SelectorQuery.h.
> 
> Where are are you heading with this? An obvious optimization direction would be caching SelectorQuery objects but this would make it less effective.

Yes, I am planning to cache SelectorQuery objects into a HashMap, which will be defined in Node::querySelector(). (It's not so obvious to implement it correctly though:-)

(Leaving aside that the patch is beneficial in that it reduces member variables, ) why do you think that the patch makes something ineffective? If we cache SelectorQuery objects, we need to anyway reconstruct SelectorChecker at every time when queryFirst() or queryAll() is called (since rootNode->document() might change or rootNode->document()->inQuirksMode() might change).

[Antti Koivisto]

(In reply to comment #3)
> (Leaving aside that the patch is beneficial in that it reduces member variables, ) why do you think that 

Reducing member variables is not beneficial or harmful in itself. It depends on context. 

Looking into this more, SelectorChecker is too big to cache per-query so removing it makes sense. Caching root node adds no value. This patch should be good.

> the patch makes something ineffective? If we cache SelectorQuery objects, we need to anyway reconstruct SelectorChecker at every time when queryFirst() or queryAll() is called (since rootNode->document() might change or rootNode->document()->inQuirksMode() might change).

If the cache is per-document neither of those should be an issue. Generally that kind of problems can be solved with a cache invalidation strategy.

[Antti Koivisto]

Comment on attachment 144332 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Remove m_rootNode and m_selectorChecker from SelectorQuery.h
>> +        https://bugs.webkit.org/show_bug.cgi?id=87650
>> +
>> +        Reviewed by NOBODY (OOPS!).
>> +
>> +        This is a refactoring patch for upcoming optimization.
>> +        We can remove m_rootNode and m_selectorChecker from SelectorQuery.h.
> 
> Where are are you heading with this? An obvious optimization direction would be caching SelectorQuery objects but this would make it less effective.

You should explain in the ChangeLog why this change is helpful for future optimizations.

[Kentaro Hara]

Created attachment 144488 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #5)
> You should explain in the ChangeLog why this change is helpful for future optimizations.

Done. Thanks for reviewing!

[WebKit Review Bot]

Comment on attachment 144488 [details]
patch for landing

Clearing flags on attachment: 144488

Committed r118748: <http://trac.webkit.org/changeset/118748>