Bug 19002 – Optimize id selectors in querySelector/querySelectorAll

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[David Smith]

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

[David Smith]

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.

[David Smith]

Created attachment 21940 [details]
Now with tests and less bugs

[Sam Weinig]

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

[David Smith]

r34823