Closed Bug 707717 Opened 8 years ago Closed 7 years ago

XPConnect prototype setup ends up doing JS_SetPrototype

Categories

(Core :: XPConnect, defect)

9 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla12

People

(Reporter: bzbarsky, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Brian ran into this in bug 703047.

Consider the way we set up the prototype for something like HTMLHtmlElement.

We first call into XPCWrappedNativeProto::GetNewOrUsed which calls XPCWrappedNativeProto::Init.  This creates the JSObject for HTMLHtmlElement.prototype using Object.prototype as its proto.  Then it calls PostCreatePrototype on the scriptable helper, if any.  PostCreatePrototype looks up the real proto to use (in this case HTMLElement.prototype) and calls JS_SetProto.

Can we just add a PreCreatePrototype callback whose job will be to hand back the real proto object to use?  Then we can create HTMLHtmlElement.prototype with the right proto up front, right?

Is it worth doing this before the new DOM bindings are coming online?  Peter, Bobby?
Summary: XPConnect prototype setup ends up doing JS_SetProto → XPConnect prototype setup ends up doing JS_SetPrototype
(In reply to Boris Zbarsky (:bz) from comment #0)

> Can we just add a PreCreatePrototype callback whose job will be to hand back
> the real proto object to use?  Then we can create HTMLHtmlElement.prototype
> with the right proto up front, right?

This seems reasonable. There may be snakes in the grass that I can't see, though.

> Is it worth doing this before the new DOM bindings are coming online? 
> Peter, Bobby?

It depends on the rewards, I guess. I'd think that we should direct most of our engineering effort to the new bindings, but if this is blocking some serious ObjShrink speedups or other such things, it may be worth doing.

Peterv will have a better idea of how tricky this would be, which is another important data point.
For what it's worth, this doesn't look terribly tricky to me, offhand.
(In reply to Boris Zbarsky (:bz) from comment #2)
> For what it's worth, this doesn't look terribly tricky to me, offhand.

Ok, then I can take a crack at it in a week or two unless peterv weighs in before then.
Assignee: nobody → bobbyholley+bmo
Comment on attachment 579184 [details] [diff] [review]
Don't dynamically mutate the proto chains of DOM prototypes.

I was halfway through writing this when Bobby grabbed the bug...

Build and runs and seems to avoid calling SetPrototype.  Whether it's correct is tbd (I pushed to try).  Brian, want to see whether this makes things happier on the JS engine end?
Attachment #579184 - Flags: feedback?(bhackett1024)
Attachment #579184 - Flags: review?(peterv)
Attachment #579184 - Flags: review?(bobbyholley+bmo)
Comment on attachment 579184 [details] [diff] [review]
Don't dynamically mutate the proto chains of DOM prototypes.

>+
>+static nsGlobalWindow*
>+FindUsableInnerWindow(nsIXPConnect *xpc, JSContext *cx, JSObject *global)
>+{
>
> ...
> 
>+
>+  return win;
>+}

I'm assuming that there's always something to keep the window alive in any context where we'd use the return value here? I don't imagine it would be a problem, but we're restricting the scope of our COMPtrs here, so I thought I'd mention it.


> 
> NS_IMETHODIMP
> nsDOMClassInfo::PostCreatePrototype(JSContext * cx, JSObject * proto)
> {
>
>...
> 
>-  // This is called before any other location that requires
>-  // sObjectClass, so compute it here. We assume that nobody has had a
>-  // chance to monkey around with proto's prototype chain before this.
>+  // Double-check sObjectClass just in case PreCreatePrototype bailed out early.
>   if (!sObjectClass) {


>+NS_IMETHODIMP
>+nsDOMClassInfo::PreCreatePrototype(JSContext * cx, JSObject * global,
>+                                   JSObject **protoProto)
>+{
>+  *protoProto = nsnull;
>+  
>+  nsGlobalWindow *win = FindUsableInnerWindow(XPConnect(), cx, global);
>+  if (!win) {
>+    return NS_OK;
>+  }
>+
>+  JSObject *winObj = win->FastGetGlobalJSObject();
>+  
>+  // This is called before any other location that requires
>+  // sObjectClass, so compute it here. We assume that nobody has had a
>+  // chance to monkey around with global's prototype chain before this.
>+  if (!sObjectClass) {
>+    FindObjectClass(winObj);
>+    NS_ASSERTION(sObjectClass && !strcmp(sObjectClass->name, "Object"),
>+                 "Incorrect object class!");
>+  }


It's kind of lame that we have to do this twice. Given that it's just a jsclass, it shouldn't matter which global we use to find it, so it seems like we could move this earlier in PreCreatePrototype and then rely on it having been called. This would let us remove it from PostCreatePrototype.

Really though, it seems like we should just wrap all uses of sObjectClass on a static accessor that caches the result. We might need to add a jsapi or jsfriendapi function to get the class we want. Crawling our way to Object.prototype seems suboptimal.

>+  return LookupPrototypeProto(cx, winObj, mData, nsnull, protoProto);
>+}
>+
>+

>+    JSObject *proto;
>+    rv = LookupPrototypeProto(cx, winobj, ci_data, name_struct, &proto);
>+    NS_ENSURE_SUCCESS(rv, rv);

What are the situations where we'll fail to (or decide not to) hook things up in PreCreatePrototype, but successfully do it in PostCreatePrototype. Do we need the PostCreatePrototype stuff at all?

r=bholley, though I'm not the expert there.
Attachment #579184 - Flags: review?(bobbyholley+bmo) → review+
> I'm assuming that there's always something to keep the window alive in any context where
> we'd use the return value here?

Well... the JSObjects involved, right?  This is obviously worth double-checking by someone other than me.

> Crawling our way to Object.prototype seems suboptimal.

Agreed.  I'll look into doing that.

> What are the situations where we'll fail to (or decide not to) hook things up in
> PreCreatePrototype, but successfully do it in PostCreatePrototype.

None, afaict, but PostCreatePrototype is not the only caller of ResolvePrototype.  :(  Luckily, ResolvePrototype does nothing with the proto chain if it's already set up right.
Attachment #579184 - Attachment is obsolete: true
Attachment #579184 - Flags: review?(peterv)
Attachment #579184 - Flags: feedback?(bhackett1024)
Comment on attachment 582358 [details] [diff] [review]
Don't dynamically mutate the proto chains of DOM prototypes.

jst, can you take a look, since it's not clear when Peter will be back and I think we need this for Fx11?
Attachment #582358 - Flags: review?(peterv) → review?(jst)
Comment on attachment 582358 [details] [diff] [review]
Don't dynamically mutate the proto chains of DOM prototypes.

>--- a/dom/base/nsDOMClassInfo.cpp
>+++ b/dom/base/nsDOMClassInfo.cpp
>+LookupPrototypeProto(JSContext *cx, JSObject *winobj,
>+                     const nsDOMClassInfoData *ci_data,
>+                     const nsGlobalNameStruct *name_struct,
>+                     JSObject **aProtoProto)
>+{
>+  if (!ci_data) {
>+    primary_iid = &name_struct->mIID;
>+  }
>+  else if (ci_data->mProtoChainInterface) {

} else if () {, please.
I just copied that, but sure.  Fixed.
(In reply to Boris Zbarsky (:bz) from comment #7)

> 
> None, afaict, but PostCreatePrototype is not the only caller of
> ResolvePrototype.  :(  Luckily, ResolvePrototype does nothing with the proto
> chain if it's already set up right.

I guess I'm wondering if we can kill PostCreatePrototype entirely. I certainly understand if it's more invasive than you want to pursue this close to the branch, though.
PostCreatePrototype does more than hooking up the prototype's proto chain.  It also actually sets up various methods and properties on the prototype, etc, right?

And yes, I'd rather not mess with this before the branchpoint.
(In reply to Boris Zbarsky (:bz) from comment #13)
> PostCreatePrototype does more than hooking up the prototype's proto chain. 
> It also actually sets up various methods and properties on the prototype,
> etc, right?

Oh, right. I forgot entirely about the subclasses.
Depends on: 711557
Comment on attachment 582358 [details] [diff] [review]
Don't dynamically mutate the proto chains of DOM prototypes.

Looks good, r=jst
Attachment #582358 - Flags: review?(jst) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/10593a36b58d
Assignee: bobbyholley+bmo → bzbarsky
Flags: in-testsuite?
Target Milestone: --- → mozilla12
As far as I can tell, the talos Dromaeo effect of this patch is not distinguishable from noise....
https://hg.mozilla.org/mozilla-central/rev/10593a36b58d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
And in fact, the regression finder script claims Dromaeo _regressions_ on CSS/V8/DOM with this patch.

Brian, I'm tempted to back this out until you can confirm what the JS engine side of things looks like with this patch.  Thoughts?
Do you know what is causing the regressions?  The JS engine will always be happier with fewer mutable prototype reassignments, but the workarounds in place in the engine seem to be doing a decent job for the dromaeo tests so I think this will mainly improve behavior on other workloads and allow removal of the workarounds.
> Do you know what is causing the regressions? 

Nope.  And compare-talos still seems to be broken.

> The JS engine will always be happier with fewer mutable prototype reassignments

Well, one important question is whether we actually got there with this patch.  I'll try double-checking that....
OK, I've confirmed that there are no JS_SetPrototype calls from this callsite when I run droameo with this patch applied.

I suppose it's possible that DOM prototype setup itself got slower with this change, because we call into the LookupPrototypeProto code twice and end up actually doing that during the benchmark?

I think I'm going to back this out for now pending the new DOM bindings, which should have much saner prototype setup, I hope.
Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/736d90d02bd4
Assignee: bzbarsky → nobody
Status: RESOLVED → REOPENED
Depends on: 622298
Resolution: FIXED → ---
I'm going to wontfix this in favor of just getting everything on WebIDL and then removing all this code wholesale.
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.