1131789 - Root the parent argument to NewObjectWithProto

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 4.1_handlify_parent_arg-v0.diff

We did not do this before because it's a total pain. Three years on and the pain is much less, now that most things are rooted. This patch adds 3 new roots. We can get rid of these pretty easily if they turn out to be problematic, it was just more code to do so than not.

If we can root proto too, we can get rid of a ton of whacky gc trigger nonsense in the new object cache.

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8562338 [details] [diff] [review]
4.1_handlify_parent_arg-v0.diff

Review of attachment 8562338 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobjinlines.h
@@ +497,5 @@
>   * default to the prototype's global if the prototype is non-null.
>   */
>  JSObject *
> +NewObjectWithGivenProto(ExclusiveContext *cx, const Class *clasp, TaggedProto proto,
> +                        HandleObject parent, gc::AllocKind allocKind, NewObjectKind newKind);

Real Soon Now, we're going to make it so parent is always the global, right? At that time, the parent parameter will go away entirely. But in the meantime, this is *almost* always the global. How nasty would it be to have this take a HandleGlobalObject instead of HandleObject? Then in the few cases where it *isn't* a global, we could have a FIXME comment explaining why we're reinterpret_casting to GlobalObject* when it ain't so.

I suspect that would be substantially more pain, but I don't know. The intermediate solution would be to have an overload for each type. That's not all that pretty either.

::: js/src/vm/Debugger.cpp
@@ +3889,5 @@
>      /* Add all events to the array. */
>      uint32_t index = 0;
>      for (EventEntry *eventItem = events; eventItem < events + num; eventItem++, index++) {
> +        RootedObject item(cx, NewObjectWithGivenProto(cx, &PlainObject::class_, nullptr,
> +                                                      GlobalObject::upcast(cx->global())));

Is there a difference between this and passing in NullPtr()? I don't think there is.

NewObjectWithClassProtoCommon:

    if (!parentArg)
        parentArg = cxArg->global();

@@ +5771,5 @@
>  
>      RootedNativeObject argsobj(cx);
>      if (frame.hasArgs()) {
>          /* Create an arguments object. */
>          Rooted<GlobalObject*> global(cx, &args.callee().global());

Huh? When is this not cx->global()? (Sorry, not your problem.)

::: js/src/vm/GlobalObject.h
@@ +216,5 @@
> +     * is not related to Handle<S>, independent of how S and T are related.
> +     * Further, Handle stores an indirect pointer and, again because of C++'s
> +     * semantics, T** is not related to S**, independent of how S and T are
> +     * related. Since we know that this specific case is safe, we provide a
> +     * manual downcast operation here to do the reinterpret_cast in a

we provide a manual downcast operation here, called "upcast"? :-)

Comment 2

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #1)
> Comment on attachment 8562338 [details] [diff] [review]
> 4.1_handlify_parent_arg-v0.diff
> 
> Review of attachment 8562338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsobjinlines.h
> @@ +497,5 @@
> >   * default to the prototype's global if the prototype is non-null.
> >   */
> >  JSObject *
> > +NewObjectWithGivenProto(ExclusiveContext *cx, const Class *clasp, TaggedProto proto,
> > +                        HandleObject parent, gc::AllocKind allocKind, NewObjectKind newKind);
> 
> Real Soon Now, we're going to make it so parent is always the global, right?
> At that time, the parent parameter will go away entirely. But in the
> meantime, this is *almost* always the global. How nasty would it be to have
> this take a HandleGlobalObject instead of HandleObject? Then in the few
> cases where it *isn't* a global, we could have a FIXME comment explaining
> why we're reinterpret_casting to GlobalObject* when it ain't so.
> 
> I suspect that would be substantially more pain, but I don't know. The
> intermediate solution would be to have an overload for each type. That's not
> all that pretty either.

Given how "Real Soon Now" works around here, I'd prefer to take the more honest approach in the meantime.

> ::: js/src/vm/Debugger.cpp
> @@ +3889,5 @@
> >      /* Add all events to the array. */
> >      uint32_t index = 0;
> >      for (EventEntry *eventItem = events; eventItem < events + num; eventItem++, index++) {
> > +        RootedObject item(cx, NewObjectWithGivenProto(cx, &PlainObject::class_, nullptr,
> > +                                                      GlobalObject::upcast(cx->global())));
> 
> Is there a difference between this and passing in NullPtr()? I don't think
> there is.
> 
> NewObjectWithClassProtoCommon:
> 
>     if (!parentArg)
>         parentArg = cxArg->global();

No, no difference. I'm even somewhat confident that that particular overload of NewObjectWithGivenProto ends up calling NewObjectWithClassProtoCommon. I'd prefer to fix that in a followup.

Perf numbers on octane don't show a regression here locally, so I think I'm going to land with just the typo fix (assuming the try run is green).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78e28afa66fc

Comment 3

[Terrence Cole [:terrence]]

Try run above is green, landing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c448634fb6c9

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c448634fb6c9