Bug 19002 - Optimize id selectors in querySelector/querySelectorAll
Summary: Optimize id selectors in querySelector/querySelectorAll
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Enhancement
Assignee: David Smith
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-11 20:50 PDT by David Smith
Modified: 2008-06-26 20:30 PDT (History)
2 users (show)

See Also:


Attachments
Initial patch (4.21 KB, patch)
2008-06-25 03:14 PDT, David Smith
no flags Details | Formatted Diff | Diff
Now with tests and less bugs (11.16 KB, patch)
2008-06-25 21:54 PDT, David Smith
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 2008-05-11 20:50:27 PDT
From irc:

othermaciej: Catfish_Man: I am belatedly reading the comments to http://webkit.org/blog/156/queryselector-and-queryselectorall/

I think we could in fact special-case ID selectors in querySelectorAll and querySelector

WebKit's internal ID cache not only caches the first match, but it also tracks whether there are multiple elements with that ID

I think it would be worth doing

So that a single uniform API can consistently give good performance
Comment 1 David Smith 2008-06-25 03:14:45 PDT
Created attachment 21927 [details]
Initial patch

Needs additional tests to ensure correct behavior in the presence of multiple elements with the same ID, and I probably missed a few coding style things. Other than that it should be ready for review.
Comment 2 David Smith 2008-06-25 21:54:34 PDT
Created attachment 21940 [details]
Now with tests and less bugs
Comment 3 Sam Weinig 2008-06-26 19:33:29 PDT
Comment on attachment 21940 [details]
Now with tests and less bugs

Please include the speedup and on what test you tested it on.

You can probably put this with the other Id related methods (getElementById, and hasElementWithId).
+    bool containsMultipleElementsWithId(const AtomicString& elementId) { return m_duplicateIds.contains(elementId.impl()); }

The parens around (querySelector->next()) are not needed.
+    if (!(querySelector->next()) && querySelector->m_match == CSSSelector::Id) {

Here too.
+    if (!(querySelector->next()) && querySelector->m_match == CSSSelector::Id && !document->containsMultipleElementsWithId(selectorValue)) {

You can remove the else here and and just return 0.
+        if (element && (isDocumentNode() || element->isDescendantOf(this)))
+            return element;
+    } else {
+        // FIXME: We can speed this up by implementing caching similar to the one use by getElementById


r=me
Comment 4 David Smith 2008-06-26 20:30:40 PDT
r34823