Closed
Bug 836255
Opened 11 years ago
Closed 11 years ago
BaselineCompiler: Support arguments objects
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(3 files, 1 obsolete file)
7.14 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
33.50 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
873 bytes,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 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•11 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•11 years ago
|
||
Attachment #708260 -
Flags: review?(kvijayan)
Assignee | ||
Comment 4•11 years ago
|
||
Forgot to qref.
Attachment #708260 -
Attachment is obsolete: true
Attachment #708260 -
Flags: review?(kvijayan)
Attachment #708261 -
Flags: review?(kvijayan)
Comment 5•11 years ago
|
||
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•11 years ago
|
Attachment #708261 -
Flags: review?(kvijayan) → review+
Updated•11 years ago
|
Attachment #708152 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee87ad80dd1
Whiteboard: [leave open]
Assignee | ||
Comment 7•11 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]
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ee87ad80dd1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•