818050 - Can we have TI trust DOM getters/methods to return a consistent value type if they claim to do so?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In practice, many DOM getters and methods always return the same value type.  Some obviously don't (return values that are "object", "any", "unsigned long", unions, etc).  But I think most actually do.  Certainly things that return strings, as long as strings are all the same type, all numeric types other than "unsigned long", booleans. 

Interface return values we might be out of luck on because they might sometimes return an object and sometimes a cross-compartment wrapper.  :(

Sequence return values (creates a new JS array and returns it) should always be returning the same type, right?

Does it make sense to have codegen flag whether it guarantees to return the same type from the getter or method and then have the jit skip the return value type guard if the type is guaranteed stable?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> all numeric types other than "unsigned long"

I lied.  float, double, and 64-bit ints go through JS_NumberValue which will sometimes return int jsvals and sometimes double ones.  I guess we can just not worry about those for the moment, but I wonder whether for float/double we should just skip the "fits in int32" step...  That part is always annoying.  :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I'm happy to write up the codegen side of this, but I'm not quite sure what the right place to patch is on the JS side.  One experiment I just tried is hacking IonBuilder::getPropTryCommonGetter to pass a null third arg to pushTypeBarrier in the case when we're doing an MGetDOMProperty.  Seems to work, based on inspection of the generated code, but seems a bit hacky.  The problem is that I don't know how to teach TI that we're planning to always call this one getter here, with a recompilation if that changes, so we really can assume things about the return type....

Thoughts?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Teach the JIT about DOM method and getter return types so that it doesn't have to type-guard when unboxing the boxed value in many cases.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 689268 [details] [diff] [review]
Teach the JIT about DOM method and getter return types so that it doesn't have to type-guard when unboxing the boxed value in many cases.

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

Nice!

::: js/src/ion/IonBuilder.cpp
@@ +3847,5 @@
>  }
>  
> +static types::StackTypeSet*
> +AdjustTypeBarrierForDOMCall(const JSJitInfo* jitinfo,
> +                            types::StackTypeSet *types,

Style nit: types::StackTypeSet *types can go on the previous line.

@@ +3852,5 @@
> +                            types::StackTypeSet *barrier)
> +{
> +    // If the return type of our DOM native is in "types" already, we
> +    // don't actually need a barrier.
> +    if (jitinfo->returnType != JSVAL_TYPE_UNKNOWN &&

Nit: I'd slightly prefer this function to be structured like:

if (type == unknown)
    return barrier;

// JSVAL_TYPE_OBJECT comment.
if (type == object)
    return barrier;

if (type != types->getKnown(..))
    return barrier;

// No need for a barrier if we're already...
return NULL;

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 689268 [details] [diff] [review]
Teach the JIT about DOM method and getter return types so that it doesn't have to type-guard when unboxing the boxed value in many cases.

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

::: js/src/ion/IonBuilder.cpp
@@ +3846,5 @@
>      return call;
>  }
>  
> +static types::StackTypeSet*
> +AdjustTypeBarrierForDOMCall(const JSJitInfo* jitinfo,

This function should be part of IonMonkey Oracle.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Nicolas, do you want me to basically just move this exact code there and expose it as a public API?  Or do something more clever?

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Nicolas, do you want me to basically just move this exact code there and
> expose it as a public API?  Or do something more clever?

This function deals with Type info, and everything which deal with Type information is supposed to be moved to the TypeOracle which is in the Ion directory, which is not a public API.  This is where we should attempt to abstract over all sources of information customizing the MIR.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I meant a "public" API on TypeOracle, not a jsapi.h kind API.  And I meant that as opposed to having the Oracle somehow magically produce the right typesets here to start with.  So the structure of the patch would be the same as now, but instead of a static method there would be a TypeOracle function.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 689268 [details] [diff] [review]
Teach the JIT about DOM method and getter return types so that it doesn't have to type-guard when unboxing the boxed value in many cases.

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

::: dom/bindings/Codegen.py
@@ +4426,5 @@
>                  setterinfo = ("%s_setterinfo" % self.member.identifier.name)
>                  setter = ("(JSJitPropertyOp)set_%s" % self.member.identifier.name)
>                  # Setters are always fallible, since they have to do a typed unwrap.
> +                result += self.defineJitInfo(setterinfo, setter, False, False,
> +                                             [self.member.type])

Wouldn't it make more sense to have this be "JSVAL_TYPE_UNDEFINED"? You could pass in [None] and make getJSReturnTypeTag return "JSVAL_TYPE_UNDEFINED" if t is None.

@@ +4516,5 @@
> +            # First element of the list; just return its type
> +            return type
> +
> +        if existingType == "JSVAL_TYPE_UNKNOWN":
> +            return existingType

I don't think you actually need this test.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Wouldn't it make more sense to have this be "JSVAL_TYPE_UNDEFINED"? 

Yes.  Will do what you suggest with that.  Not that it matters much, since the JIT ignores this field for setters anyway.

> I don't think you actually need this test.

The idea is that once I've landed on "unknown" I need to stay there.  Or is the point that if I skip this test I'll just fall into the "different types" case?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> You could pass in [None]

I'll actually pass in [BuiltinTypes[IDLBuiltinType.Types.void]].

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/150d2a82c060

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/150d2a82c060