Re: [webkit-dev] Optimizing DOMNode _nodeWith

Fri, 28 Apr 2006 13:16:41 -0700 


On Apr 28, 2006, at 12:41 PM, Matt Gough wrote:


On 24 Apr 2006, at 20:58, Maciej Stachowiak wrote:
I think a better approach would be to make a hash table of function pointers, that will be faster than the log chain of if's. This is what is used to generate the elements in the first place. 

On 24 Apr 2006, at 19:24, Timothy Hatcher wrote:
I would like to see before and after numbers when you have everything changed over.
OK, so I implemented this as a hashmap. Unfortunately my results show only marginal improvement. (i.e only a 1 - 3% reduction in time for the various sites I have tested. Investigating further it seems that my original assertion that 'For a medium sized DOM (e.g news.google.com - just over 4000 nodes) this code takes over one tenth of a second on an 867MHz G4. A third of that time is down to the calls to hasLocalName' was wrong. I suspect those timings were being done with a debug build of WebKit. On the largest page I could find (by node count) my changes made about 5ms improvement (http://whatwg.org/specs/web-apps/current-work/).
Anyway, I have put my work on this at the end of the email, but I am not convinced it is worth changing to this without there being a noticeable benefit. I guess the multiple calls to hasLocalName don't have as much impact as we might think. Maybe if someone could test this on different hardware it might make a bigger difference.
Generally, we consider a repeatable 1-3% performance improvement to be worth it. And I think your change is a cleaner design anyway. I encourage you to put it up on bugzilla. 

Regards,
Maciej





Matt



Index: WebCore/bindings/objc/DOM.mm
===================================================================
--- WebCore/bindings/objc/DOM.mm        (revision 14055)
+++ WebCore/bindings/objc/DOM.mm        (working copy)
@@ -44,10 +44,13 @@
#import "Range.h"
#import "dom_xmlimpl.h"
#import "HTMLNames.h"
+#import "QualifiedName.h"
#import "RenderImage.h"
#import <JavaScriptCore/WebScriptObjectPrivate.h>
#import <objc/objc-class.h>
+using WebCore::AtomicString;
+using WebCore::AtomicStringImpl;
using WebCore::Attr;
using WebCore::CharacterData;
using WebCore::Document;
@@ -69,6 +72,7 @@ using WebCore::NodeIterator;
using WebCore::NodeList;
using WebCore::Notation;
using WebCore::ProcessingInstruction;
+using WebCore::QualifiedName;
using WebCore::Range;
using WebCore::RenderImage;
using WebCore::RenderObject;
@@ -454,6 +458,218 @@ static ListenerMap *listenerMap;
@end
+typedef HashMap<AtomicStringImpl*, Class> ObjcClassMap;
+
+static ObjcClassMap *objcClassMap;
+
+static void addObjcClassForTag(const QualifiedName& tag, Class objcClass) 
+{
+    objcClassMap->set(tag.localName().impl(), objcClass);
+}
+
+
+static void createObjcClassMap()
+{
+    // Create the table.
+    objcClassMap = new ObjcClassMap;
+
+    // Populate it with DOM Element classes.
+    addObjcClassForTag(htmlTag, [DOMHTMLHtmlElement class]);
+    addObjcClassForTag(headTag, [DOMHTMLHeadElement class]);
+    addObjcClassForTag(linkTag, [DOMHTMLLinkElement class]);
+    addObjcClassForTag(titleTag, [DOMHTMLTitleElement class]);
+    addObjcClassForTag(metaTag, [DOMHTMLMetaElement class]);
+    addObjcClassForTag(baseTag, [DOMHTMLBaseElement class]);
+    addObjcClassForTag(isindexTag, [DOMHTMLIsIndexElement class]);
+    addObjcClassForTag(styleTag, [DOMHTMLStyleElement class]);
+    addObjcClassForTag(bodyTag, [DOMHTMLBodyElement class]);
+    addObjcClassForTag(formTag, [DOMHTMLFormElement class]);
+    addObjcClassForTag(selectTag, [DOMHTMLSelectElement class]);
+    addObjcClassForTag(optgroupTag, [DOMHTMLOptGroupElement class]);
+    addObjcClassForTag(optionTag, [DOMHTMLOptionElement class]);
+    addObjcClassForTag(inputTag, [DOMHTMLInputElement class]);
+    addObjcClassForTag(textareaTag, [DOMHTMLTextAreaElement class]);
+    addObjcClassForTag(buttonTag, [DOMHTMLButtonElement class]);
+    addObjcClassForTag(labelTag, [DOMHTMLLabelElement class]);
+    addObjcClassForTag(fieldsetTag, [DOMHTMLFieldSetElement class]);
+    addObjcClassForTag(legendTag, [DOMHTMLLegendElement class]);
+    addObjcClassForTag(ulTag, [DOMHTMLUListElement class]);
+    addObjcClassForTag(olTag, [DOMHTMLOListElement class]);
+    addObjcClassForTag(dlTag, [DOMHTMLDListElement class]);
+    addObjcClassForTag(dirTag, [DOMHTMLDirectoryElement class]);
+    addObjcClassForTag(menuTag, [DOMHTMLMenuElement class]);
+    addObjcClassForTag(liTag, [DOMHTMLLIElement class]);
+    addObjcClassForTag(divTag, [DOMHTMLDivElement class]);
+    addObjcClassForTag(pTag, [DOMHTMLParagraphElement class]);
+    addObjcClassForTag(h1Tag, [DOMHTMLHeadingElement class]);
+    addObjcClassForTag(h2Tag, [DOMHTMLHeadingElement class]);
+    addObjcClassForTag(h3Tag, [DOMHTMLHeadingElement class]);
+    addObjcClassForTag(h4Tag, [DOMHTMLHeadingElement class]);
+    addObjcClassForTag(h5Tag, [DOMHTMLHeadingElement class]);
+    addObjcClassForTag(h6Tag, [DOMHTMLHeadingElement class]);
+    addObjcClassForTag(qTag, [DOMHTMLQuoteElement class]);
+    addObjcClassForTag(preTag, [DOMHTMLPreElement class]);
+    addObjcClassForTag(listingTag, [DOMHTMLPreElement class]);
+    addObjcClassForTag(brTag, [DOMHTMLBRElement class]);
+    addObjcClassForTag(basefontTag, [DOMHTMLBaseFontElement class]);
+    addObjcClassForTag(fontTag, [DOMHTMLFontElement class]);
+    addObjcClassForTag(hrTag, [DOMHTMLHRElement class]);
+    addObjcClassForTag(aTag, [DOMHTMLAnchorElement class]);
+    addObjcClassForTag(imgTag, [DOMHTMLImageElement class]);
+    addObjcClassForTag(canvasTag, [DOMHTMLImageElement class]);
+    addObjcClassForTag(objectTag, [DOMHTMLObjectElement class]);
+    addObjcClassForTag(paramTag, [DOMHTMLParamElement class]);
+    addObjcClassForTag(appletTag, [DOMHTMLAppletElement class]);
+    addObjcClassForTag(mapTag, [DOMHTMLMapElement class]);
+    addObjcClassForTag(areaTag, [DOMHTMLAreaElement class]);
+    addObjcClassForTag(scriptTag, [DOMHTMLScriptElement class]);
+    addObjcClassForTag(tableTag, [DOMHTMLTableElement class]);
+    addObjcClassForTag(theadTag, [DOMHTMLTableSectionElement class]);
+    addObjcClassForTag(tbodyTag, [DOMHTMLTableSectionElement class]);
+    addObjcClassForTag(tfootTag, [DOMHTMLTableSectionElement class]);
+    addObjcClassForTag(tdTag, [DOMHTMLTableCellElement class]);
+    addObjcClassForTag(trTag, [DOMHTMLTableRowElement class]);
+    addObjcClassForTag(colTag, [DOMHTMLTableColElement class]);
+    addObjcClassForTag(colgroupTag, [DOMHTMLTableColElement class]);
+ addObjcClassForTag(captionTag, [DOMHTMLTableCaptionElement class]); 
+    addObjcClassForTag(framesetTag, [DOMHTMLFrameSetElement class]);
+    addObjcClassForTag(frameTag, [DOMHTMLFrameElement class]);
+    addObjcClassForTag(iframeTag, [DOMHTMLIFrameElement class]);
+}
+
+Class objcClassForTag(const AtomicString& tagName)
+{
+    if (!objcClassMap)
+        createObjcClassMap();
+
+    Class objcClass = objcClassMap->get(tagName.impl());
+    if (!objcClass)
+        objcClass = [DOMHTMLElement class];
+    return objcClass;
+}
+
+Class objcClassForHTMLElement(HTMLElement* htmlElt)
+{
+    // The old way for testing purposes
+    Class wrapperClass;
+    if (htmlElt->hasLocalName(htmlTag))
+        wrapperClass = [DOMHTMLHtmlElement class];
+    else if (htmlElt->hasLocalName(headTag))
+        wrapperClass = [DOMHTMLHeadElement class];
+    else if (htmlElt->hasLocalName(linkTag))
+        wrapperClass = [DOMHTMLLinkElement class];
+    else if (htmlElt->hasLocalName(titleTag))
+        wrapperClass = [DOMHTMLTitleElement class];
+    else if (htmlElt->hasLocalName(metaTag))
+        wrapperClass = [DOMHTMLMetaElement class];
+    else if (htmlElt->hasLocalName(baseTag))
+        wrapperClass = [DOMHTMLBaseElement class];
+    else if (htmlElt->hasLocalName(isindexTag))
+        wrapperClass = [DOMHTMLIsIndexElement class];
+    else if (htmlElt->hasLocalName(styleTag))
+        wrapperClass = [DOMHTMLStyleElement class];
+    else if (htmlElt->hasLocalName(bodyTag))
+        wrapperClass = [DOMHTMLBodyElement class];
+    else if (htmlElt->hasLocalName(formTag))
+        wrapperClass = [DOMHTMLFormElement class];
+    else if (htmlElt->hasLocalName(selectTag))
+        wrapperClass = [DOMHTMLSelectElement class];
+    else if (htmlElt->hasLocalName(optgroupTag))
+        wrapperClass = [DOMHTMLOptGroupElement class];
+    else if (htmlElt->hasLocalName(optionTag))
+        wrapperClass = [DOMHTMLOptionElement class];
+    else if (htmlElt->hasLocalName(inputTag))
+        wrapperClass = [DOMHTMLInputElement class];
+    else if (htmlElt->hasLocalName(textareaTag))
+        wrapperClass = [DOMHTMLTextAreaElement class];
+    else if (htmlElt->hasLocalName(buttonTag))
+        wrapperClass = [DOMHTMLButtonElement class];
+    else if (htmlElt->hasLocalName(labelTag))
+        wrapperClass = [DOMHTMLLabelElement class];
+    else if (htmlElt->hasLocalName(fieldsetTag))
+        wrapperClass = [DOMHTMLFieldSetElement class];
+    else if (htmlElt->hasLocalName(legendTag))
+        wrapperClass = [DOMHTMLLegendElement class];
+    else if (htmlElt->hasLocalName(ulTag))
+        wrapperClass = [DOMHTMLUListElement class];
+    else if (htmlElt->hasLocalName(olTag))
+        wrapperClass = [DOMHTMLOListElement class];
+    else if (htmlElt->hasLocalName(dlTag))
+        wrapperClass = [DOMHTMLDListElement class];
+    else if (htmlElt->hasLocalName(dirTag))
+        wrapperClass = [DOMHTMLDirectoryElement class];
+    else if (htmlElt->hasLocalName(menuTag))
+        wrapperClass = [DOMHTMLMenuElement class];
+    else if (htmlElt->hasLocalName(liTag))
+        wrapperClass = [DOMHTMLLIElement class];
+    else if (htmlElt->hasLocalName(divTag))
+        wrapperClass = [DOMHTMLDivElement class];
+    else if (htmlElt->hasLocalName(pTag))
+        wrapperClass = [DOMHTMLParagraphElement class];
+    else if (htmlElt->hasLocalName(h1Tag) ||
+             htmlElt->hasLocalName(h2Tag) ||
+             htmlElt->hasLocalName(h3Tag) ||
+             htmlElt->hasLocalName(h4Tag) ||
+             htmlElt->hasLocalName(h5Tag) ||
+             htmlElt->hasLocalName(h6Tag))
+        wrapperClass = [DOMHTMLHeadingElement class];
+    else if (htmlElt->hasLocalName(qTag))
+        wrapperClass = [DOMHTMLQuoteElement class];
+ else if (htmlElt->hasLocalName(preTag) || htmlElt->hasLocalName (listingTag)) 
+        wrapperClass = [DOMHTMLPreElement class];
+    else if (htmlElt->hasLocalName(brTag))
+        wrapperClass = [DOMHTMLBRElement class];
+    else if (htmlElt->hasLocalName(basefontTag))
+        wrapperClass = [DOMHTMLBaseFontElement class];
+    else if (htmlElt->hasLocalName(fontTag))
+        wrapperClass = [DOMHTMLFontElement class];
+    else if (htmlElt->hasLocalName(hrTag))
+        wrapperClass = [DOMHTMLHRElement class];
+    else if (htmlElt->hasLocalName(aTag))
+        wrapperClass = [DOMHTMLAnchorElement class];
+    else if (htmlElt->hasLocalName(imgTag) ||
+             htmlElt->hasLocalName(canvasTag))
+        wrapperClass = [DOMHTMLImageElement class];
+    else if (htmlElt->hasLocalName(objectTag))
+        wrapperClass = [DOMHTMLObjectElement class];
+    else if (htmlElt->hasLocalName(paramTag))
+        wrapperClass = [DOMHTMLParamElement class];
+    else if (htmlElt->hasLocalName(appletTag))
+        wrapperClass = [DOMHTMLAppletElement class];
+    else if (htmlElt->hasLocalName(mapTag))
+        wrapperClass = [DOMHTMLMapElement class];
+    else if (htmlElt->hasLocalName(areaTag))
+        wrapperClass = [DOMHTMLAreaElement class];
+    else if (htmlElt->hasLocalName(scriptTag))
+        wrapperClass = [DOMHTMLScriptElement class];
+    else if (htmlElt->hasLocalName(tableTag))
+        wrapperClass = [DOMHTMLTableElement class];
+    else if (htmlElt->hasLocalName(theadTag) ||
+             htmlElt->hasLocalName(tbodyTag) ||
+             htmlElt->hasLocalName(tfootTag))
+        wrapperClass = [DOMHTMLTableSectionElement class];
+    else if (htmlElt->hasLocalName(tdTag))
+        wrapperClass = [DOMHTMLTableCellElement class];
+    else if (htmlElt->hasLocalName(trTag))
+        wrapperClass = [DOMHTMLTableRowElement class];
+    else if (htmlElt->hasLocalName(colTag) ||
+             htmlElt->hasLocalName(colgroupTag))
+        wrapperClass = [DOMHTMLTableColElement class];
+    else if (htmlElt->hasLocalName(captionTag))
+        wrapperClass = [DOMHTMLTableCaptionElement class];
+    else if (htmlElt->hasLocalName(framesetTag))
+        wrapperClass = [DOMHTMLFrameSetElement class];
+    else if (htmlElt->hasLocalName(frameTag))
+        wrapperClass = [DOMHTMLFrameElement class];
+    else if (htmlElt->hasLocalName(iframeTag))
+        wrapperClass = [DOMHTMLIFrameElement class];
+    else
+        wrapperClass = [DOMHTMLElement class];
+
+    return wrapperClass;
+}
+
+
@implementation DOMNode (WebCoreInternal)
- (id)_initWithNode:(Node *)impl
@@ -482,122 +698,12 @@ static ListenerMap *listenerMap;
         case Node::ELEMENT_NODE:
             if (impl->isHTMLElement()) {
// FIXME: Reflect marquee once the API has been determined. - // FIXME: We could make the HTML classes hand back their class names and then use that to make 
-                // the appropriate Obj-C class from the string.
HTMLElement* htmlElt = static_cast<HTMLElement*> (impl); 
-                if (htmlElt->hasLocalName(htmlTag))
-                    wrapperClass = [DOMHTMLHtmlElement class];
-                else if (htmlElt->hasLocalName(headTag))
-                    wrapperClass = [DOMHTMLHeadElement class];
-                else if (htmlElt->hasLocalName(linkTag))
-                    wrapperClass = [DOMHTMLLinkElement class];
-                else if (htmlElt->hasLocalName(titleTag))
-                    wrapperClass = [DOMHTMLTitleElement class];
-                else if (htmlElt->hasLocalName(metaTag))
-                    wrapperClass = [DOMHTMLMetaElement class];
-                else if (htmlElt->hasLocalName(baseTag))
-                    wrapperClass = [DOMHTMLBaseElement class];
-                else if (htmlElt->hasLocalName(isindexTag))
-                    wrapperClass = [DOMHTMLIsIndexElement class];
-                else if (htmlElt->hasLocalName(styleTag))
-                    wrapperClass = [DOMHTMLStyleElement class];
-                else if (htmlElt->hasLocalName(bodyTag))
-                    wrapperClass = [DOMHTMLBodyElement class];
-                else if (htmlElt->hasLocalName(formTag))
-                    wrapperClass = [DOMHTMLFormElement class];
-                else if (htmlElt->hasLocalName(selectTag))
-                    wrapperClass = [DOMHTMLSelectElement class];
-                else if (htmlElt->hasLocalName(optgroupTag))
-                    wrapperClass = [DOMHTMLOptGroupElement class];
-                else if (htmlElt->hasLocalName(optionTag))
-                    wrapperClass = [DOMHTMLOptionElement class];
-                else if (htmlElt->hasLocalName(inputTag))
-                    wrapperClass = [DOMHTMLInputElement class];
-                else if (htmlElt->hasLocalName(textareaTag))
-                    wrapperClass = [DOMHTMLTextAreaElement class];
-                else if (htmlElt->hasLocalName(buttonTag))
-                    wrapperClass = [DOMHTMLButtonElement class];
-                else if (htmlElt->hasLocalName(labelTag))
-                    wrapperClass = [DOMHTMLLabelElement class];
-                else if (htmlElt->hasLocalName(fieldsetTag))
-                    wrapperClass = [DOMHTMLFieldSetElement class];
-                else if (htmlElt->hasLocalName(legendTag))
-                    wrapperClass = [DOMHTMLLegendElement class];
-                else if (htmlElt->hasLocalName(ulTag))
-                    wrapperClass = [DOMHTMLUListElement class];
-                else if (htmlElt->hasLocalName(olTag))
-                    wrapperClass = [DOMHTMLOListElement class];
-                else if (htmlElt->hasLocalName(dlTag))
-                    wrapperClass = [DOMHTMLDListElement class];
-                else if (htmlElt->hasLocalName(dirTag))
-                    wrapperClass = [DOMHTMLDirectoryElement class];
-                else if (htmlElt->hasLocalName(menuTag))
-                    wrapperClass = [DOMHTMLMenuElement class];
-                else if (htmlElt->hasLocalName(liTag))
-                    wrapperClass = [DOMHTMLLIElement class];
-                else if (htmlElt->hasLocalName(divTag))
-                    wrapperClass = [DOMHTMLDivElement class];
-                else if (htmlElt->hasLocalName(pTag))
-                    wrapperClass = [DOMHTMLParagraphElement class];
-                else if (htmlElt->hasLocalName(h1Tag) ||
-                         htmlElt->hasLocalName(h2Tag) ||
-                         htmlElt->hasLocalName(h3Tag) ||
-                         htmlElt->hasLocalName(h4Tag) ||
-                         htmlElt->hasLocalName(h5Tag) ||
-                         htmlElt->hasLocalName(h6Tag))
-                    wrapperClass = [DOMHTMLHeadingElement class];
-                else if (htmlElt->hasLocalName(qTag))
-                    wrapperClass = [DOMHTMLQuoteElement class];
- else if (htmlElt->hasLocalName(preTag) || htmlElt- >hasLocalName(listingTag)) 
-                    wrapperClass = [DOMHTMLPreElement class];
-                else if (htmlElt->hasLocalName(brTag))
-                    wrapperClass = [DOMHTMLBRElement class];
-                else if (htmlElt->hasLocalName(basefontTag))
-                    wrapperClass = [DOMHTMLBaseFontElement class];
-                else if (htmlElt->hasLocalName(fontTag))
-                    wrapperClass = [DOMHTMLFontElement class];
-                else if (htmlElt->hasLocalName(hrTag))
-                    wrapperClass = [DOMHTMLHRElement class];
-                else if (htmlElt->hasLocalName(aTag))
-                    wrapperClass = [DOMHTMLAnchorElement class];
-                else if (htmlElt->hasLocalName(imgTag) ||
-                         htmlElt->hasLocalName(canvasTag))
-                    wrapperClass = [DOMHTMLImageElement class];
-                else if (htmlElt->hasLocalName(objectTag))
-                    wrapperClass = [DOMHTMLObjectElement class];
-                else if (htmlElt->hasLocalName(paramTag))
-                    wrapperClass = [DOMHTMLParamElement class];
-                else if (htmlElt->hasLocalName(appletTag))
-                    wrapperClass = [DOMHTMLAppletElement class];
-                else if (htmlElt->hasLocalName(mapTag))
-                    wrapperClass = [DOMHTMLMapElement class];
-                else if (htmlElt->hasLocalName(areaTag))
-                    wrapperClass = [DOMHTMLAreaElement class];
-                else if (htmlElt->hasLocalName(scriptTag))
-                    wrapperClass = [DOMHTMLScriptElement class];
-                else if (htmlElt->hasLocalName(tableTag))
-                    wrapperClass = [DOMHTMLTableElement class];
-                else if (htmlElt->hasLocalName(theadTag) ||
-                         htmlElt->hasLocalName(tbodyTag) ||
-                         htmlElt->hasLocalName(tfootTag))
- wrapperClass = [DOMHTMLTableSectionElement class]; 
-                else if (htmlElt->hasLocalName(tdTag))
-                    wrapperClass = [DOMHTMLTableCellElement class];
-                else if (htmlElt->hasLocalName(trTag))
-                    wrapperClass = [DOMHTMLTableRowElement class];
-                else if (htmlElt->hasLocalName(colTag) ||
-                         htmlElt->hasLocalName(colgroupTag))
-                    wrapperClass = [DOMHTMLTableColElement class];
-                else if (htmlElt->hasLocalName(captionTag))
- wrapperClass = [DOMHTMLTableCaptionElement class]; 
-                else if (htmlElt->hasLocalName(framesetTag))
-                    wrapperClass = [DOMHTMLFrameSetElement class];
-                else if (htmlElt->hasLocalName(frameTag))
-                    wrapperClass = [DOMHTMLFrameElement class];
-                else if (htmlElt->hasLocalName(iframeTag))
-                    wrapperClass = [DOMHTMLIFrameElement class];
-                else
-                    wrapperClass = [DOMHTMLElement class];
+
+ // Do it both ways to simplify comparing the two ways in Shark + // Obviously only the first one should used if we go with this patch 
+                wrapperClass = objcClassForTag(htmlElt->localName());
+                wrapperClass = objcClassForHTMLElement(htmlElt);
             } else {
                 wrapperClass = [DOMElement class];
             }



_______________________________________________
webkit-dev mailing list
[email protected]
http://www.opendarwin.org/mailman/listinfo/webkit-dev

Reply via email to