When calling getAttribute from JavaScript, the code enters nsGenericElement::GetAttribute that creates a nsNodeInfo pointer. It then fetches the nodeinfo through nsGenericHTMLElement::NormalizeAttrString which calls nsNodeInfoManager::GetNodeInfo. GetNodeInfo creates the nsNodeInfo and inserts it into the hashtable used as cache and then returns. When coming back to GetAttribute, the nsNodeInfo pointer is released which causes it to be removed from the hashtable it just was added to. That visit in the hashtable is ~5% (Quantify says 15% but I believe my watch more) of the time in getAttribute. I avoided this by adding a GetNodeInfoNoCache to the nsNodeInfoManager interface which never added anything to the hashtable, just looked in it. This made my times in getAttribute (see bug 118933) go from 470 to 450ms. An even better solution would be to avoid removing the node from the hashtable, or would that add too much bloat? Interested? I can attach a patch if this is a good thing to do.

> That visit in the hashtable is ~5% of the time in getAttribute. That is 5% of the time in the JavaScript getAttribute. It's more like 20% of the time in the nsGenericElement::GetAttribute.

jst, can you review this patch? It speeds up getAttribute by bypassing the add/remove in the hashtable.

Comment on attachment 100133 [details] [diff] [review] Patch that avoid the hashtable add/remove >+// PLHashEntry *he; >+// he = PL_HashTableAdd(mNodeInfoHash, &newNodeInfo->mInner, newNodeInfo); >+// NS_ENSURE_TRUE(he, NS_ERROR_OUT_OF_MEMORY); No need to keep this, just remove instead of commenting out. >Index: html/content/src/nsGenericHTMLElement.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v >retrieving revision 1.377 >diff -u -r1.377 nsGenericHTMLElement.cpp >--- html/content/src/nsGenericHTMLElement.cpp 11 Sep 2002 01:58:59 -0000 1.377 >+++ html/content/src/nsGenericHTMLElement.cpp 22 Sep 2002 10:47:03 -0000 >@@ -1584,7 +1584,9 @@ > mNodeInfo->GetNodeInfoManager(*getter_AddRefs(nimgr)); > NS_ENSURE_TRUE(nimgr, NS_ERROR_FAILURE); > >- return nimgr->GetNodeInfo(lower, nsnull, kNameSpaceID_None, aNodeInfo); >+ nsCOMPtr<nsIAtom> nameAtom(dont_AddRef(NS_NewAtom(lower))); Use nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(lower);

Comment on attachment 106360 [details] [diff] [review] Addresses peterv's comments correctly Uses do_GetAtom and removes dead code. Ready for review round 2.

I don't like this. Sure, this will make us not put nodeinfo's in the plhash only to pull them out later, but this will cause problems the day our HTML attributes actually store the node info in the attribute... Once that's done we'll have to back this out, or at least tweak how this works, since the node info stored in the new attribute better be in the nodeinfo managers cache so that the next time someone requests this attribute we'll find the same nodeinfo and thus find the same attribute (not to mention the fact that this would make every attribute with the same name hold it's own nodeinfo). And also, if we really care about performance here, then it seems really wrong to me to allocate a new nsNodeInfo every time a hash lookup comes back empty (which I'm guessing is the common case), I'd say it's well worth introducing a one-nsNodeInfo cache here and re-use the memory when creating/destroying nsNodeInfo's this way. (Look at how nsDOMEventRTTearoff::mCachedEventTearoff is used, but leave the cache size at 1)

I can't see why this patch should be stopped because nsNodeInfo objects some time in the future might be long lived. They aren't now, and as far as I can tell they have been short lived for some time. If you some time in the future don't want the NoCache method to be used, you only have to change one line; remove 7 characters. I've been thinking of what you said about a one object nsINodeInfo cache. I will try that, but I think that both things should be done, using a one node cache and avoiding the hash table when it is of no use.

Sure, I won't hold this out of the tree (I realize I did, but I didn't mean to sound like I really didn't want this checked in), but please do comment about the fact that the nodeinfo's that are gotten from this new method may not be usable in cases where the nodeinfo will be held onto somewhere and where object identity matters.

So let's see a new patch with that comment (reference this bug) and get it in the tree.

I've had a patch including the changes jst suggested in comment #7, but my problem has been that I've no longer access to the profiling tools I used to work with so I can no longer make any good measurements of the impact. This should be faster so I would like to check it in, but I would feel happier if I could look at it in Quantify first. (Wow IBM bought Rational; that's one big fat cash cow I guess)

Comment on attachment 108481 [details] [diff] [review] Patch with everyone's comments addressed Moving review requests (shouldn't that be automated somehow?)

Comment on attachment 108481 [details] [diff] [review] Patch with everyone's comments addressed >Index: base/src/nsNodeInfo.cpp >=================================================================== >+void >+nsNodeInfo::LastRelease() >+{ >+ if (sCachedNodeInfo == nsnull) { if (!sCachedNodeInfo) >Index: base/src/nsNodeInfo.h >=================================================================== >+ /** >+ * This method gets called by Release() when it's time to delete the >+ * this object, in stead of always deleting the object we'll put the >+ * object in the cache if unless the cache is already full. >+ */ Line up the asterix on these lines like this: /** * * */ >Index: base/src/nsNodeInfoManager.cpp >=================================================================== >+/** >+ * The same as GetNodeInfo but without inserting into the hashtable. >+ * Should be used when we know that the nsINodeInfo will only live for >+ * a short time (allocated on the stack or similar life time). This >+ * will not be usable when attribute values are stored in the node >+ * info, since any changes to those won't be saved. >+ */ Put this comment in the interface header, where users of the interface will look ;-).

Comment on attachment 108481 [details] [diff] [review] Patch with everyone's comments addressed What peterv said, and... - In nsNodeInfoManager::GetNodeInfoNoCache(): + nsNodeInfo *newNodeInfo = nsNodeInfo::Create(); + NS_ENSURE_TRUE(newNodeInfo, NS_ERROR_OUT_OF_MEMORY); + + NS_ADDREF(newNodeInfo); + + nsresult rv = newNodeInfo->Init(aName, aPrefix, aNamespaceID, this); + NS_ENSURE_SUCCESS(rv, rv); if (NS_FAILED(rv)) you'll leak newNodeInfo here, make this an if check, and release newNodeInfo if we fail to initialize it. I ran before and after this patch in Quantify, and this chnage makes nsGenericElement::GetAttribute() 2.3 times faster (time per call went down from 4.55 to 1.92us) when looking for an attribute whose name is not in the nodeinfo hash (which is the case for most (all?) attributes). Nicely done! Fix those issues and "ship it"! sr=jst

Comment on attachment 108481 [details] [diff] [review] Patch with everyone's comments addressed Actually, peterv and I realized that this patch is not safe. This will for sure cause us to *not* share nodeinfo's on attributes that are set with setAttribute() if the attribute nodeinfo is not already in the nodeinfo hash... This complicated, for HTML attributes we *do* store nodeinfo for the attribute name *if* the attribute is in a namespace (i.e. has a prefix in XHTML), and in that case we do rely on the identity of the nodeinfo objects when looking for attributes that are already set, IOW, this would break those cases. IOW, we need to make sure that setAttribute() will store the nodeinfo in the nodeinfo hash *if* it's stored by the attribute. This is pretty nasty, we could always store it, or we could make the setAttribute() code depend on the fact that we're only storing the nodeinfo for an attribute when the attribute name has a prefix. I don't know what to suggest here, but this can't go in as is. Sicking will hopefully jump in soon and rewrite our attribute code to make it make more sense, so he should be aware of this...

::NormalizeAttrString should die, i have a patch to do it but it bitrotted when i landed the html-attributes change. See bug 126765

I'm glad that we analyze this before checking it in and breaking anything and I see the problem. If I really want this performance win, should I create a non-caching NormalizeAttrString? The other optimization you suggested, the one element cache, is still valid I assume.

Yeah, the one-item cache is still good, and maybe the best (and least intrusive) option is to add a boolean out argument to NormalizeAttrString() and nsINodeInfoManager::GetNodeInfoNoCache() that tells the caller whether or not the nodeinfo that's returned was from the cache or not, and then if it wasn't from the cache when setAttribute() was called then setAttribute() could cache it (by calling GetNodeInfo()), if needed (i.e. in the HTML case it would cache it if there was a prefix, or rather, if there was a namespace, specified in the attribute name). I don't know, it seems somewhat like a hack anyway we do this, but it's a nice perf improvment, so I'd say it's worth at least thinking about.

Yes, it will be hacky anyway I can think of short of a redesign, and that's a little too big for me. Your idea will work, but it will affect everyone who inherits from nsIContent and overrides NormalizeAttr in that failure to override the new function will cause broken functionality. Unfortunately the compiler won't help since there are base versions of the function. (Suddenly I understand what's good with C#'s override and new keywords) Another solution, inspired from CWnd in MFC and similar to what I mentioned in my last post is to add NormalizeAttrTemp and rename GetNodeInfoNoCache GetNodeInfoTemp. It will take the out bool you mentioned but will not guarantee that the returned object is persistant. The good thing about it is that no old code will be affected unless switched to the Temp function. If this is the way to go, should it be put only in nsGenericHTMLElement or should it be put in nsIContent? The latter makes the performance win possible to other types of elements, but the former is a much more local change with less code and less potential for problems.

IMHO the best thing to do would be to resurrect my patch in bug 126765. It gets around the entire problem since it makes us not return an nodeinfo unless it already exists.

I could give it a try. I guess I could use your previous patch as template?

all that is needed is to sync the previous patch to the tip and then implement ::GetAttrWithName for html-elements

I'm doing it right now. I even remembered to update my tree after tonight's content changes almost before I began patching. :-)

And more exactly, what do you mean with "implement ::GetAttrWithName for html-elements". There is a GetAttrWithName in nsGenericHTMLElement in your patch. Isn't that enough?

it won't work since nowadays html-elements can hold namespaced attributes. You'll have to write a new implementation of it that walks the mAttributes list and checks which has a nodeinfo name and which has an atom-name.

This is a little greek to me, but let me try anyway. I noticed (or the compiler did it for me) that attributes not only had a name but a namespace and prefix too. First I just discarded those, but you're saying that those have meaning now? Anyway, the input to the function GetAttrWithName is just the name, so nothing but the name can be compared? Or should prefix/namespace and name be concatenated someway first? Otherwise, it should be enough to in the line: return nimgr->GetNodeInfo(nameAtom, nsnull, kNameSpaceID_None, aNodeInfo); replace nsnull with the prefix and kNameSpaceID_None with the namespace from mAttributes?

A nodeinfo is a namespace, a prefix and a localName. For example the element <xhtml:div> has the prefix "xhtml", the localName "div" and the namespace that "xhtml" mapped to while parsing the original XML file. ::GetAttrWithName (just as ::NormalizeAttrString) returns a nodeInfo that has the same name as the requested string. So ::GetAttrWithName("foo:bar") should return a nodeInfo with the full name "foo:bar". The difference between ::GetAttrWithName and ::NormalizeAttrString is that ::NormalizeAttrString returns a nodeinfo even if the element doesn't have such an attribute. So what you should do in ::GetAttrWithName for html-elements is to go through all attributes in the element and compare the name of the attribute to the requested name. What is special about html-elements is that the name of the attributes can be either an atom (which means that the attribute doesn't have a prefix) or an nodeInfo

I see. But I also see that nsHTMLAttributes::GetAttributeNameAt(...) handles all that so if the name is a node info it is converted to an atom name before returning. So in nsGenericHTMLElement::GetAttrWithName, we can still only compare the name and forget about the rest, except for using if creating a new nsNodeInfo?

Anyway, I've ported all of your patch to the current tree, except maybe for nsGenericElement::GetAttrWithName, and something seems to be wrong. I get the wrong window icon, autocomplete doesn't work, I can not load pages by pressing enter in the url bar. I can load pages from my bookmarks and they do look fine as far as I can tell. Do you have a guess about what can be wrong? Something with XUL? I did a diff of your and mine patch and they do look the same except for line numbers and such. The patch is attached in bug 126765.

So I found the problem and cleaned up the patch. It is http://bugzilla.mozilla.org/attachment.cgi?id=109110&action=edit in bug 126765.

Mass-reassigning bugs to dom_bugs@netscape.com

Fixed with the fix for bug 126765.