116551 - De-COMtaminate nsINodeInfo

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

There's no need for nsINodeInfo to be [XP]COM clean, and there are performance
reasons for breaking the [XP]COM cleanness (virtual method calls where we could
inline trivial things n' such). Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 2

[Johnny Stenback (:jst)]

Attachment: More inlining fun...

Comment 3

[Chris Waterson]

*** Bug 105430 has been marked as a duplicate of this bug. ***

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Is there any need at all for ns*I*NodeInfo?

Comment 5

[Chris Waterson]

Why does nsINodeInfo need any pure virtual methods at all? Cross-library calls?

Comment 6

[Johnny Stenback (:jst)]

Yes, nsINodeInfo needs to be callable across libraries, there are several
libraries that use nsINodeInfo, and I don't think we want to, nor do I think it
makes sense to change all those.

Whether or not we need ns*I*NodeInfo is not clear, but we do need *a* interface
with abstract methods, but we could get rid of nsINodeInfo in favor of
nsNodeInfo, or something like that (which would mean renaming of nsNodeInfo to
nsNodeInfoImpl or somesuch). Would that be preferred?

Comment 7

[Brendan Eich [:brendan]]

nsAFoo seems to be preferred for abstract but not XPCOM class names.

/be

Comment 8

[Johnny Stenback (:jst)]

True, but I forgot to mention that nsXNodeInfo thingies need to be refcounted,
and being able to use nsCOMPtr<>'s for helping with that is really nice, so we
don't want to loose nsISupports from the inheritance chain. Dbaron, did you
start working on a non-nsISupports based auto refcounting helper type a'la
nsCOMPtr already (IIRC you did something like that)? Is there such a thing
available for use today or sometime soon? If not, we could leave nsINodeInfo
until we have a different solution, or we could create a "class nsANodeInfo :
public nsISupports {...}" (ew, weird).

Either way, the attached patch should get rid of the CPU cache misses waterson
reported that were often caused by calls to the virtual nsINodeInfo::Equals()
methods. I'm happy to either check this in as-is and get rid of nsINodeInfo once
a cleaner solution is available (this way these changes won't bit-rot), or we
could wait for a non-nsISupports based refcount helper class and go with
nsANodeInfo directly. I'd prefere the former.

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Just checking this in seems fine with me.  My auto-pointer stuff is working as
far as the tests I've written show, but I need to write more tests and make sure
it works on platforms other than gcc3, VC++ 6.0, and CW7.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Would there need to be any uses of nsINodeInfo outside of content if
nsIElementFactory had a version of |CreateInstanceByTag| that took as parameters
the information needed to construct the nodeinfo object rather than the object
itself?

Comment 11

[Johnny Stenback (:jst)]

Yes, nsINodeInfo makes a lot of things easier, and it's also the only non-string
based way of accessing the prefix of an element, something I'd imagine being
very important to code like transformiix. Sure, we could change the
nsIElementFactory API, but even if we do, nsINodeInfo should be useable across
XPCOM libraries...

Comment 12

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

dude! i missed this entire discussion :)

should have this fully reviewed soon, so far it looks good except that you 
remove a lot of NS_ENSURE_TRUE() where things really need to be true. How about 
NS_ASSERTION at least?

Comment 13

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

actually, once we have that non-nsISupports refcount baseclass we shouldn't 
need ns(I|A)NodeInfo at all i think, shouldn't just nsNodeInfo be enough? That 
way we wouldn't need a vtable pointer at all, right?

Comment 14

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

Why have the nsNodeInfoInner struct? Why not place mName, mPrefix and 
mNamespaceID directly in nsINodeInfo? I guess that part will be redone once 
there no longer is an nsINodeInfo "interface" though?

in nsNodeInfo::Equals(const nsAString& aName, const nsAString& aPrefix), you 
need to check if mInner.mPrefix is set even when !aPrefix.IsEmpty()

in nsNodeInfo::Equals(const nsAString& aName, const nsAString& aPrefix, PRInt32 
aNamespaceID), will nsAString::Equals(PRUnichar*) really return PR_TRUE if the 
string is empty and the argument is nsnull?

other then that r=sicking

Comment 15

[Johnny Stenback (:jst)]

Attachment: Fix bugs found by sicking.

Comment 16

[Johnny Stenback (:jst)]

I removed the NS_ENSURE_* and decided not to even add an NS_ASSERTION() since
it'll be obvious what went wrong if we ever crash due to a nsNodeInfo not being
properly initialized, so the assertion wouldn't really add any real value.

nsNodeInfoInner is used for two things, it's used as a hash key, in which case
we don't want anything that's not part of the key to be part of the key
struct/class. Right now, we could probably get rid of nsNodeInfoInner and move
the members out into nsINodeInfo and use an instance of nsINodeInfo as the key,
but I don't like that. Also, in the future we might add more members to
nsINodeInfo (mDocument, mOwnerManager, ...) which are not part of the hash key,
so it's cleaner to leave it in...

As for your question in comment #13, yes, we do need a vtable since we want
other libraries (like layout and probably transformiix) to use ns[A|I]NodeInfo
too, and that won't be possible w/o a vrable unless we inline the whole thing,
which doesn't seem like a good idea.

Comment 17

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

Comment on attachment 63096 [details] [diff] [review]
Fix bugs found by sicking.

r=sicking

Comment 18

[Chris Waterson]

Comment on attachment 63096 [details] [diff] [review]
Fix bugs found by sicking.

sr=waterson

Comment 19

[Johnny Stenback (:jst)]

Fixed.

Comment 20

[Chris Waterson]

Looks like this led to a drop in times across the board (page load, startup, new
window). Nice!

Comment 21

[Cathleen]

woopi!  Thanks Johnny!!!