976504 - Inline IsOpaqueTypedObject() and IsTransparentTypedObject()

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Niko Matsakis [:nmatsakis]]

These intrinsics are simple comparisons against the class of an object that are sometimes used in the inner loops of things in self-hosted code.

Comment 1

[Lars T Hansen]

Attachment: typedObjectPrimitives.patch

The optimizer specializes a type test to true, false, or to a run-time test for the polymorphic case.  To support the latter case there is a new MIR op, MIsClass, which follows MHaveSameClass and MGuardClass in its implementation.  The test cases can be used to verify the operation by means of iongraph, see comments in tests.

Main concerns: whether I need to be creating MIsClass to begin with, and whether abandoning JSThreadSafeWrapper for the inlined functions is correct and/or necessary.

(I also see there is a little bit of cleanup to do in this patch once it meets approval.  Asking for feedback only at this time.)

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8381290 [details] [diff] [review]
typedObjectPrimitives.patch

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

Nice work! Just FYI, bug 932714 has a patch that adds MHasClass (I'd slightly prefer that name), so this seems like a useful MIR instruction to have. That patch has bitrotted by now probably, so I'll just rebase it on top of this bug :)

Niko is more familiar with JSNativeThreadSafeWrapper I think so I'll leave that part to him.

::: js/src/jit/MCallOptimize.cpp
@@ +1447,5 @@
>      return InliningStatus_Inlined;
>  }
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineTransparentOrOpaqueTypedObject(CallInfo &callInfo, bool transparent)

It'd be nice to pass a "const Class *" to this function and rename it to inlineHasClass or something.

Comment 3

[Lars T Hansen]

(In reply to Jan de Mooij [:jandem] from comment #2)

> Just FYI, bug 932714 has a patch that adds MHasClass (I'd slightly prefer that name), 
> so this seems like a useful MIR instruction to have.

Agree.  I will rename.

> That patch has bitrotted by now probably, so I'll just rebase it on top of this bug :)

Works for me :)

> It'd be nice to pass a "const Class *" to [the inliner function] and rename it to
> inlineHasClass or something.

Agree.  It even simplifies the code.

Comment 4

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8381290 [details] [diff] [review]
typedObjectPrimitives.patch

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

You only asked for feedback, but I'm setting the r+ bit because this looks good to land, presuming that you:

1. Fix the nits.
2. Rename MIsClass to MHasClass as jandem suggested.
3. Change the function inlineTransparentOrOpaqueTypedObject() to take a `const js::Class*` pointer rather than boolean (and probably change name to something like "inlineClassTestFunction").

::: js/src/jit/MCallOptimize.cpp
@@ +155,5 @@
>      if (native == intrinsic_ToObject)
>          return inlineToObject(callInfo);
>  
> +    // TypedObject intrinsics.
> +    if (native == intrinsic_ObjectIsTransparentTypedObject || 

Nit: trailing whitespace

@@ +157,5 @@
>  
> +    // TypedObject intrinsics.
> +    if (native == intrinsic_ObjectIsTransparentTypedObject || 
> +        native == intrinsic_ObjectIsOpaqueTypedObject)
> +        return inlineTransparentOrOpaqueTypedObject(callInfo, 

Nit: trailing whitespace, and braces required if `if` condition spans multiple lines

@@ +158,5 @@
> +    // TypedObject intrinsics.
> +    if (native == intrinsic_ObjectIsTransparentTypedObject || 
> +        native == intrinsic_ObjectIsOpaqueTypedObject)
> +        return inlineTransparentOrOpaqueTypedObject(callInfo, 
> +                                                    native == intrinsic_ObjectIsTransparentTypedObject);

Nit: over 100 chars?

Given the previous 2 nits, I personally would be inclined to break this one `if` into two, one calling `inlineTransparentOrOpaqueTypedObject()` with (the constant) true and one with false -- or, better yet, what jandem said :)

@@ +1466,5 @@
> +
> +    types::TemporaryTypeSet *types = callInfo.getArg(0)->resultTypeSet();
> +    if (const Class *knownClass = types->getKnownClass()) {
> +        bool result;
> +        if (knownClass == &TransparentTypedObject::class_)

I think jandem's suggestion would make this much clearer. These kinds of constructions kind of bend my mind (i.e., "if cond { result = some_bool } else { result = !some_bool }")

@@ +1470,5 @@
> +        if (knownClass == &TransparentTypedObject::class_)
> +            result = transparent;
> +        else if (knownClass == &OpaqueTypedObject::class_)
> +            result = !transparent;
> +        else 

Nit: trailing whitespace

Hint: configure emacs to show trailing whitespace for you.

@@ +1481,5 @@
> +
> +    // Run-time decision.
> +
> +    MIsClass *isClass = MIsClass::New(alloc(), 
> +                                      callInfo.getArg(0), 

Nit: trailing whitespace here and on the line before.

::: js/src/jit/MIR.h
@@ +9364,5 @@
> +    const Class *class_;
> +
> +    MIsClass(MDefinition *object, const Class *clasp)
> +      : MUnaryInstruction(object)
> +      , class_(clasp)

Nit: we don't typically follow this "leading comma" style, though I personally kind of dig it.

@@ +9366,5 @@
> +    MIsClass(MDefinition *object, const Class *clasp)
> +      : MUnaryInstruction(object)
> +      , class_(clasp)
> +    {
> +        setResultType(MIRType_Boolean);

Add assertion that `object` is an object

::: js/src/vm/SelfHosting.cpp
@@ +711,5 @@
>                JSNativeThreadSafeWrapper<js::ObjectIsTypeDescr>,
>                &js::ObjectIsTypeDescrJitInfo, 5, 0),
>      JS_FNINFO("ObjectIsTransparentTypedObject",
> +              intrinsic_ObjectIsTransparentTypedObject,
> +              //JSNativeThreadSafeWrapper<js::ObjectIsTransparentTypedObject>,

Nit: remove the commented code.

@@ +718,5 @@
>                JSNativeThreadSafeWrapper<js::TypedObjectIsAttached>,
>                &js::TypedObjectIsAttachedJitInfo, 1, 0),
>      JS_FNINFO("ObjectIsOpaqueTypedObject",
> +              intrinsic_ObjectIsOpaqueTypedObject,
> +              //JSNativeThreadSafeWrapper<js::ObjectIsOpaqueTypedObject>,

Nit: remove the commented code.

Comment 5

[Lars T Hansen]

Attachment: typedObjectPrimitives.patch

Relatively minor changes that address the comments (introduces MHasClass and generalizes the code in MCallOptimizer to use that operation).  Also fixes a null-pointer bug that was cleverly camouflaged by the C++ bind-and-test syntax.  Will ask for review once I've figured out how to test it and land it...

Comment 6

[Lars T Hansen]

Attachment: moreTypedObjectPrimitives.patch

Inlining and specializing ObjectIsTypeDescr, sits on top of the previous patch.

Comment 7

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8381430 [details] [diff] [review]
typedObjectPrimitives.patch

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

::: js/src/jit-test/tests/ion/inlining/TypedObject-storage-opaque.js
@@ +9,5 @@
> + * In this case the argument type is always an opaque object, so
> + * ObjectIsOpaqueTypedObject resolves to true and
> + * ObjectIsTransparentTypedObject resolves to false.
> + *
> + * Uses the soon-to-be-obsolete TypedObject arrays.

No real need to mention this... we'll have to port the tests, of course, but the idea of typed object arrays is not obsolete, just the details of the API.

::: js/src/jit-test/tests/ion/inlining/TypedObject-storage-transparent.js
@@ +9,5 @@
> + * In this case the argument type is always a transparent object, so
> + * ObjectIsOpaqueTypedObject resolves to false and
> + * ObjectIsTransparentTypedObject resolves to true.
> + *
> + * Uses the soon-to-be-obsolete TypedObject arrays.

Same as previous test.

::: js/src/jit-test/tests/ion/inlining/TypedObject-storage-unknown.js
@@ +10,5 @@
> + * JIT, so both ObjectIsOpaqueTypedObject and
> + * ObjectIsTransparentTypedObject are resolved as uses of the
> + * "HasClass" primitive.
> + *
> + * Uses the soon-to-be-obsolete TypedObject arrays.

Same as previous test.

::: js/src/jit/MCallOptimize.cpp
@@ +1459,5 @@
> +        return InliningStatus_NotInlined;
> +
> +    types::TemporaryTypeSet *types = callInfo.getArg(0)->resultTypeSet();
> +    const Class *knownClass = types ? types->getKnownClass() : nullptr;
> +    if (knownClass)

Nit: braces on the else means braces on the if case are required

Comment 8

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8381555 [details] [diff] [review]
moreTypedObjectPrimitives.patch

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

If we're inlining this at all, might as well do the more general version.

::: js/src/jit/MCallOptimize.cpp
@@ +1492,5 @@
> +    const Class *knownClass = types ? types->getKnownClass() : nullptr;
> +    if (!knownClass)
> +        return InliningStatus_NotInlined;
> +
> +    pushConstant(BooleanValue(IsTypeDescrClass(knownClass)));

We can actually generalize this fairly easily -- that is, getKnownClass() returns NULL if there are multiple known classes that are distinct. We could rewrite this to check that all known classes are type descriptor classes. (See TemporaryTypeSet::getKnownClass())

Comment 9

[Lars T Hansen]

Attachment: typedObjectPrimitives.patch

Fixed the nits, no other changes.

Comment 10

[Lars T Hansen]

Attachment: moreTypedObjectPrimitives.patch

Generalized as suggested.  I'm not wedded to this optimization, I've tried my hand at it since it came up as a possible candidate in an earlier discussion.

Comment 11

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8382034 [details] [diff] [review]
moreTypedObjectPrimitives.patch

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

::: js/src/jsinfer.h
@@ +675,5 @@
>  
> +    /* Return false if the set is not well-defined or if it is empty.
> +     * Otherwise apply func to each class in the set and to state, and return true.
> +     */
> +    bool forEachClass(void (*func)(const Class *clasp, void * state), void *state);

Higher-order... cute, though not something I think appears elsewhere in the JS engine. I iterators are more common (though consumers of type sets tend to just inline the body of this all over the place).

If we keep a method, which is fine, I'd definitely suggest a template method rather than taking a `void*`. Down with casts! You will have to add the def'n to jsinferinlines.h, though.

It might save more code if you had the callback return a bool and changed this to |forAllClasses()| (or |trueForAllClasses()|?). In that case I think it should return true if the set is empty and false is the set of classes is unknown. I find the current return value of false somewhat surprising in any case. (Note: I think it's fine for the caller to inline a result of true if the set of classes is empty; that code could never execute without a type barrier triggering some place before.)

Comment 12

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2719a9dbf96

Try: https://tbpl.mozilla.org/?tree=Try&rev=b9a0eb8484e9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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