750733 - Use handles in API object hooks where possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

The JS API functions and function pointer signatures should take handles where appropriate (generally when the function can allocate GC things).  This has two benefits:

- Improves efficiency due to fewer stack rooters being created at the roots of the API functions.

- Makes it easier for API users to write code that roots things correctly, as calling the methods without passing in a rooted location will give a compile error.

The old, non-handle API should still be an option for (non-Firefox) API users, however.  Handles are C++ only, and requiring all API users to use them would freeze out C embedders.  So instead, the handle API should be opt-in with a --enable-handle-api or something (or, eventually, tied to generational/moving GC since if the option isn't enabled then the conservative scanner would still be used) that changes appropriate parameters from pointers into handles (the C++ code using them is the same either way).

Comment 1

[David Mandelin [:dmandelin]]

(In reply to Brian Hackett (:bhackett) from comment #0)

I was talking about this with Terrence last Friday. I want this too.

> The old, non-handle API should still be an option for (non-Firefox) API
> users, however.  Handles are C++ only, and requiring all API users to use
> them would freeze out C embedders.  

Just switching to a C++ API is a live option:

- I hear most embedders can compile with C++. We'd want to check on MongoDB.
- We're about to put out a source released based on ESR10, which could serve as a "last C API" version.
- It kind of makes sense that as we release a new GC, we will switch to a handle API to keep it sane.

An option one step back from that is to put out a source release based on ESR17 that has the C API, and go to C++ after that. We'd probably still want optional handles before then, though.

> So instead, the handle API should be
> opt-in with a --enable-handle-api or something (or, eventually, tied to
> generational/moving GC since if the option isn't enabled then the
> conservative scanner would still be used) that changes appropriate
> parameters from pointers into handles (the C++ code using them is the same
> either way).

We talked about a related option on Friday: add just enough C++ API to connect Firefox with a handle API. It could go in jsfriendapi.h or something like that.

As we put handles into the API, I think we should fix the names. To me:

- What is now named |Handle<T>| would be better named |Rooted<T>|, because then having a |Rooted<T>| means that you have the address of a T that has been rooted.
- What is now named |RootedVar<T>| might be better named |Handle<T>|, which seems to be the common usage for that sort of thing. But RootedVar makes a lot of sense too.

But the API concepts will may change over time, so I'm not sure those are the right names to go with. Random questions about that:

- What's the ideal API for GGC?
- Do we need Root<T> for the future? It seems like the kind of thing we basically don't want for GGC. So is it something temporary for use by the rooting analysis, or something more?
- Does V8 have an analogue to the Handle/RootedVar split, or do they just have one concept? How do they name them?

Comment 2

[Brian Hackett [Laid off!]]

(In reply to David Mandelin from comment #1)
> We talked about a related option on Friday: add just enough C++ API to
> connect Firefox with a handle API. It could go in jsfriendapi.h or something
> like that.

I think this runs into problems when dealing with function pointers.  The API uses these extensively and they are so fundamental to how the internals of e.g. objects work that it seems hard to put a shim in place for them.  e.g.

typedef JSBool
(* JSPropertyOp)(JSContext *cx, JSObject *obj, jsid id, jsval *vp);

Should take a handle for the object and id at least.

Comment 3

[David Mandelin [:dmandelin]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> (In reply to David Mandelin from comment #1)
> > We talked about a related option on Friday: add just enough C++ API to
> > connect Firefox with a handle API. It could go in jsfriendapi.h or something
> > like that.
> 
> I think this runs into problems when dealing with function pointers.  The
> API uses these extensively and they are so fundamental to how the internals
> of e.g. objects work that it seems hard to put a shim in place for them. 
> e.g.
> 
> typedef JSBool
> (* JSPropertyOp)(JSContext *cx, JSObject *obj, jsid id, jsval *vp);
> 
> Should take a handle for the object and id at least.

One option would be to cheat and tell users that they can feel free to create a rooted from any GCable parameter of these functions. Obviously that's pretty ugly and only good as an intermediate step to a handle API.

How did you want to implement optional handle APIs?

Comment 4

[Brian Hackett [Laid off!]]

(In reply to David Mandelin from comment #3)
> One option would be to cheat and tell users that they can feel free to
> create a rooted from any GCable parameter of these functions. Obviously
> that's pretty ugly and only good as an intermediate step to a handle API.
> 
> How did you want to implement optional handle APIs?

I think the simplest and cleanest way to do this is to just use the HandleObject etc. classes in jsapi.h, and typedef those to e.g. either Handle<JSObject*> or JSObject*, depending on configuration options.

Handles do have subtly different semantics than the underlying type, if the underlying location changes its value during the lifetime of the handle.  This is incorrect usage of the handles, though not (yet) asserted.

This would also need a little work to get things to keep compiling under all configurations --- Handle has instance methods address() and value() which would need to be statics instead, but that's not a great hardship.

Another option bandied about last week was to have a separate set of either-handle-or-underlying-type classes that are just used in the API, but that is more moving parts and seems confusing.  Just using Handles is also preferable if we do end up ditching the C API, as then we'd just be removing the non-handle configuration and no code changes needed.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> (In reply to David Mandelin from comment #3)
> > One option would be to cheat and tell users that they can feel free to
> > create a rooted from any GCable parameter of these functions. Obviously
> > that's pretty ugly and only good as an intermediate step to a handle API.
> > 
> > How did you want to implement optional handle APIs?
>  
> Handles do have subtly different semantics than the underlying type, if the
> underlying location changes its value during the lifetime of the handle. 
> This is incorrect usage of the handles, though not (yet) asserted.

What exactly are the differences? Is it just that if the user queries the value twice it's not guaranteed to be the same, or is there more?

> I think the simplest and cleanest way to do this is to just use the
> HandleObject etc. classes in jsapi.h, and typedef those to e.g. either
> Handle<JSObject*> or JSObject*, depending on configuration options.
> 
> This would also need a little work to get things to keep compiling under all
> configurations --- Handle has instance methods address() and value() which
> would need to be statics instead, but that's not a great hardship.

That sounds pretty good. I don't want to change accessor instance methods to statics long-term, but as a transitional step it seems like it shouldn't cause too much trouble.

I forgot to mention last time that I am somewhat concerned about supporting both an exact-rooting and a conservative scanning system--I doubt the non-Firefox one will get much testing or attention. I need more time to think about options but just switching to a C++ API still seems preferable to me, as long as there isn't some big obstacle.
 
> Another option bandied about last week was to have a separate set of
> either-handle-or-underlying-type classes that are just used in the API, but
> that is more moving parts and seems confusing.  

I'm not sure what that means. Would it be a class that holds either a rooted JSObject* (handle) or an unrooted JSObject*, where the API entry points root it if it isn't already rooted?

> Just using Handles is also
> preferable if we do end up ditching the C API, as then we'd just be removing
> the non-handle configuration and no code changes needed.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: hooks WIP (86d28b6fa4fc)

Update the signatures of all the JS class hooks to take handles instead of bare pointers/ids.  Bubbles these handles up within the VM where appropriate, and cleans up some of the millions of do-stuff-to-this-object methods floating around (though it's still pretty messy).  Compiles in the shell, browser needs work next.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: hooks patch (86d28b6fa4fc)

Green on try.  Dave, can you look at the PolyIC changes (changes to pass Handles to JSPropertyOp hooks).  Bill, can you skim the rest (huge amounts of s/JSObject*/HandleObject/ and s/jsid/HandleId/).

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 623419 [details] [diff] [review]
hooks patch (86d28b6fa4fc)

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

That PolyIC thing is kinda gross. Why couldn't you write those pointers to the C stack instead of the VM stack?

::: js/src/jsobj.h
@@ +117,5 @@
>  
>  typedef Vector<PropDesc, 1> PropDescArray;
>  
> +/*
> + * The baseops namespace encapsulate the default behavior when performing

encapsulate -> encapsulates

::: js/src/jsscopeinlines.h
@@ -311,5 @@
> -     * |with (it) color;| ends up here, as do XML filter-expressions.
> -     * Avoid exposing the With object to native getters.
> -     */
> -    if (obj->isWith())
> -        obj = &obj->asWith().object();

I don't know enough to understand what's going on here. Why can this be removed?

::: js/src/methodjit/PolyIC.cpp
@@ +1126,5 @@
>          RegisterID cxReg = Registers::ArgReg0;
>  #endif
>          masm.loadPtr(FrameAddress(offsetof(VMFrame, cx)), cxReg);
>  
> +        /* Grap registers for parameters. */

Grap?

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #8)
> ::: js/src/jsscopeinlines.h
> @@ -311,5 @@
> > -     * |with (it) color;| ends up here, as do XML filter-expressions.
> > -     * Avoid exposing the With object to native getters.
> > -     */
> > -    if (obj->isWith())
> > -        obj = &obj->asWith().object();
> 
> I don't know enough to understand what's going on here. Why can this be
> removed?

obj is not used after this point, this was just cleanup.

> ::: js/src/methodjit/PolyIC.cpp
> @@ +1126,5 @@
> >          RegisterID cxReg = Registers::ArgReg0;
> >  #endif
> >          masm.loadPtr(FrameAddress(offsetof(VMFrame, cx)), cxReg);
> >  
> > +        /* Grap registers for parameters. */
> 
> Grap?

Doh, that's what I get for copy-paste, we were grapping another register lower down.

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc7462dd394

Comment 11

[Matt Brubeck (:mbrubeck)]

Backed out because of assertions in test_Scriptaculous.html on Android:
https://hg.mozilla.org/integration/mozilla-inbound/rev/932a19f737d9

Comment 12

[Brian Hackett [Laid off!]]

Round 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/32cfab3a6aa5

The android orange was because the return value of the getter stubs was being placed in a different slot from where the rejoin code expected to find it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Original landing and backout:
https://hg.mozilla.org/mozilla-central/rev/5fc7462dd394
https://hg.mozilla.org/mozilla-central/rev/932a19f737d9

We'll pick up the re-landing with the next inbound-->m-c merge.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Brian Hackett (:bhackett) from comment #12)
> Round 2:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/32cfab3a6aa5

https://hg.mozilla.org/mozilla-central/rev/32cfab3a6aa5

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Pages like

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JSNewResolveOp

still need to be updated for this change.