BaselineCompiler: Support arguments objects

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 708152 [details] [diff] [review]
Part 1: Refactor some functions to work with baseline frames

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

5 years ago
Created attachment 708211 [details] [diff] [review]
Part 2: Support arguments objects

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

5 years ago
Created attachment 708260 [details] [diff] [review]
Part 3: Mark the arguments object
Attachment #708260 - Flags: review?(kvijayan)
(Assignee)

Comment 4

5 years ago
Created attachment 708261 [details] [diff] [review]
Part 3: Mark the arguments object

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+

Updated

5 years ago
Attachment #708261 - Flags: review?(kvijayan) → review+
Attachment #708152 - Flags: review?(dvander) → review+
(Assignee)

Comment 7

5 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.