747287 - Codegen the metadata the JIT needs to go fast for getters and setters

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This means the specialized natives those would need to call, as well as whatever else we need to do for bug 747285 step (B).

Comment 1

[Eric Faust [:efaust]]

OK. Here are the changes that need to be made, from my point of view:

With the patch attached to bug 766448, the DOM should just need to define a JITInfo, and insert a pointer to it as the last element in the passed JSPropertySpecs to JS_DefineProperties(), and the rest should be taken care of.

As for what should go into a JITInfo (currently stub defined in jsfriendapi.h, but should be actually defined there), I think the following is not unreasonable:

struct JITInfo {
    js::Native bottom_half;
    uint32_t protoID;
    uint32_t depth;
}

I think the easiest thing for the callback to do is "I have this prototype, and the property told me to tell you about these two numbers (protoid and depth). Is it safe?"

The uint32_t is a cheap hack to get prototype::IDs into the engine while letting the DOM maintain total control of their definition. The engine then just treats it as a magic value and hands it back to the DOMcallback for its consumption. It probably does require using an admittedly unsavory idiom used elsewhere, in which we generate all the values in an anonymous enum and assign them into a typedef'd uint32_t everywhere. I am open to any and all suggestions on this front.

Barring that, we could have a struct containing the depth and the protoid that the engine will just blindly hand back to the callback, then it really has no idea about protoids or depth or anything and we don't have to teach it.

so:

bool SOMENAMEHERE(JSHandleObject proto, uint32_t protoid, uint32_t depth);
OR
bool SOMENAMEHERE(JSHandleObject proto, DOMData_t domData);

This on top of the expected top-half/bottom-half divisions, that should be all we need in order to make this optimization actually happen.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So... We need a different signature for bottom_half.  We don't want a js::Native.  What we actually want is possibly slightly different for getters and setters.  In particular, the getter should look like this:

  bool Gettter(JSContext* cx, void* thisPtr, jsval* vp);

The setter _can_ have that signature (with *vp being the in param and the return value defined to be "undefined") or it can be:

  bool Setter(JSContext* cx, void* thisPtr, jsval arg);

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Codegen bits

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to add a runtime callback to let the JIT interrogate the DOM

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> So... We need a different signature for bottom_half.  We don't want a
> js::Native.  What we actually want is possibly slightly different for
> getters and setters.  In particular, the getter should look like this:
> 
>   bool Gettter(JSContext* cx, void* thisPtr, jsval* vp);
> 
> The setter _can_ have that signature (with *vp being the in param and the
> return value defined to be "undefined") or it can be:
> 
>   bool Setter(JSContext* cx, void* thisPtr, jsval arg);

They can never cause any GC ?

This can easily be done by creating a VMFunction which is well handled by IonMonkey.
So creating one VMFunction for the getter and one for the setter should do it.


(In reply to Eric Faust from comment #1)
> With the patch attached to bug 766448, the DOM should just need to define a
> JITInfo, and insert a pointer to it as the last element in the passed
> JSPropertySpecs to JS_DefineProperties(), and the rest should be taken care
> of.
> 
> struct JITInfo {
>     js::Native bottom_half;
>     uint32_t protoID;
>     uint32_t depth;
> }

I am not sure to understand why there is only one bottom_half for the getter and setter?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> They can never cause any GC ?

They can cause GC.  The attached patch actually uses this signature for both getter and setter:

+typedef
+bool (* JSJitPropertyOp)(JSContext *cx, JSObject* thisObj,
+                         void *specializedThis, JS::Value *vp);

and the callees will assume that vp is rooted, as it is now for JSNatives.

> I am not sure to understand why there is only one bottom_half for the getter and setter?

There isn't.  Attached patch has:

+struct JSJitInfo {
+    JSJitPropertyOp getter;
+    JSJitPropertyOp setter;
+    uint32_t protoID;
+    uint32_t depth;
+};

Comment 7

[Boris Zbarsky [:bzbarsky]]

By the way...  Going forward, we can add more stuff to JSJitInfo.  For example, whether the getter is idempotent, whether it can trigger GC, whatever would help with engine optimizations like LICM and such.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Boris Zbarsky (:bz) from comment #6)
> > They can never cause any GC ?
> 
> They can cause GC.  The attached patch actually uses this signature for both
> getter and setter:
> 
> +typedef
> +bool (* JSJitPropertyOp)(JSContext *cx, JSObject* thisObj,
> +                         void *specializedThis, JS::Value *vp);
> 
> and the callees will assume that vp is rooted, as it is now for JSNatives.

Then this means you need HandleObject and HandleValue.  VmFunction template machinery is used to recover if the function has an out-param or not based on the prototype.

A getter should look like:

typedef
bool (*JSJitPropertyGetOp)(JSContext *cx, HandleObject thisObj,
                           ???? specializedThis, JS::Value *vp);

and a setter:

typedef
bool (*JSJitPropertySetOp)(JSContext *cx, HandleObject thisObj,
                           ???? specializedThis, HandleValue vp);

If the specializedThis is a gc::Cell then you might also need an Handle type which would be rooted each time the GC is marking IonMonkey stack, otherwise we can add a case to support a 'void *' in VMFunction templates.

The out-param is determined by the prototype of the function.  IonMonkey relies on this to simplify the way we make call to any VM functions.

> > I am not sure to understand why there is only one bottom_half for the getter and setter?
> 
> There isn't.  Attached patch has:
> 
> +struct JSJitInfo {
> +    JSJitPropertyOp getter;
> +    JSJitPropertyOp setter;
> +    uint32_t protoID;
> +    uint32_t depth;
> +};

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Then this means you need HandleObject and HandleValue.

OK.  How do I produce a HandleValue out of an existing JS::Value* (of the sort a JSNative has)?

> If the specializedThis is a gc::Cell

It's not; it's a pointer to an underlying C++ object.  Basically, the return value of .toPrivate() on a slot on thisObj.

> The out-param is determined by the prototype of the function.

I'm not sure I follow.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> > Then this means you need HandleObject and HandleValue.
> 
> OK.  How do I produce a HandleValue out of an existing JS::Value* (of the
> sort a JSNative has)?

Usually you need to use something like:

RootedValue val(cx, <JS::Value>);

At least this is what we are doing in the JS engine.  Otherwise the best person to ask would be billm.

On the other hand, I don't think you have to do it since these function will only be called by IonMonkey and IonMonkey is emulating the RootedValue effect.

> > If the specializedThis is a gc::Cell
> 
> It's not; it's a pointer to an underlying C++ object.  Basically, the return
> value of .toPrivate() on a slot on thisObj.

Should we mark these pointers?

efaust: This imply we might need to have a MIRType_Private for unboxing this.

> > The out-param is determined by the prototype of the function.
> 
> I'm not sure I follow.

js/src/ion/VMFunction.h use templates to reverse engineer the prototype of a function and generate the metadata used by IonMonkey to generate a call to this function.  The layout of these functions are:

bool/void  (*)(JSContext*, A1, A2, A3)

where the last argument A3 can be an out-param of the function (T*), in which case the code generated by IonMonkey allocate some space and push the address of the reserved space to the function.

We may still use "const Value *" as type of the last argument — in which case this is not an out-param — but if the function can cause a GC cycle before a read of the Value, this pointer might not be valid anymore.  In such cases, we need a HandleValue to survive the future moving GC.

Comment 11

[Eric Faust [:efaust]]

(In reply to Nicolas B. Pierron [:pierron] from comment #10)
 
> efaust: This imply we might need to have a MIRType_Private for unboxing this.

I'm not sure I follow. The whole point was to remove the cost of dynamically unboxing this value. We are moving the unbox to compile time, and baking in the void * into the compiled CallWithABI().

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I don't think you have to do it since these function will only be called by IonMonkey

Of course not.  My JSNative property setters will be calling them too, so I don't have to duplicate all that code.

> Should we mark these pointers?

You mean in the JS GC sense?  Absolutely not.

> but if the function can cause a GC cycle before a read of the Value, this pointer might
> not be valid anymore.

I don't understand why a _pointer_ to a Value would ever go invalid due to moving GC.  The actual thing stored in the Value might change, of course...

In any case, I'm happy to make this a HandleValue if that's not incurring a performance penalty compared to what happens right now with a JSNative.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> We are moving the unbox to compile time

Uh, no.  You can't do that.  You have to unbox at runtime, because the value depends on the specific object instance!

What's moving to compile time is the verification that the unboxing is safe and whatever sanity-checking is needed to ensure that you're working with a fixed slot and can just generate fast code for that.

Note that "unboxing" a private value out of a fixed slot should be a single load at runtime afaict, assuming you already have the JSObject* in a register.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Boris Zbarsky (:bz) from comment #12)
> > but if the function can cause a GC cycle before a read of the Value, this pointer might
> > not be valid anymore.
> 
> I don't understand why a _pointer_ to a Value would ever go invalid due to
> moving GC.  The actual thing stored in the Value might change, of course...

True, in fact an HandleValue is a JS::Value*, except that this type is expected by functions which can cause some GC.

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Eric Faust from comment #11)
> (In reply to Nicolas B. Pierron [:pierron] from comment #10)
>  
> > efaust: This imply we might need to have a MIRType_Private for unboxing this.
> 
> I'm not sure I follow. The whole point was to remove the cost of dynamically
> unboxing this value.

We generate a MUnbox, which produce code which is executed at runtime.  Which is just loading the payload of a value in one move instruction.

> We are moving the unbox to compile time, and baking in
> the void * into the compiled CallWithABI().

I think you should look at CallVM first.  CallWithABI is the low-level way to make a call and this might be more difficult to handle.  If we have some performance issue related to VM function calls, then we might want to inline the assembly of the function instead of calling it.

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, so I did some digging.

The codegen for setters currently depends on the fact that it has a Value* with the following properties:

1)  The Value is rooted.
2)  The Value is mutable, and can be used to store something we might need to root.

JSNative provides such a guarantee right now, as does JSStrictPropertyOp.  So ideally so would the new thing, for now....

Comment 17

[Eric Faust [:efaust]]

OK, A few things we should still change from the patches above:

1) If we want to do idempotency on getters, we should have that included in the JSJitInfo for the getter, and we can use that.
2) Codegen knows if the operation in question is infallible. We can save a branch if the JIT gets this information.
3) Looking forward to doing methods, we should move from one JSJitInfo per JSPropertySpec to one JSJitInfo per accessor.

To this end, I intend to change JSPropertySpec to bundle a JSJitInfo in a wrapper struct with the JSPropertyOp, so we should plan to reflow the JSPropertySpecs that flow into JS_DefineProperties accordingly.

Comment 18

[Eric Faust [:efaust]]

Attachment: Codegen changes

Intended to land with https://bug766448.bugzilla.mozilla.org/attachment.cgi?id=640877 which changes JSJitPropertySpec to expect a struct containing the JS{Strict,}PropertyOp and a const JSJitInfo *.

Comment 19

[Eric Faust [:efaust]]

Attachment: Extend to include specialized getter infalibility information

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 641309 [details] [diff] [review]
Extend to include specialized getter infalibility information

For isCallback(), isObject(), and "any" worth documenting that the reason they're not returning True is that they involve a JS_WrapValue.

In fact, it _might_ be worth returning a tuple from setValue() so that the comment is not needed.  Not sure whether it would make the code simpler or more complicated.

The JS_NewNumberValue case never returns a false JSBool, afaict, so could return True for infallibility.

Comment 21

[Eric Faust [:efaust]]

Attachment: Infallibility reflecting bz's comments

Comment 22

[Eric Faust [:efaust]]

Attachment: Allow callback interface for IM to query DOM inheritance information

Fixed up so it actually builds ;)

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 641349 [details] [diff] [review]
Allow callback interface for IM to query DOM inheritance information

Looks good if we can move all the APIs to jsfriendapi.h.

Comment 24

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

Comment on attachment 641349 [details] [diff] [review]
Allow callback interface for IM to query DOM inheritance information

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

::: dom/base/nsJSEnvironment.cpp
@@ +3819,5 @@
> +NS_InstanceClassHasProtoAtDepth(JSHandleObject protoObject, uint32_t protoID,
> +                                uint32_t depth)
> +{
> +  JSClass* instanceClass = static_cast<JSClass*>(
> +    js::GetReservedSlot(protoObject, DOM_PROTO_INSTANCE_CLASS_SLOT).toPrivate());

Should probably assert something about protoObject, and this function should live in BindingUtils.cpp.

@@ +3820,5 @@
> +                                uint32_t depth)
> +{
> +  JSClass* instanceClass = static_cast<JSClass*>(
> +    js::GetReservedSlot(protoObject, DOM_PROTO_INSTANCE_CLASS_SLOT).toPrivate());
> +  MOZ_ASSERT(IsDOMClass(instanceClass));

Can instanceClass be null? Worth asserting too.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Should probably assert something about protoObject

Not much we can assert, other than that it has enough reserved slots.  And I think GetReservedSlot does that already.

Comment 26

[Peter Van der Beken [:peterv]]

Comment on attachment 641307 [details] [diff] [review]
Codegen changes

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

::: dom/bindings/Codegen.py
@@ +970,3 @@
>  
>          def specData(attr):
> +            return (attr.identifier.name, flags(attr), getter(attr), setter(attr))

Drop this change if it's only changing whitespace.

@@ +3231,5 @@
> +                    "  %s,\n"
> +                    "  %s,\n"
> +                    "  %s,  /* isInfallible. False for setters. */\n"
> +                    "  false  /* isConstant. False for setters. */\n"
> +                    "};\n" % (infoName, opName, protoID, depth, failstr))

Line up the " (and make the first line be just "\n").

@@ +4052,5 @@
>              cgThings.extend([CGNativeSetter(descriptor, a) for a in
>                               descriptor.interface.members if
>                               a.isAttr() and not a.readonly])
> +            cgThings.extend([CGPropertyJITInfo(descriptor, a) for a in
> +                             descriptor.interface.members if a.isAttr()])

It might be nice to keep things together per attribute, so something like:

    for m in descriptor.interface.members:
        if m.isAttr():
            cgThings.append(CGSpecializedGetter(descriptor, m))
            cgThings.append(CGNativeGetter(descriptor, m))
            if not m.readonly:
                cgThings.append(CGSpecializedSetter(descriptor, m))
                cgThings.append(CGNativeSetter(descriptor, m))
            cgThings.append(CGPropertyJITInfo(descriptor, m))

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 641330 [details] [diff] [review]
Infallibility reflecting bz's comments

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

r=me, but please check the isCreator thing with bz.

::: dom/bindings/Codegen.py
@@ +2461,5 @@
>      elif tag in [IDLType.Tags.int64, IDLType.Tags.uint64, IDLType.Tags.float,
>                   IDLType.Tags.double]:
>          # XXXbz will cast to double do the "even significand" thing that webidl
>          # calls for for 64-bit ints?  Do we care?
> +        return (wrapAndSetPtr("JS_NewNumberValue(cx, double(%s), ${jsvalPtr})" % result), True)

Hmm, we don't verify anywhere that JS_NewNumberValue actually always returns true. It doesn't look easy to add because of the wrapAndSetPtr.

@@ +2511,5 @@
> +    CURRENT ASSUMPTIONS:
> +        We assume that successCode for wrapping up return values cannot contain
> +        failure conditions.
> +    """
> +    return getWrapTemplateForType(type, descriptorProvider, 'result', None, False)[1]

It seems to me that passing False for isCreator will break attributes marked as a Creator, I think this should pass True as the last argument

@@ +3256,5 @@
>          getterinfo = ("%s_getterinfo" % self.attr.identifier.name)
>          getter = ("(JSJitPropertyOp)specialized_get_%s" %
>                    self.attr.identifier.name)
> +        getterinfal = "infallible" in self.descriptor.getExtendedAttributes(self.attr, getter=True)
> +        getterinfal = getterinfal and infallibleForType(self.attr.type, self.descriptor)

or pass |self.attr.getExtendedAttribute("Creator") is not None| through to infallibleForType and getWrapTemplateForType here.

Comment 28

[Boris Zbarsky [:bzbarsky]]

JS_NewNumberValue is just sadmaking, and I hope that bug 752223 will fix things so that we don't have to guess whether it's fallible.  ;)

For creator stuff, I think we should just add a helper method called isCreator() that does the extended attribute thing and then pass in the right isCreator value.  Peter is right that just passing false will make codegen fail if there's ever an attribute whose getter is in fact a creator for good reasons (unlikely as that is to happen).

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 641349 [details] [diff] [review]
Allow callback interface for IM to query DOM inheritance information

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

NS_InstanceClassHasProtoAtDepth needs a comment explaining what its return values mean (ie what does it mean for this function to return true).

::: dom/base/nsJSEnvironment.cpp
@@ +3819,5 @@
> +NS_InstanceClassHasProtoAtDepth(JSHandleObject protoObject, uint32_t protoID,
> +                                uint32_t depth)
> +{
> +  JSClass* instanceClass = static_cast<JSClass*>(
> +    js::GetReservedSlot(protoObject, DOM_PROTO_INSTANCE_CLASS_SLOT).toPrivate());

Move this to BindingUtils.cpp, it's only valid for the new DOM bindings.

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 641349 [details] [diff] [review]
Allow callback interface for IM to query DOM inheritance information

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

Actually, could you explain why we go through the additional step of storing the JSClass of (one of the) concrete implementations in a slot on the interface prototype object? It seems like the one caller of this in your patch in bug 747288 has an actual object and gets the proto just to call this callback, why can't we just get the JSClass off of the actual object? Also, how does that interact with random non-DOM objects with a DOM interface prototype object as their proto?

Comment 31

[Peter Van der Beken [:peterv]]

Comment on attachment 641349 [details] [diff] [review]
Allow callback interface for IM to query DOM inheritance information

Unsetting r+, see questions in comment 30.

Comment 32

[Boris Zbarsky [:bzbarsky]]

The reason we do that is that a type in TI implies a particular proto but not yet a particular JSClass, in general.  So what TI has to work with is the proto.

Now in our particular case, I believe the immediate proto of a DOM object does in fact imply the JSClass of the DOM object, right?  Which is what that part is doing.... (Note that I wrote most of that code, which is why you're stuck reviewing it, just in case I was wrong about parts of the above!)

Comment 33

[Eric Faust [:efaust]]

(In reply to Peter Van der Beken [:peterv] from comment #30)
> Comment on attachment 641349 [details] [diff] [review]
> Allow callback interface for IM to query DOM inheritance information
> 
> Review of attachment 641349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, could you explain why we go through the additional step of storing
> the JSClass of (one of the) concrete implementations in a slot on the
> interface prototype object? It seems like the one caller of this in your
> patch in bug 747288 has an actual object and gets the proto just to call
> this callback, why can't we just get the JSClass off of the actual object?

The caller doesn't actually have concrete access to the object itself, only the prototype, only probably a type object that refers to a bunch of objects with a given prototype. As Boris says, we can't (but hope to in future) get a JSClass from a TypeObject, so we need a mapping from what we can get to what we need.

> Also, how does that interact with random non-DOM objects with a DOM
> interface prototype object as their proto?

If *any* of those objects given by a specific TypeObject from TI was created with a class without JSCLASS_IS_DOMJSCLASS (which is no longer a random embedder bit, but a full JS bit) set, *all* of the objects represented by that TypeObject are considered unfit for such an optimization, and current jitcode that relies upon it is discarded. Any sets of __proto__ lead to similar invalidations. The callback is guaranteed by the JIT to only be called in situations where we know the types are safe (using the same flag scheme), and reading that slot will be valid.

Comment 34

[Boris Zbarsky [:bzbarsky]]

We should probably document that somewhere (e.g. on the jsapi for setting the callback?).

Comment 35

[Eric Faust [:efaust]]

I will add a comment to the effect that generally unsafe-looking code is guaranteed to not be called in unsafe situations before landing.

Comment 36

[Peter Van der Beken [:peterv]]

Comment on attachment 641349 [details] [diff] [review]
Allow callback interface for IM to query DOM inheritance information

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #32)
> Now in our particular case, I believe the immediate proto of a DOM object
> does in fact imply the JSClass of the DOM object, right?

Right. The thing that's unclear is that this is only called on immediate protos. It really needs more documenting.

Comment 37

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87ddeea57fbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddacf21b5431
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d9b9f1158a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9428d40e8ead

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87ddeea57fbd
https://hg.mozilla.org/mozilla-central/rev/ddacf21b5431
https://hg.mozilla.org/mozilla-central/rev/7d9b9f1158a2
https://hg.mozilla.org/mozilla-central/rev/9428d40e8ead