Closed Bug 1131877 Opened 5 years ago Closed 5 years ago

Root the proto argument to NewObjectWithProto

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Attached patch 5.1_handlify_proto_arg-v0.diff (obsolete) — Splinter Review
This adds a ton of Rooted, but doesn't seem to have a significant impact on octane, so might be fine.

There is one new Rooted that I am worried about for correctness reasons, however. In CodeGenerator.cpp in CreateThisForFunctionWithProtoWrapper, we seem to treat callee and proto identically in the compiler (at least up to the MIR), yet we take one out as an indirect Handle and the other as a direct pointer. Jan, what am I missing? Moreover, what magic do I need to add to make proto usable as a Handle here so we can skip the Rooted constructor.
Attachment #8562459 - Flags: feedback?(jdemooij)
Comment on attachment 8562459 [details] [diff] [review]
5.1_handlify_proto_arg-v0.diff

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

(In reply to Terrence Cole [:terrence] from comment #0)
> In CodeGenerator.cpp in CreateThisForFunctionWithProtoWrapper, we
> seem to treat callee and proto identically in the compiler (at least up to
> the MIR), yet we take one out as an indirect Handle and the other as a
> direct pointer. Jan, what am I missing?

I don't know, maybe we rooted "callee" at some point because the static analysis complained, and didn't bother rooting proto. Using handles for both is much nicer, thanks.

::: js/src/jit/CodeGenerator.cpp
@@ +4601,5 @@
>  }
>  
>  static JSObject *
>  CreateThisForFunctionWithProtoWrapper(JSContext *cx, js::HandleObject callee, JSObject *proto)
>  {

You can just change proto to a HandleObject here (and js::HandleObject -> HandleObject while you're here).

@@ +4608,3 @@
>  }
>  
>  typedef JSObject *(*CreateThisWithProtoFn)(JSContext *cx, HandleObject callee, JSObject *proto);

And change it here as well. That should be enough to make JIT code pass a Handle.
Attachment #8562459 - Flags: feedback?(jdemooij) → feedback+
Wow, that's some impressive tooling.
Attached patch 5.1_handlify_proto_arg-v1.diff (obsolete) — Splinter Review
Tests seem to pass locally, so let's get this in the review queue.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02465374a324
Attachment #8562459 - Attachment is obsolete: true
Attachment #8562909 - Flags: review?(sphink)
Attachment #8562909 - Flags: review?(sphink) → review+
Uhg, so now that I'm rooting the TaggedProto variants I've run into two problems.

First, it looks like somehow a bunch of callers of NewObjectWithGivenProto with an unrooted proto were escaping over to the TaggedProto path. TaggedProto's constructor is explicit, so I'm not sure how this is happening, but I'm having to add a ton of Rooted for what should have been the untagged callers.

Second, NullPtr() converts to both HandleObject and Handle<TaggedProto> so suddenly almost all of the callers are ambiguous.

So I think this all has a good solution, but I'll need to re-base a bit. To disambiguate I've made the Tagged variants use Tagged in the name. According to the comment, only a few Proxy items actually want the Tagged variants, so I think this will be good in practice. But because of 1, my perf measurements were probably way off. 

So, backing up, I'd like to first take the Tagged variants out of the equation by changing the name, then rooting those solidly. Then we can see what the actual impact of rooting proto is going to be. If it's big, I've got some good ideas now for how we can cheat.
As per comment 4, making these take Handle makes NullPtr() ambiguous. The simplest solution is to just give them a different name -- they're used few enough places and we always know exactly when we want this variant anyway.
Attachment #8563057 - Flags: review?(bhackett1024)
This is identical to the one you reviewed before with the addition of a handful of new Rooted, none of them particularly significant. Feel free to skim.
Attachment #8562909 - Attachment is obsolete: true
Attachment #8563059 - Flags: review?(sphink)
Now that we've eliminated the holes, handlifying the TaggedProto variants is as trivial as we expected.
Attachment #8563064 - Flags: review?(sphink)
Blocks: 1132286
Comment on attachment 8563059 [details] [diff] [review]
0.2_handlify_proto_newobj_funcs-v1.diff

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

::: js/src/builtin/Intl.cpp
@@ +438,5 @@
>  static bool
>  intl_availableLocales(JSContext *cx, CountAvailable countAvailable,
>                        GetAvailable getAvailable, MutableHandleValue result)
>  {
> +    RootedObject locales(cx, NewObjectWithGivenProto<PlainObject>(cx, NullPtr(), js::NullPtr()));

Get rid of the js::
Attachment #8563059 - Flags: review?(sphink) → review+
Comment on attachment 8563064 [details] [diff] [review]
0.3_handlify_taggedproto_funcs-v0.diff

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

::: js/src/vm/ObjectGroup.h
@@ +119,5 @@
> +// pointer to a TaggedProto.
> +inline Handle<TaggedProto>
> +AsTaggedProto(HandleObject obj)
> +{
> +    MOZ_ASSERT(sizeof(JSObject*) == sizeof(TaggedProto));

can you use a static_assert instead?
Attachment #8563064 - Flags: review?(sphink) → review+
Attachment #8563057 - Flags: review?(bhackett1024) → review+
You need to log in before you can comment on or make changes to this bug.