Closed Bug 844887 Opened 12 years ago Closed 11 years ago

Sanity check argument types for ParallelArray functions


(Core :: JavaScript Engine, defect)

Not set





(Reporter: nmatsakis, Assigned: shu)



(Whiteboard: [PJS][js:t])


(2 files, 1 obsolete file)

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.
Whiteboard: PJS
Blocks: PJS
Whiteboard: PJS → [PJS][js:t]
I'd like to write some tests for these, but not sure how. Ideas welcome.
Assignee: general → shu
Attachment #736528 - Flags: review?(sstangl)
Blocks: 860965
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
Attachment #736528 - Flags: review?(sstangl) → review+
Whiteboard: [PJS][js:t] → [PJS][js:t][leave open]
Depends on: 864637
No longer depends on: 864637
Reminder to self: still need to nop IsCallable for known function-classed objects.
Attachment #743261 - Flags: review?(sstangl)
Attachment #743261 - Flags: review?(sstangl) → review+
Thanks for the quick review! Sorry about another iteration though; I realized there are more cases I can inline with constant true/false.
Attachment #743261 - Attachment is obsolete: true
Attachment #743299 - Flags: review?(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().
Attachment #743299 - Flags: review?(sstangl) → review+
Whiteboard: [PJS][js:t][leave open] → [PJS][js:t]
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.