This is a refactoring patch for upcoming optimization. We can remove m_rootNode and m_selectorChecker from SelectorQuery.h
Created attachment 144332 [details] Patch
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.
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).
(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 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.
Created attachment 144488 [details] patch for landing
(In reply to comment #5) > You should explain in the ChangeLog why this change is helpful for future optimizations. Done. Thanks for reviewing!
Comment on attachment 144488 [details] patch for landing Clearing flags on attachment: 144488 Committed r118748: <http://trac.webkit.org/changeset/118748>