Closed
Bug 70647
Opened 24 years ago
Closed 23 years ago
implement brutal sharing for XBL methods and properties
Categories
(Core :: XBL, defect, P2)
Core
XBL
Tracking
()
VERIFIED
FIXED
mozilla0.9.5
People
(Reporter: trudelle, Assigned: hyatt)
References
()
Details
(Keywords: perf, Whiteboard: PDT+)
Attachments
(6 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
just do it.
Reporter | ||
Updated•24 years ago
|
Whiteboard: [xbl1.0]
Reporter | ||
Comment 2•24 years ago
|
||
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
Comment 3•24 years ago
|
||
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
Assignee | ||
Comment 4•24 years ago
|
||
Yeah.
Comment 5•24 years ago
|
||
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
Comment 6•24 years ago
|
||
shaver helped do the XUL stuff, IIRC.
mscott, you gonna use the same s3kr3t global object for XBL precompilation?
/be
Comment 7•24 years ago
|
||
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.
Comment 8•24 years ago
|
||
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.
Comment 9•24 years ago
|
||
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?
Comment 10•24 years ago
|
||
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
Comment 11•24 years ago
|
||
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.
Comment 12•24 years ago
|
||
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
Comment 13•24 years ago
|
||
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.
Comment 14•24 years ago
|
||
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.
Comment 15•24 years ago
|
||
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
Comment 16•24 years ago
|
||
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.
Comment 17•24 years ago
|
||
DOM Node.prototype not on the prototype chain of the target object to which you
are binding? Hmm. Cc'ing jst&jband.
/be
Comment 18•24 years ago
|
||
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?
Comment 19•24 years ago
|
||
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.
Comment 20•24 years ago
|
||
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?
Comment 21•23 years ago
|
||
adding PDT+
Whiteboard: [xbl1.0] wanted for 0.9.1/Mojo beta → [xbl1.0][PDT+] wanted for 0.9.1/Mojo beta
Comment 22•23 years ago
|
||
mscott, how are you doing on this?? will you be able to land it by tomorrow?
Comment 23•23 years ago
|
||
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.
Comment 24•23 years ago
|
||
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...
Comment 25•23 years ago
|
||
jst, what's the bug number to your DOM constructor/prototype changes?? can we
link it to this bug?? :-)
Comment 26•23 years ago
|
||
Cathleen, see bug 83433.
Comment 27•23 years ago
|
||
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?)
Comment 28•23 years ago
|
||
Taking off 092 radar, adding nsBranch keyword.
Keywords: nsBranch
Whiteboard: [xbl1.0][PDT+] need eta (limbo instead?) → [xbl1.0]
Comment 30•23 years ago
|
||
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.
Comment 32•23 years ago
|
||
now that the blocker bug is resolved... :-) it would be nice to put in some
effort to nail this one down!
Comment 33•23 years ago
|
||
I started working on this again last Friday. I'm still working on it.
Assignee | ||
Comment 34•23 years ago
|
||
mscott, I have cycles free now if you want to hand this back to me.
Assignee | ||
Comment 36•23 years ago
|
||
Daniel, yes, this is what we're attacking.
Comment 37•23 years ago
|
||
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.
Assignee | ||
Comment 38•23 years ago
|
||
Brendan and I are interested in reviewing when you're ready.
Comment 39•23 years ago
|
||
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.
Comment 40•23 years ago
|
||
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.
Comment 41•23 years ago
|
||
Comment 42•23 years ago
|
||
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.
Comment 43•23 years ago
|
||
> 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?
Assignee | ||
Comment 44•23 years ago
|
||
nsXBLDocGlobalObject would probably be ok.
Comment 45•23 years ago
|
||
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.)
Assignee | ||
Comment 46•23 years ago
|
||
sr=hyatt
Comment 47•23 years ago
|
||
Branch builds for windows and linux can be found here:
ftp://ftp.mozilla.org/pub/mozilla/nightly/latest-XBL_BRUTAL_SHARING
Comment 48•23 years ago
|
||
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
Comment 49•23 years ago
|
||
Comment 50•23 years ago
|
||
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.
Assignee | ||
Comment 51•23 years ago
|
||
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.
Assignee | ||
Comment 52•23 years ago
|
||
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.
Assignee | ||
Comment 53•23 years ago
|
||
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.
Assignee | ||
Comment 54•23 years ago
|
||
Ignore the second test on the page. Looks like the trunk fails it too. Sigh.
I'll be filing a bug on jst. :)
Comment 55•23 years ago
|
||
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.
Assignee | ||
Comment 56•23 years ago
|
||
I updated the test page to fix the second test case. Both now work with the
trunk (and should work with this patch).
Comment 57•23 years ago
|
||
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.
Comment 58•23 years ago
|
||
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.
Assignee | ||
Comment 59•23 years ago
|
||
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.
Comment 60•23 years ago
|
||
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.
Comment 61•23 years ago
|
||
Comment 62•23 years ago
|
||
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
Comment 63•23 years ago
|
||
Comment 64•23 years ago
|
||
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.
Comment 65•23 years ago
|
||
> 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.
Comment 66•23 years ago
|
||
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
Comment 67•23 years ago
|
||
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.
Assignee | ||
Comment 68•23 years ago
|
||
r/sr=hyatt.
Comment 69•23 years ago
|
||
Comment 70•23 years ago
|
||
Comment 71•23 years ago
|
||
+ 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
Assignee | ||
Comment 72•23 years ago
|
||
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. :)
Assignee | ||
Comment 73•23 years ago
|
||
Is this all ready to slam into the tree?
Comment 74•23 years ago
|
||
yup, just waiting for the a= from drivers assuming they are willing to take it.
Comment 75•23 years ago
|
||
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)
Comment 76•23 years ago
|
||
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
Comment 77•23 years ago
|
||
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.
Comment 79•23 years ago
|
||
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].
Comment 81•23 years ago
|
||
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
Comment 82•23 years ago
|
||
Comment 83•23 years ago
|
||
mscott: hyatt changed his IRC nickname to "mscott" and did the deed. Thanks for
all your work here. On to FastLoading of XBL!
/be
Comment 84•23 years ago
|
||
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
Comment 86•23 years ago
|
||
I just checked this into the branch. Closing this off.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•23 years ago
|
Whiteboard: PDT → PDT+
You need to log in
before you can comment on or make changes to this bug.
Description
•