844887 - Sanity check argument types for ParallelArray functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

The self-hosted ParallelArray functions needs to do more sanity checks on their arguments.

For example, most of them take functions, but we do not check that those functions are callable, which should look something like this:

  if (!IsCallable(func))
    ThrowError(JSMSG_NOT_FUNCTION, DecompileArg(1, func));

There is one minor complication to this: we need to ensure that these checks are safe for parallel execution, so as to support nested calls to PA functions.  In the case of the check above, for example, IsCallable() is an intrinsic and thus we cannot call it in parallel.  We'd need to add an inlined version to MCallOptimize.

The good news is that adding support for these checks generally means that they will become very efficient as well (no-ops, essentially).  For example, we can replace calls to IsCallable() where TI says the argument is a function with the constant TRUE.  That would cause the if to be optimized out altogether by GVN/UCE.

For now, I have added FIXMEs into ParallelArray.js.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Patch to inline ToObject and IsCallable intrinsics

I'd like to write some tests for these, but not sure how. Ideas welcome.

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 736528 [details] [diff] [review]
Patch to inline ToObject and IsCallable intrinsics

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

::: js/src/ion/CodeGenerator.cpp
@@ +5950,5 @@
> +    Register output = ToRegister(ins->output());
> +
> +    masm.loadObjClass(object, output);
> +
> +    // An object is callable if (isFunction() || getClass()->call).

prefer "iff"

@@ +5952,5 @@
> +    masm.loadObjClass(object, output);
> +
> +    // An object is callable if (isFunction() || getClass()->call).
> +    Label notFunction;
> +    Label done;

Can just be "Label notFunction, done;"

@@ +5955,5 @@
> +    Label notFunction;
> +    Label done;
> +    masm.branchPtr(Assembler::NotEqual, output, ImmWord(&js::FunctionClass), &notFunction);
> +    masm.move32(Imm32(1), output);
> +    masm.jump(&done);

vertical whitespace after this line

Comment 3

[Shu-yu Guo [:shu]]

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

try: https://tbpl.mozilla.org/?tree=Try&rev=01457eb754d5

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Shu-yu Guo [:shu]]

Reminder to self: still need to nop IsCallable for known function-classed objects.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: followup: nop IsCallable on functions

Comment 7

[Shu-yu Guo [:shu]]

Attachment: followup: better inlining v2

Thanks for the quick review! Sorry about another iteration though; I realized there are more cases I can inline with constant true/false.

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 743299 [details] [diff] [review]
followup: better inlining v2

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

::: js/src/ion/MCallOptimize.cpp
@@ +1347,5 @@
> +        types::StackTypeSet *types = callInfo.getArg(0)->resultTypeSet();
> +        Class *clasp = types ? types->getKnownClass() : NULL;
> +        if (clasp) {
> +            knownIsCallable = true;
> +            isCallable = clasp == &FunctionClass || clasp->call;

This might be better to implement as some Class::isCallable(), since it's an open-coded version of JSObject::isCallable().

Comment 9

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19c4bcde66f1

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/19c4bcde66f1