Closed
Bug 116551
Opened 23 years ago
Closed 23 years ago
De-COMtaminate nsINodeInfo
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla0.9.8
People
(Reporter: jst, Assigned: jst)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [HAVE FIX])
Attachments
(1 file, 2 obsolete files)
31.96 KB,
patch
|
sicking
:
review+
waterson
:
superreview+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.8
Assignee | ||
Comment 2•23 years ago
|
||
Attachment #62627 -
Attachment is obsolete: true
Comment 3•23 years ago
|
||
*** Bug 105430 has been marked as a duplicate of this bug. ***
Is there any need at all for ns*I*NodeInfo?
Comment 5•23 years ago
|
||
Why does nsINodeInfo need any pure virtual methods at all? Cross-library calls?
Assignee | ||
Comment 6•23 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?
Comment 7•23 years ago
|
||
nsAFoo seems to be preferred for abstract but not XPCOM class names. /be
Assignee | ||
Comment 8•23 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•23 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•23 years ago
|
||
Attachment #62673 -
Attachment is obsolete: true
Assignee | ||
Comment 16•23 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•23 years ago
|
Summary: De-COMtamify nsINodeInfo → De-COMtaminate nsINodeInfo
Comment 18•23 years ago
|
||
Comment on attachment 63096 [details] [diff] [review] Fix bugs found by sicking. sr=waterson
Attachment #63096 -
Flags: superreview+
Assignee | ||
Comment 19•23 years ago
|
||
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 20•23 years ago
|
||
Looks like this led to a drop in times across the board (page load, startup, new window). Nice!
Comment 21•23 years ago
|
||
woopi! Thanks Johnny!!!
Updated•18 years ago
|
Flags: in-testsuite-
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•