898746 - Fix rest arguments array type

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Rest argument types aren't tracked because it was deemed not worth it to be tracked in bug 887002.

It's still true that property types aren't worth the trouble to track for rest args, but we should do better than unknownProperties, so as to enable inlining of Array natives in Ion.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: v0

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 782138 [details] [diff] [review]
v0

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

::: js/src/ion/BaselineIC.cpp
@@ +8211,5 @@
>      unsigned numActuals = frame->numActualArgs();
>      unsigned numRest = numActuals > numFormals ? numActuals - numFormals : 0;
>      Value *rest = frame->argv() + numFormals;
>  
> +    RootedObject obj(cx, NewDenseCopiedArray(cx, numRest, rest, NULL));

FixRestArgumentTypes can't actually GC, so this root is unnecessary.

::: js/src/ion/IonBuilder.cpp
@@ +7178,5 @@
>  
> +static JSObject *
> +CreateTemplateRestArgumentsObject(JSContext *cx, unsigned length)
> +{
> +    RootedObject templateObject(cx, NewDenseUnallocatedArray(cx, 0, NULL, TenuredObject));

Rooting isn't necessary in IonBuilder.

@@ +7189,5 @@
>  IonBuilder::jsop_rest()
>  {
>      // We don't know anything about the callee.
>      if (inliningDepth_ == 0) {
> +        RootedObject templateObject(cx, CreateTemplateRestArgumentsObject(cx, 0));

ditto

@@ +7207,5 @@
>      // We know the exact number of arguments the callee pushed.
>      unsigned numActuals = inlineCallInfo_->argv().length();
>      unsigned numFormals = info().nargs() - 1;
>      unsigned numRest = numActuals > numFormals ? numActuals - numFormals : 0;
> +    RootedObject templateObject(cx, CreateTemplateRestArgumentsObject(cx, numRest));

ditto

::: js/src/ion/VMFunctions.cpp
@@ +723,5 @@
>          }
>          return arrRes;
>      }
>  
> +    Rooted<ArrayObject *> arrRes(cx, NewDenseCopiedArray(cx, length, rest, NULL));

This root isn't necessary.

::: js/src/jsinfer.cpp
@@ +3130,5 @@
>      }
>  };
>  
>  void
> +TypeCompartment::fixArrayTypeHelper(JSContext *cx, JSObject *obj, Type type)

This could use a better name.  TypeCompartment::fixArrayTypeForMonomorphicElement?

@@ +3153,5 @@
> +        /*
> +         * Root the type if it's not unknown, as the calls below it might
> +         * trigger moving GC. Initialize it with a dummy value to avoid
> +         * triggering asserts.
> +         */

This comment is incorrect, this function is called with GC suppressed (via AutoEnterAnalysis) and won't trigger either a moving or a normal GC.  The unnecessary roots below are preexisting but it would be good if you could remove them.

@@ +3174,5 @@
> +        // The key's fields may have been moved by moving GC and therefore the
> +        // AddPtr is now invalid. ArrayTypeTable's equality and hashcodes
> +        // operators use only the two fields (type and proto) directly, so we
> +        // can just conditionally update them here.
> +        if (!type.isUnknown() && (type != origType || key.proto != obj->getProto())) {

Again, old code but this comment is incorrect and the tests are unnecessary.

::: js/src/vm/Stack.cpp
@@ +188,5 @@
>      JS_ASSERT(fun()->hasRest());
>      unsigned nformal = fun()->nargs - 1, nactual = numActualArgs();
>      unsigned nrest = (nactual > nformal) ? nactual - nformal : 0;
>      Value *restvp = argv() + nformal;
> +    RootedObject obj(cx, NewDenseCopiedArray(cx, nrest, restvp, NULL));

This root isn't necessary.

Comment 3

[Shu-yu Guo [:shu]]

Ah okay, I'll remove the roots.

I'll rename fixArrayTypeHelper to setTypeToHomogenousArray.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: v1

Sorry for the review churn. The previous approach wasn't good enough -- if we iterated any rest arguments, it would cause all other rest arguments to not be able to inline Array methods in Ion due to the presence of OBJECT_FLAG_ITERATED. We still end up needing something like allocation site-keyed types, just with an unknown JSID_VOID typeset so we don't bother to track property types.

Other stuff rolled into the patch:
 - Remove JOF_TYPESET from JSOP_REST.
 - Remove the baseline JSOP_REST IC in favor of a callVM, since we never generated any optimized stubs.
 - Remove previous unnecessary roots in fixArrayType.

Comment 5

[Brian Hackett [Laid off!]]

Can you explain some more why site keyed times are necessary here?  This concern applies equally to all other arrays whose types were derived from arrayTypeTable, and rather than doing a spot fix for what is for now an obscure feature of the language we should instead use a more comprehensive fix if this is performance critical somewhere, and be able to cope with objects with OBJECT_FLAG_ITERATED when inlining those natives.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #5)
> Can you explain some more why site keyed times are necessary here?  This
> concern applies equally to all other arrays whose types were derived from
> arrayTypeTable, and rather than doing a spot fix for what is for now an
> obscure feature of the language we should instead use a more comprehensive
> fix if this is performance critical somewhere, and be able to cope with
> objects with OBJECT_FLAG_ITERATED when inlining those natives.

They're necessary here not really because of OBJECT_FLAT_ITERATED, but because we want deoptimizing things done to rest arguments to be localized to the function that the rest args belongs to.

In self-hosted code, some PJS code uses Array.pop on rest args. In the previous patch, since all rest args shared the same TypeObject, if any of them got a deoptimizing flag, like say, being iterated or having sparse indexes, now this self-hosted code will fail to compile and run in parallel for no fault of its own.

I thought arrayTypeTable were fixes for singleton arrays -- so this "action at a distance" thing causing things to not compile/inline wouldn't happen.

Comment 7

[Shu-yu Guo [:shu]]

FWIW, I think it might not be too uncommon for user scripts to operate on rest args in ways to cause them to be unoptimizable, since they're just Arrays.

Comment 8

[Brian Hackett [Laid off!]]

arrayTypeTable allows arrays with similar properties to have the same type.  It is used for arrays defined at the top level of scripts, for JSON scripts, and so forth.  Giving each of these arrays different types would be bad both for memory usage and for the quality of the type information.  A similar concern applies here: if the user page (or self hosted code) contains many scripts that use rest arguments, we will end up creating a lot of type objects whose contents are effectively identical.  This wastes memory, and if those rest arguments are manipulated via common helpers then the precision of the type information will degrade.

It really isn't great that the type information needs to be massaged in this way just to get code to compile, especially when doing so involves the above trade/offs that can hurt user scripts.  I think the fundamental problem here is with the robustness of the compiler; if the type information isn't good enough to allow Array.pop to be inlined then you can add MIR nodes with dynamic checks for presence on the iterator list (does this exist in PJS?) or for sparse indexes.

Comment 9

[Shu-yu Guo [:shu]]

Okay, that's fine. I'll use v0 and add PJS-safe versions the necessary Array methods.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80fe42f29748