Sanity check argument types for ParallelArray functions

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nmatsakis, Assigned: shu)

Tracking

Trunk
mozilla23
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: PJS
(Reporter)

Updated

6 years ago
Blocks: 801869
Whiteboard: PJS → [PJS][js:t]
(Assignee)

Comment 1

6 years ago
Created attachment 736528 [details] [diff] [review]
Patch to inline ToObject and IsCallable intrinsics

I'd like to write some tests for these, but not sure how. Ideas welcome.
Assignee: general → shu
Attachment #736528 - Flags: review?(sstangl)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
Whiteboard: [PJS][js:t] → [PJS][js:t][leave open]
Depends on: 864637
No longer depends on: 864637
(Assignee)

Comment 5

6 years ago
Reminder to self: still need to nop IsCallable for known function-classed objects.
(Assignee)

Comment 6

6 years ago
Created attachment 743261 [details] [diff] [review]
followup: nop IsCallable on functions
Attachment #743261 - Flags: review?(sstangl)
Attachment #743261 - Flags: review?(sstangl) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 743299 [details] [diff] [review]
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.
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+
(Assignee)

Updated

6 years ago
Whiteboard: [PJS][js:t][leave open] → [PJS][js:t]
https://hg.mozilla.org/mozilla-central/rev/19c4bcde66f1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.