BaselineCompiler: Support arguments objects

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

6 years ago
No description provided.
Assignee

Comment 1

6 years ago
Patch for m-c to use AbstractFramePtr instead of StackFrame * in some arguments-related functions so that it works with baseline frames.
Attachment #708152 - Flags: review?(dvander)
Assignee

Comment 2

6 years ago
Applies on top of the "eval" patches and part 1.

The engine tries to avoid creating an arguments object if all accesses are of the form: (1) arguments.length, (2) arguments[i] or (3) f.apply(x, arguments). In that case, JSOP_ARGUMENTS pushes a MagicValue(JS_OPTIMIZED_ARGUMENTS). Else, it must push a newly created arguments object. (Note that JSOP_ARGUMENTS is the first op in the script: the result is stored in a local slot, so every script has at most one JSOP_ARGUMENTS op.)

In some cases (calling a non-default apply function, out-of-bounds access etc), we have to deoptimize though, and JSScript::argumentsOptimizationFailed creates an arguments object for all frames on the stack for that script, invalidates JIT code and sets script->needsArgsObj_ to true.

For the baseline compiler we don't want to invalidate like that though, so we can't rely on script->needsArgsObj() during compilation. So instead we set a flag on BaselineScript that's checked by JSOP_ARGUMENTS, and when it's set we create an arguments object.
Attachment #708211 - Flags: review?(kvijayan)
Assignee

Comment 3

6 years ago
Attachment #708260 - Flags: review?(kvijayan)
Assignee

Comment 4

6 years ago
Forgot to qref.
Attachment #708260 - Attachment is obsolete: true
Attachment #708260 - Flags: review?(kvijayan)
Attachment #708261 - Flags: review?(kvijayan)
Comment on attachment 708211 [details] [diff] [review]
Part 2: Support arguments objects

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

::: js/src/ion/BaselineIC.cpp
@@ +1644,5 @@
> +        if (!GetElemOptimizedArguments(cx, frame, &lhsCopy, rhs, res, &isOptimizedArgs))
> +            return false;
> +    }
> +
> +    if (!isOptimizedArgs && !GetElementMonitored(cx, &lhsCopy, rhs, res))

From what I can tell, GetElemOptimizedArguments doesn't monitor its result.  If GetElemOptimizedArguments succeeds here, and sets isOptimizedArgs to true, then we won't monitor the result here.

@@ +2114,5 @@
> +        BaselineFrame *frame = GetTopBaselineFrame(cx);
> +        if (IsOptimizedArguments(frame, val.address())) {
> +            // TODO: attach optimized stub.
> +            res.setInt32(frame->numActualArgs());
> +            return true;

Type monitoring should be done here.  This may be the first time an Int32 result is seen at this site.

::: js/src/jit-test/tests/arguments/access-formals.js
@@ +20,5 @@
> +}
> +
> +var o1 = {apply: g1};
> +assertEq(f1(3, 5, o1), 3630);
> +assertEq(f1(3, 5, o1), 3630);

Why are the assertEqs repeated twice?

@@ +40,5 @@
> +}
> +
> +var o2 = {apply: g2};
> +assertEq(f2(3, 5, o2), 400);
> +assertEq(f2(3, 5, o2), 400);

Repeated assertEqs.

::: js/src/vm/Stack.cpp
@@ +2039,5 @@
>        case SCRIPTED:
>          return interpFrame()->hasArgsObj();
> +      case ION:
> +#ifdef JS_ION
> +        JS_ASSERT(data_.ionFrames_.isBaselineJS());

Is it OK to unconditionally assert here?  Is there no potential for someone to do a hasArgsObj() query on a non-baseline ion frame?
Attachment #708211 - Flags: review?(kvijayan) → review+
Attachment #708261 - Flags: review?(kvijayan) → review+
Attachment #708152 - Flags: review?(dvander) → review+
Assignee

Comment 7

6 years ago
Part 2 and 3:

https://hg.mozilla.org/projects/ionmonkey/rev/9422c3552156
https://hg.mozilla.org/projects/ionmonkey/rev/b57fb74ba001

(In reply to Kannan Vijayan [:djvj] from comment #5)
> From what I can tell, GetElemOptimizedArguments doesn't monitor its result. 
> If GetElemOptimizedArguments succeeds here, and sets isOptimizedArgs to
> true, then we won't monitor the result here.

Thanks! Fixed.

> ::: js/src/jit-test/tests/arguments/access-formals.js
> @@ +20,5 @@
> > +}
> > +
> > +var o1 = {apply: g1};
> > +assertEq(f1(3, 5, o1), 3630);
> > +assertEq(f1(3, 5, o1), 3630);
> 
> Why are the assertEqs repeated twice?

JSOP_ARGUMENTS is always the first op in the script. So the first time we call f1, it assumes we can optimize the arguments object. Then the optimization fails, and the second time we call the function, JSOP_ARGUMENTS should create an arguments object.

> Is it OK to unconditionally assert here?  Is there no potential for someone
> to do a hasArgsObj() query on a non-baseline ion frame?

As far as I know we don't call it anywhere for (optimized) Ion frames, before this change we would hit a JS_NOT_REACHED in this case.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/7ee87ad80dd1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.