Closed Bug 70647 Opened 24 years ago Closed 23 years ago

implement brutal sharing for XBL methods and properties

Categories

(Core :: XBL, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.5

People

(Reporter: trudelle, Assigned: hyatt)

References

()

Details

(Keywords: perf, Whiteboard: PDT+)

Attachments

(6 files)

just do it.
Whiteboard: [xbl1.0]
->1.0, per hyatt
Keywords: perf
Target Milestone: --- → mozilla1.0
Blocks: 49141
This is part of one of the worst responsiveness defects we have (slow window display), and is a dependency of one of the most-voted-for nscatfood bugs. Any chance we could make progress on it for 0.9.1?
Priority: -- → P2
Whiteboard: [xbl1.0] → [xbl1.0] wanted for 0.9.1/Mojo beta
I'm going to try to take a crack at this. But I doubt it will be done by .9.1. Hyatt, I was poking at the xul cache and noticed that for a given xul document prototype we cache an xbl document info object. This xbl document info object stores xbl prototype bindings. Is the basic idea behind this bug to extend the xbl document info to store prototypes for XBL methods and properties in addition to the bindings?
Assignee: hyatt → mscott
Target Milestone: mozilla1.0 → mozilla0.9.2
Yeah.
Here's my plan of attack: I'd like to cache a compiled version of each XBL property and method. Of course what context do you compile the JS against in the proto type? Well, I found some code in nsXULDocument/nsXULElement which pre-compiles JS using a "special prototype script object" as the parent. Then when we later want to execute the JS we can re-resolve it against the actual JS context for the document. We've got a little helper class called nsXULPrototypeScript in nsXULElement.cpp which helps manage this. I'm going to do something very similar over in XBL. We'll store compiled versions of the properties and methods on nsXBLPrototypeBinding. Then when we create an actual XBLBinding we'll iterate over all our compiled objects and execute them against the current JS context.
Status: NEW → ASSIGNED
shaver helped do the XUL stuff, IIRC. mscott, you gonna use the same s3kr3t global object for XBL precompilation? /be
Yeah, I'm currently looking at a clean way to pass the xul document prototype's script context down into the xbl binding prototype so I can use it there. Since the XBL is part of the XUL document it seems correct/ok to leverage and share the same prototype's script context.
Actually it looks pretty easy to get the xul prototype document's script context from the xbl prototype binding. I can just go through the xul cache with the document url to get the xul prototype document then QI for the script context owner.
brendan/shaver, I have a JS API question for you guys. I want to pre-compile an XBL method or property against the special prototype script context and then later on install it into context for an actual document. For properties (getters/setters), I want to call JS_DefineUCProperty with the appropriate attribute flags (JSPROP_SETTER, JSPROP_GETTER) passing in my pre-compiled JS object. But a method isn't going to work using this method. Can I use JS_DefineFunction or something similar to incorporate a JS object which is a method into a new context?
JS_DefineUCProperty will work just fine, even for non-getter/setter cases: JS_DefineUCProperty(cx, obj, ucname, uclength, OBJECT_TO_JSVAL(funobj), NULL, NULL, JSPROP_ENUMERATE); given a precompiled method's funobj, where (ucname, uclength) give the method's name, and obj is the target on which the method should be defined, will do the deed. A method is just a function-valued property. /be
Thanks Brendan. One more question for you. Do you know why XBL has a list of JSClass objects attached to the XBL Service? Before we compile a property or method in the current code we attempt to create a JSClass object (or to find a JSClass object in the list maintained by the XBL Service) using a XPCWrapper from the current document and a context from the current document. (see nsXBLBinding::InitClass). This class object is then used as an argument to CompileFunction. I'm a bit fuzzy as to how this will work in a cache system where I want to pre-compile the JS and store it in a prototype. I don't want to create a class object using the xpc wrapper & context for the first document using the xbl widget. It would be easy to use the context for the prototype xul document but I don't believe we have a "special" XPC wrapper. I suspect I'll need to break this process (nsXBLBinding::InitClass) out into two parts. One happens when compiling the function and storing it in a proto type. We'll want to create a JSClass to use. Then when we want to install the proto type into a document, we'll do the last part of this method which generates the native wrapper and has the document add a reference to this native wrapper.
mscott: you got it -- JSClasses are readonly and sharable by any number of objects, and XBL ref-counts them and manages them in an LRU list. You want the wrapper stuff to happen per document, but the JSClass creation can be done once per class name (binding URI). /be
Status update: I've made some really good progress on this over the last couple days. I've got some code which I'm now testing against. Unfortunately I made a flawed assumption when I started. I thought I could always get the special context for the xul document prototype and compile my xbl JS against that. However, you can have xbl without having XUL (i.e the scrollbars that come from an HTML page). So now I've got XBL without a special context to compile it under. This means, I'm going to need to duplicate the code in nsXULPrototypeDocument which implements this special context (in particular nsXULPDGlobalObject). I'm going to have the nsXBLDocumentInfo support nsIScriptGlobalObjectOwner and it'll have access to a version of nsXULPDGlobalObject which it will hand out to callers of nsIScriptGlobalObjectOwner. Since one can have XBL without XUL and XUL without XBL I believe I need two separate classes to implement the special context for xul and special context for XBL. One positive effect of this is that it removes my "cheat" dependency of XBL on nsIXULPrototypeDocument in order to get the prototype document's nsIScripgGlobalObjectOwner.
Well I have the design in place and now it's just a matter of getting the code to work. =). I don't believe I'm precompiling the JS correctly. If i have a simple xbl widget with the following property: <implementation> <!-- public implementation --> <property name="checked" onset="dump('hello');" onget="dump('goodbye'); return true"/> </implementation> and I call var mybool = widget.checked; the property is correctly getting called and the JS is getting executed. But I get an error about "dump" not being defined when we execute the getter for the property. Same thing if I have something more complex like this.getAttribute. Still poking. I'll post some sample code illustrating how I'm creating the classObject for a binding, and precompiling methods and properties on this classObject. I'll also post code showing how I get a script object from the final JSContext and a native wrapper. The native wrapper is then added to the final document.
mscott: JS functions (including getters and setters) are statically scoped. That means when called, their scope chain goes through the object in which their source declarations were evaluated (your secret proto-global). That object has no dump function, clearly. The solution JS and XBL use, each for its own internally-detectable cases where the precompiled scope isn't the same as the execution context scope, is to clone a precompiled function object and give the clone the right scope parent (which is the object in which the clone is being made _in lieu_ of recompiling its declaration is if it were loaded in that object). Why isn't that happening for you? Grep and breakpoint JS_CloneFunctionObject. /be
Brendan: JS_CloneFunctionObject isn't being called because in my ignorance I was being ignorant and not calling it before attempting to call JS_DefineUCProperty with the compiled JS. So no resolution against the actual context was happening. With that improvement, now simple XBL properties are now working (my little dump example). I'm still failing on calls to this.setAttribute/this.getAttribute with errors about this.getAttribute not being defined. I just need to sit back and crank through the debugger a little more. Thanks again for all the help.
DOM Node.prototype not on the prototype chain of the target object to which you are binding? Hmm. Cc'ing jst&jband. /be
Yeah it does look like I've lost the dom node prototypes. 'this' is properly resolving to a XUL Element. dom node calls on that xul element are failing. Anything in particular I need to do to make sure the dom prototype is installed?
Yup, Node.prototype is no longer in the prototype chain of DOM nodes, the prototype chains set up by XPConnect and the DOM helpers right now is quite shallow, it's basically instance -> xpcwrapped_proto -> Object, always. I'm guessing we need to change this, but I'm not sure what the best approach is yet.
jst, can I somehow force the dom prototype to get installed on my node when I'm binding the xbl protototype to a real dom element?
adding PDT+
Whiteboard: [xbl1.0] wanted for 0.9.1/Mojo beta → [xbl1.0][PDT+] wanted for 0.9.1/Mojo beta
mscott, how are you doing on this?? will you be able to land it by tomorrow?
No I won't, I'm in the same waiting pattern I was a couple weeks ago. We are blocked waiting for Johnny to make his changes to xpconnect and the dom to make sure the dom prototype is installed on prototype chains built by xpconnect. He's been on vacation for a while and just got back Thursday.
I have the DOM constructor/prototype stuff working in my tree now, I'll need to do some more testing and lots of cleanup before it's ready for checkin, but it shouldn't be too far away any more now...
jst, what's the bug number to your DOM constructor/prototype changes?? can we link it to this bug?? :-)
Cathleen, see bug 83433.
Depends on: 83433
Please add status whiteboard comment for eta. Seems like it might be better to get this all built somewhere and well tested and then come into the limbo build instead of rushing in today or tomorrow. Switch the PDT+ to nsBranch keyword if you are on that path.
Whiteboard: [xbl1.0][PDT+] wanted for 0.9.1/Mojo beta → [xbl1.0][PDT+] need eta (limbo instead?)
Taking off 092 radar, adding nsBranch keyword.
Keywords: nsBranch
Whiteboard: [xbl1.0][PDT+] need eta (limbo instead?) → [xbl1.0]
moving to 0.9.3 for nsbranch
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Just some numbers. I quantified opening 10 empty windows with CTRL-n. XBL-related: 16% of the total time is in nsXBLService::LoadBindings. (Is that what this bug is attacking?) of that time, 38% is in nsJSContext::CompileFunction 24% is in nsJSContext::EvaluateStringWithValue 15% is in nsXBLBinding::GenerateAnonymousContent 8% is in nsXBLBinding::InitClass 7% is in nsXBLService::LoadBindingDocumentInfo It would be really nice if the sharing can remove all this without adding too much.
removing nsbranch keyword.
Keywords: nsBranch
now that the blocker bug is resolved... :-) it would be nice to put in some effort to nail this one down!
I started working on this again last Friday. I'm still working on it.
mscott, I have cycles free now if you want to hand this back to me.
moving to 0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Blocks: 91351
Daniel, yes, this is what we're attacking.
Blocks: 94199
I landed the XBL brutal sharing patch on a branch I cut last night. The name of the branch is: XBL_BRUTAL_SHARING_20010807_BRANCH I've only landed the project changes for windows right now. I'll land linux makefile changes tonight and the mac hopefully tomorrow. feel free to pull and try it out. I'll work with release to spin up a set of QA builds to test and do performance measurements on. This patch should cut into the 10% of new window time spent compiling JS.
Brendan and I are interested in reviewing when you're ready.
I did a test with the xbl-brutal branch, comparing it to a current trunk mozilla build. There's a little controller window that opens 10 browser windows one at a time, then 10 in a row (total 100 windows opened and timed). Each browser window loads a trivial HTML file that lets the opener know when it is done. At any rate, for the test as a whole, the branch build was overall 10% faster on a 500MHz/128MB/win2k machine [not 9 or 11. 10 like you said]. Sweet. Thanks for getting this done mscott! We'll get some tests in on the other platforms with builds tomorrow.
wow. you rock. Thanks for building my branch John. As you just alluded to, leaf is spinning windows and linux builds of the branch today for anyone else whose interested. I have the mac changes in my tree on the trunk but didn't check them into the branch.
Here's a snapshot of the changes between the trunk and the branch. I still have some things to clean up such as inconsistent brace formatting. Here's the basic idea: 1) an XBL prototype binding now has a linked list of prototype properties just like it has a linked list of prototype events. 2) When we first parse an XBL binding, we don't actually compile any of the properties or methods. Intead we hang onto the content node from the xbl binding. When we first have to instantiate an xbl binding of this type, then we parse out the property & pre-compile it. 3) I've created a special script global object (nsXBLPDGlobalObject) which is a clone of the one used by xul documents. Each XBL document has it's own script global object and you can fetch this global object from the xbl document info object. 4) Properties & methods are pre-compiled using this global script object and a class object (which is stored on the prototype binding since each binding uses a single class). So the prototype property gets the class object from the binding and the global script object from the xbl document and pre-compiles the JS. The pre-compiled JS object is stored on the prototype property. 5) Subsequent attempts to install the properties on an xbl binding cause us to rebind the pre-compiled JS against the real script context for the target document.
> 3) I've created a special script global object (nsXBLPDGlobalObject) which is > a clone of the one used by xul documents. Each XBL document has it's own > script global object and you can fetch this global object from the xbl > document info object. I've always hated the name nsXULPDGlobalObject (It meant nsXULPrototypeDocumentGlobalObject, which I found too long -- see bug 28570). It also doesn't seem to make much sense given that there's no nsXBLPrototypeDocument. Is there a better name? Maybe nsXBLSharedGlobalObject? Maybe something shorter? I hate naming things. brendan?
nsXBLDocGlobalObject would probably be ok.
You might want to pull over the modifications to AddJSGCRoot and RemoveJSGCRoot from bug 93146, and perhaps note the duplication at both sites, if not move them to one place usable from both callers. Have you run some leak tests? This raises some interesting possibilities for leaks. (What owns the nsXBLDocumentInfo? What owns the nsXBLPrototypeProperty? Both can break cycles in their destructors.)
sr=hyatt
Branch builds for windows and linux can be found here: ftp://ftp.mozilla.org/pub/mozilla/nightly/latest-XBL_BRUTAL_SHARING
I've been trying out 08/17 16:51 builds from that directory and I have a reproducible crash when visiting www.redhat.com. By comparison, a trunk build from (late) in the same day does not display this problem. I'm not sure how relevant that is since I don't know when the branch was cut... Otherwise I have experienced no other problems after a few hours of browsing with the brutal sharing build. Platform: Windows NT
I just merged the branch onto the trunk and these diffs show the differences. It also includes suggestions from hyatt and brendan to remove some nasty set prototype footwork. I also renamed nsXBLPDGlobalObject to nsXBLDocGlobalObject. I still haven't updated my versions of AddJSGCRoot and RemoveJSGCRoot as dbaron suggested nor have I noted the dependency yet.
There is still one problem. Let me use an example of two bindings A and B, where A extends B. For the two contexts, I'll just use the terms "XBLDoc" and "XULDoc". Let's say I want to attach binding A to an element in a XUL doc. The way it should work is this: Class objects for A and B should be created in the XBLDoc context. A can be connected to B via the prototype chain, but I don't think it really matters at all whether or not the two class objects are connected. Methods, getters and setters for the A binding should be compiled and installed on the class object A. Methods, getters and setters for B should be compiled and installed on the class object B. So far so good. Your patch does this. Now when I want to make the real binding, I have to worry about fixing the real prototype chain and ensuring that all the methods, getters and setters should be installed in the right place. This means that new class objects for A and B need to be made in the XUL doc (if they haven't been defined already). We'll call those A' and B'. The prototype chain for a XUL element, X, is tweaked so that it becomes X -> A' -> B' -> (the js object's original prototype) Still so far so good. Where you get into trouble is with the installation of methods, getters, and setters. If a method is defined on A, it should be installed on A'. If a method/getter/setter is defined on B, it should be installed on B'. If I'm reading the patch correctly, you're still installing all methods/getters/setters on X. Methods/getters/setters should be installed on the corresponding created class object. Raw properties can be installed right on X. So the only bug left that I see has to do with your treatment of methods/getters/setters.
Another optimization that must be retained (that the trunk has) is that once methods/getters/setters have been copied from A to A', you never need to do that again for the XUL doc. The current trunk does this by assuming that if it had to create A', then it had to install the methods/getters/setters for A'. If A' already exists in the XUL doc, then the assumption is that it's already initialized and that you don't need to do the copying from A to A' again.
mscott, I have added a test case in the URL field of the bug. You can browse to the HTML page and follow the instructions to test the code.
Ignore the second test on the page. Looks like the trunk fails it too. Sigh. I'll be filing a bug on jst. :)
thanks for the test case Dave. You are indeed right, my code fails the first test case. I only get the first dialog. I'm still not sure I understand why though. I'm clearly not "getting" something. I still preserve the initialization of parent bindings before I install the properties and methods for the current binding just like the old code. When I look at it, I'm getting the same script object using the same mBoundElement object as the trunk is today. I can't see how the trunk would have a different value for these in nsXBLBinding::InstallProperties(). To help me understand what I'm missing, where do you see in my patch that I'm installing all the methods and properties on X instead of on the parent binding? When I look at it, I'm seeing us call from the InstallProperties for X over to InstallProperties on A which in turn creates a script object and causes the class for A to get initialized just like the old trunk code did. Methods get installed on this script object then we return to our child's nsXBLBinding::InstallProperties method (for X) which also initializes a script object, initializes the class and installs it's properties and methods on the target.
I updated the test page to fix the second test case. Both now work with the trunk (and should work with this patch).
New diffs forthecoming. After a little chat with hyatt we figured out what I was still missing. With these new diffs I now pass both of Dave's test cases.
Unfortunately my changes to fix Dave's test case now caused text fields for passwords to break (text fields themselves work okay, we just get empty strings back if the text field was used for a password =() and it broke some of the xbl in the auto complete widget.
mscott, a guess would be that you're skipping the phase where raw properties need to be installed on X. Make sure your optimization to avoid cloning methods/getters/setters from A to A' more than once doesn't keep you from still installing raw properties on X.
Excellent catch Dave! That is indeed what I was doing. Okay now those two problems are fixed and your test cases also pass. Posting patches for final review.
Too tired to give full review, I will after I sleep. Here are some nits: + static const nsIID& GetIID() { static nsIID iid = NS_IXBLPROTOTYPEPROPERTY_IID; return iid; } Use NS_DEFINE_STATIC_IID_ACCESSOR(NS_IXBLPROTOTYPEPROPERTY_IID) instead (no semi after). +NS_IMPL_ADDREF(nsXBLDocGlobalObject) +NS_IMPL_RELEASE(nsXBLDocGlobalObject) + +NS_INTERFACE_MAP_BEGIN(nsXBLDocGlobalObject) + NS_INTERFACE_MAP_ENTRY(nsIScriptGlobalObject) + NS_INTERFACE_MAP_ENTRY(nsIScriptObjectPrincipal) + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIScriptGlobalObject) +NS_INTERFACE_MAP_END Use NS_IMPL_ISUPPORTS2(nsXBLDocGlobalObject, nsIScriptGlobalObject, nsIScriptObjectPrincipals) instead. +JSBool PR_CALLBACK nsXBLDocGlobalObject_resolve(JSContext *cx, JSObject *obj, jsval id) +{ + if (JSVAL_IS_STRING(id)) { + JSString *str = JSVAL_TO_STRING(id); + jschar *s = ::JS_GetStringChars(str); + } Oops, the above is unused deadwood. Nuke it! + JSBool did_resolve = JS_FALSE; + return JS_ResolveStandardClass(cx, obj, id, &did_resolve); +} +NS_IMETHODIMP +nsXBLDocumentInfo::ReportScriptError(nsIScriptError *errorObject) +{ + nsresult rv; + + if (errorObject == nsnull) + return NS_ERROR_NULL_POINTER; + + // Get the console service, where we're going to register the error. + nsCOMPtr<nsIConsoleService> consoleService (do_GetService("@mozilla.org/consoleservice;1")); + + if (consoleService != nsnull) { + rv = consoleService->LogMessage(errorObject); + if (NS_SUCCEEDED(rv)) { + return NS_OK; + } else { + return rv; + } + } else { + return NS_ERROR_NOT_AVAILABLE; + } else after return is a non-sequitur, and maybe this if/if-else statement could look better as just if-if with early returns: if (consoleService == nsnull) return NS_ERROR_NOT_AVAILABLE; rv = consoleService->LogMessage(errorObject); if (NS_FAILED(rv)) return rv; return NS_OK; + nsCOMPtr<nsIXBLPrototypeProperty> curr = propertyChain; + + if (!propertyChain) return NS_OK; // kick out if our list is empty + Revise the nsCOMPtr initialized decl and the if (!propertyChain) test? Use if (!ptr) rather than if (ptr == nsnull), see above. + while (curr) + { Use a do-while loop, we know curr is non-null here the first time through the loop. + // mscott: we really don't want to return a class object here...this breaks an optimization... + // i need to investigate this more. + //*aClassObject = (void *) proto; What's the story, here? + // We want to pre-compile the properties against a "special context". Then when we actual bind + rv = aContext->CompileFunction(mClassObject, + nsCAutoString("onget"), + 0, + nsnull, + getter, + functionUri.get(), + 0, + PR_FALSE, + &mJSGetterObject); + if (NS_FAILED(rv)) return NS_ERROR_FAILURE; + mJSAttributes |= JSPROP_GETTER | JSPROP_SHARED; + if (mJSGetterObject) Oops, 3rd and 2nd to last lines overindented by one space. Lines above of underhanging actual args are underindented, need another space. + // if we came through all of this without a getter or setter, look for a raw + // literal property string... + if (mJSSetterObject || mJSGetterObject) + return NS_OK; + else + { + return ParseLiteral(aContext, aPropertyElement); + } // if we have a literal string else after return is pointless (so are the braces around the else's return). + if (mJSMethod) + { + // Root the compiled prototype script object. + JSContext* cx = NS_REINTERPRET_CAST(JSContext*, + aContext->GetNativeContext()); + if (!cx) return NS_ERROR_UNEXPECTED; + + rv = AddJSGCRoot(cx, &mJSMethod, "nsXBLPrototypeProperty::mJSMethod"); + if (NS_FAILED(rv)) return rv; + } + } + + for (PRUint32 l = 0; l < argCount; l++) + nsMemory::Free(args[l]); Four space indentation after 2, and the modeline says 2. Sorry, I'll sleep and give a real review. Feel free to attach a nit-picked patch -- the only other nit is that opening brace in hyatt code goes on the same line as if, for, while, etc. but sometimes we phase-shift to mscott-land and back, within the same function. :-) /be
Attached patch revised patch w/ Brendan's nits (deleted) — Splinter Review
Thanks for starting to take a look at this patch Brendan. I just posted a patch that should address the nits you listed above. The one thing I wanted to comment about: + // mscott: we really don't want to return a class object here...this breaks an optimization... + // i need to investigate this more. + //*aClassObject = (void *) proto; >What's the story, here? That's some debugging code I just took out. I was having some problems earlier this afternoon when I didn't return a class object. it's all better now. +NS_IMPL_ADDREF(nsXBLDocGlobalObject) +NS_IMPL_RELEASE(nsXBLDocGlobalObject) + +NS_INTERFACE_MAP_BEGIN(nsXBLDocGlobalObject) + NS_INTERFACE_MAP_ENTRY(nsIScriptGlobalObject) + NS_INTERFACE_MAP_ENTRY(nsIScriptObjectPrincipal) + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIScriptGlobalObject) +NS_INTERFACE_MAP_END >Use NS_IMPL_ISUPPORTS2(nsXBLDocGlobalObject, nsIScriptGlobalObject, >nsIScriptObjectPrincipals) instead. I changed to NS_IMPL_ISUPPORTS. In my super reviews I always make people change their code to the newer table macro that I used above. I thought that was the "en vogue" macro. Is the older macro the encouraged one to use? This newer patch also contains a small change to nsXBLPrototypeBinding to support weak references. I then modified nsXBLPrototypeProperty to have an explicit weak reference back to the prototype binding. This code introduces (or we already had) some serious XBL leaks. I'm still analyzing it right now but the tip of the ice berg seems to be that the special global script objects are getting leaked because the objects aren't being finalized during GC. I'm still investigating the source of that leak.
> I changed to NS_IMPL_ISUPPORTS. In my super reviews I always make people change > their code to the newer table macro that I used above. I thought that was the > "en vogue" macro. Is the older macro the encouraged one to use? NS_IMPL_ISUPPORTS{0,1,2,...,9,10} expand exactly to the table macros that you used, except they require less typing and they automatically put in the nsISupports entry (using the nsISupports from the first interface given to the macro). It's NS_IMPL_ISUPPORTS (no number) that expands to NS_IMPL_QUERYINTERFACE (no number) that (a) implements QI in a way that will still compile even if the class doesn't inherit from the interface and (b) only handles QI for one interface (which is assumed to be at offset 0) plus nsISupports. At least I'm assuming that's why scc marked it as deprecated.
When you compile a function in the special (nsXBLDocGlobalObject) global object, the function holds a strong (JS has only strong refs) ref to that object via its parent slot. If any XBL prototype method or event handler (I guess those aren't yet brutally shared, but methods are) leaks, its global object will. Of course, a clone of a prototype method's function object will hold the prototype via its proto slot. Is it possible we're leaking? If not, perhaps there's a cycle through XPCOM and the JS GC heap? (Cycles only in the JS GC heap are no problem for the garbage collector.) /be
Never mind the fuss about the leaking. I had re-written NS_NewXBLPrototypeBinding and left an old NS_IF_ADDREF in there by accident. That caused the leak. We are in good shape now. I'm ready for some hardcore r/sr action.
r/sr=hyatt.
+ if (mClassObject) { + *aClassObject = mClassObject; + return NS_OK; + } + + InitClass(aClassName, aContext, aScriptObject, &mClassObject); + *aClassObject = mClassObject; My only concern here, not a bug, just future-proofing or future-debug-aiding, is that InitClass returns nsnull in mClassObject if it has already been called. Of course, the above won't call it again if mClassObject was set. Would it be better if InitClass always returned the class prototype? Or do you want nsnull plus NS_OK to mean "already initialized, you should have saved the first result (as the above code does in mClassObject)"? sr=brendan@mozilla.org in any event. /be
The old code returned null in the class object for that method if the class object was already initialized. It's what I'm used to at any rate. :)
Is this all ready to slam into the tree?
yup, just waiting for the a= from drivers assuming they are willing to take it.
Drivers@mozilla wants us to hold off on this until .9.5 which is sometime next week. I'm going to be out of town all of next week so look for this to landing sometime around september 10th (Monday)
I was proposing at the performance meeting that we might land this on the trunk next week and see how it shakes out. I said it all looks good, but that it's not the sort of patch we take late in a freeze. mscott, you ok with hyatt or me landing it on the trunk after 0.9.4 branches (probably Tuesday) while you're away? /be
I'm okay with it if one of you are willing to run point on handling any regressions the landing may incurr. I hate having to leave someone else in charge of my messes.
milestone tweakage.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
I ran an optimized build on win32, pulled Sunday evening, with the changes in attachment id=47527 applied to it, through the mozilla smoketests at http://www.mozilla.org/quality/smoketests/ [with the ommission of the installer, plugins/java, and SSL). Additionally, I have been using this build to do normal work with bugzilla and using email. I haven't turned up any areas where there was a problem. So, hyatt/brendan: is this good for landing on the .9.5 trunk to bake. I would like to see this in 0.9.4, or barring that, to be in a position to land on that branch post 0.9.4. [If need be, I can run through this testing on Linux, pre-landing].
Keywords: nsbranch
Whiteboard: [xbl1.0] → PDT
taking.+
Assignee: mscott → hyatt
Status: ASSIGNED → NEW
Dave, I'm back from vacation. If you and brendan didn't go to my machine and check this in, then I can do it now. I'll take it back from you. Since it's not marked fixed, I'm guessing you guys decided not to land it in my absence.
Assignee: hyatt → mscott
mscott: hyatt changed his IRC nickname to "mscott" and did the deed. Thanks for all your work here. On to FastLoading of XBL! /be
oops I see what happened. So it landed but the bug is left open so we can land on the branch. Re-assigning back to Dave since he said he was willing to land it on the branch along with some other xbl changes he had. go go fastloading xbl....
Assignee: mscott → hyatt
Blocks: 99227
nsbranch+
Keywords: nsbranchnsbranch+
I just checked this into the branch. Closing this off.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: PDT → PDT+
verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: