we can save about 3-4000 mallocs on startup by fixing this up. the offenders are: 1) nsID::ToString - this one's pretty hot, and almost all the callers can use a stack string. 2) nsIJSID::GetId - this is in the idl but it's noscript, and only used internally to xpconnect. just handing out a reference to its internal nsID works for almost every caller, so we can just change its semantics to do so, and those that need to can allocate and copy. by fixing this, the ::Equals method avoids mallocs too, and they're both hot methods. this affects nsJSID, nsJSIID, nsJSCID. 3) a bunch of code all over the tree is using nsIInterfaceInfo::GetInterfaceIID and GetName when they can use GetIIDShared and GetNameShared. for 1), i added a method nsID::ToProvidedString(char (&dest)[NSID_LENGTH])... perhaps a little too strict wrt what callers can pass in, but it does sidestep the whole snprintf-esque "is my string long enough" problem. not too hot on the name either, suggestions are welcome (ToAutoString, CopyString, ToBuffer... or maybe just overload ToString?).

You cannot hand out a reference to the internal nsID unless you ensure that you keep a reference to the ID object around (for threadsafety reasons). I think it would be safer to have a signature: nsIJSID::GetId(nsID* outIID) and have the getter fill in an nsID struct passed by the caller (presumably on the stack).

ok, i can make that change. but i'm curious, i'd like to understand - what are the reasons, specifically? GetId() was never threadsafe to begin with, so if thread-related badness were to happen in the middle of GetId() itself, we could still end up fubar. as it stands with the patch, that reference doesn't last for long... it goes up two or so callers, is used, and done with. of course, if you're worried about someone abusing that reference and storing it somewhere, then i can understand. but given that (as far as i understand it) this method is intended to be xpconnect-internal, do we have to worry about it?

(In reply to comment #2) > but given that (as far as i understand it) > this method is intended to be xpconnect-internal, do we have to worry about it? No, we do not -- if xpconnect-internal means what it says. We should use the most efficient code we can within a module such as xpconnect. Great work dwitte! /be

Comment on attachment 294898 [details] [diff] [review] patch v1 > /** > * A "unique identifier". This is modeled after OSF DCE UUIDs. > * @status FROZEN > */ > > struct nsID { Er, you don't care about @status FROZEN ?

not in this case, since we're not breaking ABI compat with this change, and adding a method to the API won't harm anyone. (and no-one's gonna be reimplementing nsID with their own version...) however, if people disagree, we could easily implement this as a class-external helper function.

a few minor tweaks (most notably, fixed crash in nsWindowSH::GlobalResolve() where we weren't holding strong refs to the owners of the nsID and name string long enough). i've audited all the places where these lifetimes are important, and i'm pretty sure they're all correct. the only remaining one i'm not positive about is here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp&rev=1.169&mark=1963-1972#1959 xpc_JSObjectToID now returns a weak |iid| pointer, which is consumed by the QueryInterface call - what does that AutoJSSuspendRequest do in between? if it can result in the destruction of |obj|... anyway, this is ready for review. mrbkap, i hope you don't mind me tagging you for this one :)

Yea - we want this for b3

Comment on attachment 295009 [details] [diff] [review] patch v1.1 - In nsJSCID::GetService(): - nsID iid; + const nsID* iid; // If an IID was passed in then use it if(argc) { JSObject* iidobj; jsval val = *argv; - nsID* piid = nsnull; if(JSVAL_IS_PRIMITIVE(val) || !(iidobj = JSVAL_TO_OBJECT(val)) || - !(piid = xpc_JSObjectToID(cx, iidobj))) + !(iid = xpc_JSObjectToID(cx, iidobj))) { return NS_ERROR_XPC_BAD_IID; } - iid = *piid; - nsMemory::Free(piid); } else - iid = NS_GET_IID(nsISupports); + iid = &NS_GET_IID(nsISupports); This block of code looks identical to what exists in CreateInstance(). Wanna either break this out into a separate function here (GetIIDArg()?), or file a followup bug? - In nsScriptSecurityManager::CanCreateInstance() etc: - nsXPIDLCString cidStr; - cidStr += aCID.ToString(); + char cidStr[NSID_LENGTH]; + aCID.ToProvidedString(cidStr); Nice leak fix! :) r+sr=jst (mrbkap, if you have spare cycles don't let me stop you from reviewing this, before or after it lands).

landed, with a few fixes to init some |const char*| variables to null - nsXPIDLCString does that, and the consumer code depended on it since it ignores rv's. (some of those methods on nsIInterfaceInfo would *really* be better deCOM'ed...) looks like this reduced A numbers by a few thousand on the tinderbox tests.