Bug 87650

Summary: Remove m_rootNode and m_selectorChecker from SelectorQuery.h
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: CSSAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: koivisto, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87625    
Attachments:
Description Flags
Patch
none
patch for landing none

Description Kentaro Hara 2012-05-28 04:25:46 PDT
This is a refactoring patch for upcoming optimization. We can remove m_rootNode and m_selectorChecker from SelectorQuery.h
Comment 1 Kentaro Hara 2012-05-28 04:28:38 PDT
Created attachment 144332 [details]
Patch
Comment 2 Antti Koivisto 2012-05-29 01:10:23 PDT
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.
Comment 3 Kentaro Hara 2012-05-29 01:30:52 PDT
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).
Comment 4 Antti Koivisto 2012-05-29 02:02:23 PDT
(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.
Comment 5 Antti Koivisto 2012-05-29 02:03:32 PDT
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.
Comment 6 Kentaro Hara 2012-05-29 02:25:42 PDT
Created attachment 144488 [details]
patch for landing
Comment 7 Kentaro Hara 2012-05-29 02:26:25 PDT
(In reply to comment #5)
> You should explain in the ChangeLog why this change is helpful for future optimizations.

Done. Thanks for reviewing!
Comment 8 WebKit Review Bot 2012-05-29 03:43:05 PDT
Comment on attachment 144488 [details]
patch for landing

Clearing flags on attachment: 144488

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