De-COMtaminate nsINodeInfo

RESOLVED FIXED in mozilla0.9.8

Status

()

defect
RESOLVED FIXED
18 years ago
a month ago

People

(Reporter: jst, Assigned: jst)

Tracking

(Blocks 1 bug, {perf})

Trunk
mozilla0.9.8
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
Posted patch Proposed fix. (obsolete) — Splinter Review
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 2

18 years ago
Posted patch More inlining fun... (obsolete) — Splinter Review
Attachment #62627 - Attachment is obsolete: true

Comment 3

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

Updated

18 years ago
Blocks: deCOM
Is there any need at all for ns*I*NodeInfo?

Comment 5

18 years ago
Why does nsINodeInfo need any pure virtual methods at all? Cross-library calls?
(Assignee)

Comment 6

18 years ago
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?
nsAFoo seems to be preferred for abstract but not XPCOM class names.

/be
(Assignee)

Comment 8

18 years ago
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.
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.
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?
(Assignee)

Comment 11

18 years ago
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...
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?
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?
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
(Assignee)

Comment 15

18 years ago
Attachment #62673 - Attachment is obsolete: true
(Assignee)

Comment 16

18 years ago
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 on attachment 63096 [details] [diff] [review]
Fix bugs found by sicking.

r=sicking
Attachment #63096 - Flags: review+
(Assignee)

Updated

18 years ago
Summary: De-COMtamify nsINodeInfo → De-COMtaminate nsINodeInfo

Comment 18

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

sr=waterson
Attachment #63096 - Flags: superreview+
(Assignee)

Comment 19

18 years ago
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 20

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

Updated

18 years ago
Keywords: perf

Comment 21

18 years ago
woopi!  Thanks Johnny!!!

Updated

13 years ago
Flags: in-testsuite-
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.