230816 - [FIXr]Make <constructor>/<destructor> methods, not handlers

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

That should make perf a little better.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Tada.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Taadaa.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154323 [details] [diff] [review]
Taadaa.

Basic idea: turn the event handlers into methods, but don't define them on the
object.  Instead, add an Execute() hook that can be used to run the functions
in the right context and with the right "this".

Comment 4

[Brian Ryner (not reading)]

Comment on attachment 154323 [details] [diff] [review]
Taadaa.

Looks ok to me.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 154323 [details] [diff] [review]
Taadaa.

>@@ -134,28 +134,28 @@ nsXBLContentSink::FlushText(PRBool aCrea
>   else if (mState == eXBL_InImplementation) {
>     if (mSecondaryState == eXBL_InConstructor ||
>         mSecondaryState == eXBL_InDestructor) {
>       // Construct a handler for the constructor/destructor.
>       // XXXdwh This is just awful.  These used to be handlers called
>       // BindingAttached and BindingDetached, and they're still implemented
>       // using handlers.  At some point, we need to change these to just
>       // be special functions on the class instead.

Hey, you can rip out this XXXdwh comment now! ;-)  And revise the "handler"
word in the first comment sentence to method or whatever.

>@@ -215,8 +221,63 @@ nsXBLProtoImplMethod::CompileMember(nsIS
>     JSContext* cx = NS_REINTERPRET_CAST(JSContext*,
>                                         aContext->GetNativeContext());
>     if (!cx) return NS_ERROR_UNEXPECTED;
>     AddJSGCRoot(&mJSMethodObject, "nsXBLProtoImplMethod::mJSMethodObject");
>   }
>   
>   return NS_OK;
> }
>+
>+nsresult
>+nsXBLProtoImplAnonymousMethod::Execute(nsIContent* aBoundElement)
>+{
>+  // Get the script context the same way
>+  // nsXBLProtoImpl::InstallImplementation does.
>+  nsIDocument* document = aBoundElement->GetDocument();
>+  if (!document) {
>+    return NS_OK;
>+  }
>+
>+  nsIScriptGlobalObject* global = document->GetScriptGlobalObject();
>+  if (!global) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIScriptContext> context = global->GetContext();
>+  if (!context) {
>+    return NS_OK;
>+  }
>+  
>+  JSContext* cx = (JSContext*) context->GetNativeContext();
>+
>+  JSObject * globalObject = ::JS_GetGlobalObject(cx);

Space before and after * in declaration glitch, and another below.

>+
>+  if (mJSMethodObject) {

Ok, why test this here after all that set-up, when you won't use cx or
globalObject if !mJSMethodObject?  Just test that early and bail out if null
with an early return, I say.  Hope I'm not missing some side effect.

Is the null case due to OOM, or some other condition that can happen?  How do
we get into Execute with null mJSMethodObject, I mean (I should know, but
thought I'd ask).

>+    JSObject * method = ::JS_CloneFunctionObject(cx, mJSMethodObject,
>+                                                 globalObject);

Glitchy spacing mentioned above.

>+class nsXBLProtoImplAnonymousMethod : public nsXBLProtoImplMethod {
>+public:
>+  nsXBLProtoImplAnonymousMethod() :
>+    nsXBLProtoImplMethod(EmptyString().get())
>+  {}
>+  
>+  nsresult Execute(nsIContent* aBoundElement);
>+
>+  // Override InstallMember; these methods never get installed as members

But they do get added as members -- worth distinguishing here?	This threw me
on the first read-through and I went back to check that the mConstructor and
mDestructor refs are weak ones into the mMembers list.

>+  virtual nsresult InstallMember(nsIScriptContext* aContext,
>+                                 nsIContent* aBoundElement, 
>+                                 void* aScriptObject,
>+                                 void* aTargetClassObject,
>+                                 const nsCString& aClassStr) {
>+    return NS_OK;
>+  }
>+};

Good to see this patch -- sr=me with above tweaks; thanks.

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Hey, you can rip out this XXXdwh comment now!

Indeed.  And the other comment change.

>  Just test that early and bail out

Seems reasonable.  Will do.

> Space before and after * in declaration glitch

Will fix, here and below.

> Is the null case due to OOM, or some other condition that can happen?

OOM or something like <constructor /> (with explicit end tag, even, as long as
the actual code is the empty string).

> But they do get added as members -- worth distinguishing here?

Sure.  I'll make this comment clearer.

I'll post a patch merged to trunk in about a week (and hopefully land this then
too).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in.  This sped up Txul by about 2%.

Comment 9

[Ronald Tilby]

Tinderbox Linux Brad clbr RLK: went from 268B to 559KB, and Lk: from 305KB to
3.83MB on 10 Sep 04 around the time this patch went in.
Is there a memory leak problem with this patch?

Comment 10

[Boris Zbarsky [:bzbarsky]]

It's possible.  Investigating.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Filed bug 258832 for the memory leak and bug 258833 on a scope chain change that
may break some poorly-coded bindings.

Comment 12

[Bernd]

*** Bug 259276 has been marked as a duplicate of this bug. ***