659539 - Move more useful information into nsINodeInfo

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bug 614171 made us store the nodeName for elements in the nsINodeInfo object as to avoid doing string manipulation and case conversion from GetNodeName.

However, we can go further. By storing the nodeName for *all* nodes in the nsINodeInfo we can make quickstubs avoid calling any virtual functions in order to implement the nodeName getter.

We can also do the same for the nodeType and the localName. (we could also do the namespaceURI, but I doubt that's used commonly enough to worry about performance).

This means that we have to add the nodeType to the key for the nodeInfos. However this really shouldn't cause us to have any more nodeInfos since there is little to no collisions between attribute and element names. Especially given that elements are generally in the XHTML/XUL/SVG namespace and attributes are in the null namespace.

There is however a tricky part. DocTypes and Processing Instructions have a .nodeName different from what we today use as nsINodeInfo->NameAtom() atom. We could change that such that PIs and doctypes use their "real" name as name-atom. However this would make things like nsIContent::Tag and nsIContent::NodeInfo return names which look a lot like elements but aren't.

This *should* be fine since people should be checking the namespace anyway, but in reality there are lots of places that don't. So I prefer to hold off on that change for now and instead just give nsINodeInfo another atom which is only used for PIs and DocTypes.

Patches coming up.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 1: Store nodeType/nodeName/localName in the nsINodeInfo

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 2: Create nsINode::NodeType/NodeName/LocalName and make them non-virtual

Also removes nsINode::GetNodeType.

nsINode::NodeType and nsINode::NodeName are actually getting added in bug 659053 which this applies on top of.

Same thing for the nsDocumentFragmentForward class which was created to let me use the NS_FORWARD_NSIDOMNODE macro for docfragments and still override a couple of functions. However since these functions no longer needs to be overridden I can get rid of it.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 3: Use nsINode::NodeName

This makes us use nsINode::NodeName in a few places. Enough that we can get rid of nsINodeInfo::QualifiedNameCorrectedCase.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 534963 [details] [diff] [review]
Part 3: Use nsINode::NodeName

Oh, this does contain a behavioral change. We no longer uppercase the nodeName of HTML attributes in XSLT/XPath. I don't know why we ever did, but since the DOM doesn't it doesn't make much sense to do it in XPath.

Peter: would be great if you could sign off on that change.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 4: Use nsINode::NodeType

Since we now have the new spiffy nodeType accessor director on nsINode, there is no longer any need to QI to nsIDOMNode and use the uglier and slower GetNodeType signature there.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 5: Store the nsIDocument directly on the nsINodeInfo

Currently we have to go through 2 memory dereferences in order to implement GetOwnerDoc() and GetCurrentDoc(). It seems much more efficient to store the document directly on the nsINodeInfo and instead.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 4: Use nsINode::NodeType -w

For easier reviewing

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 534961 [details] [diff] [review]
Part 1:  Store nodeType/nodeName/localName in the nsINodeInfo

This patch is missing a

  NS_IF_RELEASE(mInner.mExtraName);

in the nsINodeInfo dtor.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534961 [details] [diff] [review]
Part 1:  Store nodeType/nodeName/localName in the nsINodeInfo

Please document the values that NodeType() and mNodeType use.  If these are the nsIDOMNode *_NODE values, just say that.

The various places where you assert that NodeType() is correct (generic dom data node, textnode, generic element, doctype, xmlprocessinginstruction, etc) should probably just use NS_ABORT_IF_FALSE.

There are various places where you pass -1 as the aNodeType.  I'd prefer that this were documented as something that can be done and what it means (and perhaps passing PR_UINT16_MAX would be clearer).

I don't know why you changed the existing NS_ABORT_IF_FALSE calls to use NS_PRECONDITION in the nsNodeInfo constructor... please change them back and make your preconditions also NS_ABORT_IF_FALSE?

Again, PR_UINT16_MAX instead of PRUint16(-1) in the ctor.

Why does it make sense to assert aNodeType != 0 (as opposed to asserting that it's either PR_UINT16_MAX or in the range of valid nodetypes)?

In the nodeinfo ctor's last precondition, you don't need to check aNamespaceID, since the third precondition already checked that for all those nodetypes.  Taking that out would make it a bit more readable, I think.

Add a NS_ABORT_IF_FALSE(aNodeType == -1) default case to the switch at the end of the constructor, please.

You caught the need to release mExtraName in the dtor already.

In the new setup, looks like SetAttributeNS will probably create a new nodeinfo each time (since nothing in the cache will have a type of -1).  That's probably ok.  SetAttributeNS is not used much.

In nsNodeInfoManager::GetNodeInfo methods the assert about aNodeType being nonzero should instead be a range check just like I asked for in the nsNodeInfo ctor.

In the version of GetNodeInfo that takes a string namespace URI, nameAtom is now unused.  Take it out, please.

In nsHTMLParanoidFragmentSink::NameFromNode just skip the nsCOMPtr and do |*aResult = NS_NewAtom(aNode.GetText())|.

Please document why are we using -1 as the nodetype in nsXULPrototypeElement::Serialize for the atom attributes instead of using ATTRIBUTE_NODE.  Or better yet, use ATTRIBUTE_NODE.

In fact, per IRC discussion, let's just drop this -1 thing and use the right type throughout.  That means also in the XUL content sink and in SetAttributeNS.  nsXULPrototypeDocument will have to read and write the nodeinfo type, and we'll need to rev the fastload version.

r=me with those nits addressed.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(adding dependency since i'd rather not deal with the merge pain of landing these patches out of order)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534962 [details] [diff] [review]
Part 2: Create nsINode::NodeType/NodeName/LocalName and make them non-virtual

Any reason to not use LocalName() in nsGenericDOMDataNode::GetLocalName?

In the localName and nodeName quickstubs, can you just make |result| a reference so that you don't have to create a new nsString object?

r=me with the latter change no matter what you do for the former.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534963 [details] [diff] [review]
Part 3: Use nsINode::NodeName

Again, use a reference in the quickstub.  r=me with that.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #11)
> Comment on attachment 534962 [details] [diff] [review] [review]
> Part 2: Create nsINode::NodeType/NodeName/LocalName and make them non-virtual
> 
> Any reason to not use LocalName() in nsGenericDOMDataNode::GetLocalName?

No, that should work too. Is there a reason you prefer that?

> In the localName and nodeName quickstubs, can you just make |result| a
> reference so that you don't have to create a new nsString object?

Can't do that since the function which converts nsString->JSString modifies the former (asks it to drop the reference to its buffer).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534962 [details] [diff] [review]
Part 2: Create nsINode::NodeType/NodeName/LocalName and make them non-virtual

Oh, and document that NodeType() returns the nsINode::*_NODE constants, ok?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534964 [details] [diff] [review]
Part 4: Use nsINode::NodeType

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534965 [details] [diff] [review]
Part 5: Store the nsIDocument directly on the nsINodeInfo

Two things:

1)  Document that mDocument is a weak pointer.
2)  If mInner is 56 bytes or more on a 64-bit system, move mDocument before mInner so it ends up in the first cache line (ideally in the same cache line as the commonly used mInner members, in fact).

r=me with that.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> No, that should work too. Is there a reason you prefer that?

Just consistency... and only having to modify one place if for some reason the behavior changes. ;)

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #17)
> > No, that should work too. Is there a reason you prefer that?
> 
> Just consistency... and only having to modify one place if for some reason
> the behavior changes. ;)

There's still two separate implementations though.

I agree we could move more functions from being on nsGenericElement and nsGenericDOMDataNode (and possibly nsDocument) to be shared on nsINode. But I'd rather do that separately I guess.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I meant that the LocalName() is already the right thing (sets the string to null), so why not just call it here instead of having LocalName() and GetLocalName() independently doing the same thing?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in! Thanks for the quick reviews.

http://hg.mozilla.org/mozilla-central/rev/dd6e523b0b82
http://hg.mozilla.org/mozilla-central/rev/af6c49e88933
http://hg.mozilla.org/mozilla-central/rev/55b260f20710
http://hg.mozilla.org/mozilla-central/rev/a135002c33b6
http://hg.mozilla.org/mozilla-central/rev/ccf0a112a858

I chatted with Smaug and he was ok with landing part 5 as well. All that should be needed to merge with bug 335998 is to make sure to call

PL_HashTableEnumerateEntries(mNodeInfoHash, DropNodeInfoDocument, nsnull);

any time mDocument is nulled out. (Or if there are other simpler means of nulling out the mDocument pointer on all nodeinfos, that works too obviously).

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 534961 [details] [diff] [review]
Part 1:  Store nodeType/nodeName/localName in the nsINodeInfo

>--- a/content/html/document/src/nsHTMLFragmentContentSink.cpp
>+++ b/content/html/document/src/nsHTMLFragmentContentSink.cpp
> nsHTMLParanoidFragmentSink::NameFromNode(const nsIParserNode& aNode,
>                                          nsIAtom **aResult)
> {
>   nsresult rv;
>   eHTMLTags type = (eHTMLTags)aNode.GetNodeType();
>   
>   *aResult = nsnull;
>   if (type == eHTMLTag_userdefined) {
>-    nsCOMPtr<nsINodeInfo> nodeInfo;
>-    rv =
>-      mNodeInfoManager->GetNodeInfo(aNode.GetText(), nsnull,
>-                                    kNameSpaceID_XHTML,
>-                                    getter_AddRefs(nodeInfo));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    NS_IF_ADDREF(*aResult = nodeInfo->NameAtom());
>+    nsCOMPtr<nsIAtom> atom = do_GetAtom(aNode.GetText());
>+    atom.forget(aResult);
>   } else {
>     rv = NameFromType(type, aResult);
>   }
>   return rv;
> }

Thanks. I've been looking for someone to prove that "uninitialized variable" warnings are useful for a while.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm...  Bug filed on that?  We need to fix it on aurora, right?  Or beta too?  Wish this bug had a target milestone set.... :(