IonMonkey: Add support for ArgumentsObject

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: djvj, Unassigned)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Identified as significant contributor to dromaeo regressions in the Prototype framework.

After talking this over with Jan on IRC, basic game plan is to implement this in as simple a way as possible.  Currently, if |needsArgsObj| is not set on the function, Ion will compile the function, and in certain cases where |needsArgsObj| gets flipped on dynamically, Ion will dynamically invalidate the code.

The modified behaviour will still do this when |needsArgsObj| starts out false, invalidating when it becomes true.  When compiling a function with |needsArgsObj| enabled, however, Ion will do the following:

1. Add an extra implicit slot for the arguments object.
2. Modify codegen to create this object eagerly on function entry
3. Modify OSR from accepting a ScopeObject directly, into accepting a pointer to an array of size 2, containing both the ScopeObject and the Arguments object (if needed).
4. All JSOP_GETARG and JSOP_GETARG go through the the arguments object instead of directly accessing slots.

This should cover all the bases.  We can look at optimizing specific cases down the line.
(Reporter)

Comment 1

5 years ago
Also: Need to unpack arguments object into interpreter frame when bailing out.
(Reporter)

Comment 2

5 years ago
Also: For now, disable inlining of functions that |needsArgsObj|.  The argsObj construction now directly grabs the argv array from the IonJS frame, which won't be correct for inlined frames.
(Reporter)

Comment 3

5 years ago
Created attachment 736657 [details] [diff] [review]
Nearly there

This patch passes almost all jit-tests, tried with the following options:
--no-jm
--no-baseline
--no-baseline --no-jm

It fails on a single test with the options: --no-baseline --no-jm --ion-eager.
That test is ./jit-test/tests/basic/bug592927.js, which deals with accessing arguments objects via |fn.arguments|.

In this case, when |fn.arguments| is accessed for a function with 2 arguments (x and y), the object returned is not ARGS[x, y], but rather ARGS[undefined, x].

I'm pretty sure I know why that's happening.  My patch is adds an extra slot to the ion frame, right before the |this| value, for all functions which have |argumentsHasVarBinding| set on them.  This pushes all other slots up by one.

Somewhere, there is some code running that's reading slots 2 and 3 expecting to get the values of x and y, and instead it's getting values for |this| and x, instead.  I suspect this the result of some code constructing the arguments object on demand by snooping stack frames, but I can't find it at the moment.

Otherwise, the patch runs clean.  Yay?
(Reporter)

Comment 4

5 years ago
Created attachment 737079 [details] [diff] [review]
Final patch for args obj support

Add arguments object support to Ion.  See comment-to-follow for some expository notes about the patch, and some caveats.
Attachment #736657 - Attachment is obsolete: true
Attachment #737079 - Flags: review?(jdemooij)
(Reporter)

Comment 5

5 years ago
Caveats first:

This patch doesn't actually speed up dromaeo one bit.  I wrote a simple microbenchmark that exercised it and ran it on a microbenchmark using the arguments object.  It showed about a 16% speedup.  This is definitely a disappointing result on a microbench that's specifically designed to exercise this new functionality.

My suspicion is that the overhead of arguments object creation is high, and furthermore, the costs of subsequent access via arguments[i] and arguments.length (now a slowpath call), and indirection of GETARG and SETARG through arguments.. severely undermines the benefits of enabling Ion on these scripts.

However, I think this can be addressed.  However, I'll post my reasoning on the main dromaeo performance bug (bug 856625).


For this patch, I'll just summarize the main changes that needed to happen, to help with review:

1. ArgumentsObject gains a new static ::createForIon() method for construction, which operates off of an IonJSFrameLayout, and an explicitly passed scopeChain/call object.

2. Ignore the new field JSScript::hasFailedArgumentsOptimization.  It's not needed, not used, and will be stripped from my pach.

3. Arguments support is not enabled for OSR entry, so a number of "isIonCompileable" checks are parametrized now on whether the compilation is OSR or not.  Also, inlining of functions with |needsArgsObj| is not supported.

4. JSScripts with |argumentsHasVarBinding| are compiled with an extra resumePoint slot which captures the arguments object.  If the function is NOT |needsArgsObj|, this is a constant undefined.  If the function has |needsArgsObj|, then this slot is initialized in the entry.  There are a number of changes to various places that traverse slots (snapshot reading/writing, etc.) related to this additional slot.  In several places, function signatures have been changed to include an extra JSScript argument because that's the only way to decide whether to expect the argumentsObject slot or not.

5. Ion gets 3 new IR instructions: MCreateArgumentsObject,  MGetArgumentsObjectArg, and MSetArgumentsObjectArg.  MCreateArgumentsObject is used to initialize the special argsObj slot on function entry.  MGetArgumentsObjectArg and its set variant is used for JSOP_GETARG and JSOP_SETARG, but only for scripts which are |argsObjAliasesFormals| (i.e. strict scripts with |needsArgsObj| can still access arguments without going through the object).

6. Bailout code is modified to extract any arguments object on the frame and set it on the interpreter or baseline frame.
(Reporter)

Updated

5 years ago
Attachment #737079 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 6

5 years ago
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2bf540222540

The Fedora64 build failure is some unmet package dependency, probably a build machine issue.  Looks clean.
(Reporter)

Updated

5 years ago
Blocks: 861596
(Reporter)

Comment 7

5 years ago
Just ran dromaeo on OSX (64-bit build), here are post/pre results (with patch is right hand side, without is left hand side):
http://dromaeo.com/?id=193536,193538

Another nice bump.  There are modest gains in a number of places, and some significant improvements on Query(Prototype) and Query(jQuery).  Still sucking on jquery.trigger, and but inching up in Prototype.
> with patch is right hand side, without is left hand side

Other way around?
(Reporter)

Comment 9

5 years ago
Yes indeed, sorry about that.
Comment on attachment 737079 [details] [diff] [review]
Final patch for args obj support

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

Looks great, I was afraid this would be more complicated. r=me with comments below addressed and feedback+ from gkw and/or decoder.

::: js/src/ion/BaselineBailouts.cpp
@@ +1214,5 @@
>  
> +            // If the frame doesn't even have a scope chain set yet, then it's resuming
> +            // into the the prologue before the scope chain is initialized.  Any
> +            // necessary args object will also be initialized there.
> +            if (topFrame->scopeChain() && frame->script()->needsArgsObj()) {

s/topFrame/frame

::: js/src/ion/CodeGenerator.cpp
@@ +2857,5 @@
> +    Register temp = ToRegister(lir->getTemp(0));
> +    Register argsObj = ToRegister(lir->getArgsObject());
> +    ValueOperand out = ToOutValue(lir);
> +    MGetArgumentsObjectArg *mir = lir->mir();
> +    

Nit: some trailing whitespace here and in the Set* method below.

@@ +2871,5 @@
> +    Register temp = ToRegister(lir->getTemp(0));
> +    Register argsObj = ToRegister(lir->getArgsObject());
> +    ValueOperand value = ToValue(lir, LSetArgumentsObjectArg::ValueIndex);
> +    MSetArgumentsObjectArg *mir = lir->mir();
> +    

We need a pre-barrier here I think for incremental GC. See for instance visitStoreFixedSlotV.

::: js/src/ion/CompileInfo.h
@@ +13,5 @@
>  namespace js {
>  namespace ion {
>  
>  inline unsigned
> +CountArgSlots(RawScript script, JSFunction *fun)

You can get script from |fun| (makes it impossible to pass a script for another function). Also, while you are here, can you implement this using StartArgSlot?

@@ +30,5 @@
> +    return slots;
> +}
> +
> +inline unsigned
> +StartArgSlot(RawScript script, JSFunction *fun)

Same here.

@@ +174,5 @@
>          return firstStackSlot() + i;
>      }
>  
> +    uint32_t startArgSlot() const {
> +        return StartArgSlot(script(), fun());

I think this is like firstArgSlot(), but does not include |this|? To avoid confusion, maybe remove firstArgSlot and use thisSlot instead?

::: js/src/ion/Ion.cpp
@@ +1322,5 @@
>          return AbortReason_Alloc;
>  
> +    if (info->needsArgsObj()) {
> +        IonSpew(IonSpew_Scripts, "Script %s:%d needs arguments object!",
> +                script->filename(), script->lineno);

Nit: not sure if this needs its own message in the "script" channel. Maybe add this to the "Analyzing script %s:%d" line in IonBuilder.

::: js/src/ion/IonBuilder.cpp
@@ +686,5 @@
> +bool
> +IonBuilder::initArgumentsObject()
> +{
> +    IonSpew(IonSpew_Scripts, "%s:%d - Emitting code to initialize arguments object! block=%p",
> +                              script()->filename(), script()->lineno, current);

Nit: IonSpew_MIR

@@ +5118,5 @@
>      // Update slotTypes for slots that may have a different type at this join point.
>      if (!oracle->getOsrTypes(loopEntry, slotTypes))
>          return NULL;
>  
> +    // Skip 0 - no type checks on callee slot.

Nit: s/callee/scopechain?

::: js/src/ion/Lowering.cpp
@@ +333,5 @@
> +    LSetArgumentsObjectArg *lir = new LSetArgumentsObjectArg(argsObj, temp());
> +    if (!useBox(lir, LSetArgumentsObjectArg::ValueIndex, ins->getValue()))
> +        return false;
> +
> +    return add(lir, ins) && assignSafepoint(lir, ins);

You don't need the safepoint.

::: js/src/ion/MIR.h
@@ +2060,5 @@
> +
> +    // Creation of arguments object is not idempontent.
> +    AliasSet getAliasSet() const {
> +        return AliasSet::None();
> +    }

I'd just remove this method and use the default, effectful getAliasSet. It's one of the first instructions so it's unlikely to affect GVN/LICM much.

@@ +2113,5 @@
> +    MSetArgumentsObjectArg(MDefinition *argsObj, size_t argno, MDefinition *value)
> +      : MBinaryInstruction(argsObj, value),
> +        argno_(argno)
> +    {
> +        setResultType(MIRType_Value);

You can remove this line to use the default MIRType_None.

::: js/src/ion/ParallelArrayAnalysis.cpp
@@ +131,5 @@
>      UNSAFE_OP(CreateThis)
>      UNSAFE_OP(CreateThisWithTemplate)
>      UNSAFE_OP(CreateThisWithProto)
> +    UNSAFE_OP(CreateArgumentsObject)
> +    SAFE_OP(GetArgumentsObjectArg)

ArgumentsLength and GetArgument use UNSAFE_OP, so we probably want that here too.

::: js/src/jsscript.h
@@ +523,5 @@
>      /* See comments below. */
>      bool            argsHasVarBinding_:1;
>      bool            needsArgsAnalysis_:1;
>      bool            needsArgsObj_:1;
> +    bool            hasFailedArgsOptimization_:1;       

This is not used anywhere, can we remove it?

@@ +569,5 @@
>       */
>      bool analyzedArgsUsage() const { return !needsArgsAnalysis_; }
>      bool needsArgsObj() const { JS_ASSERT(analyzedArgsUsage()); return needsArgsObj_; }
>      void setNeedsArgsObj(bool needsArgsObj);
> +    bool hasFailedArgsOptimization();

Ditto.
Attachment #737079 - Flags: review?(jdemooij) → review+
Comment on attachment 737079 [details] [diff] [review]
Final patch for args obj support

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

Set as r=- to avoid confusion as they are multiple reviewers.

Do not add an extra slots as the "arguments" variable is already contained inside the snapshot.  Doing this modification will remove most of the code which update the index of the snapshots.

In addition to your patch, make sure the compilation is not disabled when compiling with --ion-eager, such as most of the test cases which are running the same function twice can be exercise on this patch.

::: js/src/ion/Bailouts.cpp
@@ +103,5 @@
> +
> +        // If the script is |argumentsHasVarBinding|, then next slot will before the
> +        // arguments object.
> +        if (script()->argumentsHasVarBinding())
> +            iter.skip();

As metioned on IRC, the "arguments" has a var binding, which implies that there is already a slot reserved for the argument object.  I don't see any need for adding an extra slot to the snapshot.

::: js/src/ion/Ion.cpp
@@ +1457,4 @@
>  {
> +    if (osr && script->needsArgsObj()) {
> +        // OSR-ing into functions with arguments objects is not supported.
> +        IonSpew(IonSpew_Abort, "OSR script has argsobj");

Can you detail what are the limitations?  Is that we would need to recover arguments from the argument object instead of recovering from the stack?

::: js/src/ion/IonBuilder.cpp
@@ +970,5 @@
> +        } else {
> +            // TODO: if hasArguments() is true, and the script has a JSOP_SETARG, then
> +            // convert all arg accesses to go through the arguments object.
> +            if (info().hasArguments())
> +                return abort("NYI: arguments & setarg.");

Doing an abort here would prevent any future compilation, I will recommend to replace this abort by something which return an AbortReason which does not forbid future recompilation.  I'll recommend you to do that as part of this patch such as you can benefit from --ion-eager in the test suite.
Attachment #737079 - Flags: review?(nicolas.b.pierron) → review-
(Reporter)

Comment 12

5 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> In addition to your patch, make sure the compilation is not disabled when
> compiling with --ion-eager, such as most of the test cases which are running
> the same function twice can be exercise on this patch.

I've been using --ion-eager to find bugs, so it's definitely enabled with --ion-eager.  I'll fix the abort issue, though and see if it raises any more bugs.

> As metioned on IRC, the "arguments" has a var binding, which implies that
> there is already a slot reserved for the argument object.  I don't see any
> need for adding an extra slot to the snapshot.

Good point, will do this.

> Can you detail what are the limitations?  Is that we would need to recover
> arguments from the argument object instead of recovering from the stack?

If there's already an arguments object generated by the interpreter, it'd need to be passed into the baseline entry jitcode.  I'd rather not mess with that code because the utility of OSR-ing into loops contained inside args-heavy functions is not that great.
(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> As metioned on IRC, the "arguments" has a var binding, which implies that
> there is already a slot reserved for the argument object.  I don't see any
> need for adding an extra slot to the snapshot.

I think it's necessary to handle cases like this:

function f(x) {
    arguments[0] = 3;
    var arguments = null;
    print(arguments); // null
    print(x); // read from arguments object, 3
}
f(10)

We could disallow assignments to the arguments-local to handle this case, but it's also possible for the |arguments| binding to be stored in the call object, so I didn't think it's unreasonable to have an extra slot. We don't allow eval's from Ion containing the string "arguments" though, so it may work to abort when somebody touches the arguments binding...
(Reporter)

Comment 14

5 years ago
Ah, thanks for bringing that up, Jan, forgot about it.  I had actually started on ripping out the extra slot before reading your comment.

Nicholas: Given that the additional slot is not an issue, can I get an R+ on the patch?  I'd like to push it soon.
Comment on attachment 737079 [details] [diff] [review]
Final patch for args obj support

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

I am surprised, I don't see any case to handle MagicValue(JS_FORWARD_TO_CALL_OBJECT) in the generated code.  I would have expected such thing near the manipulation of the formals with arguments vector.

Q: Can you add a test case which capture a formal argument and manipulate the argument vector?

::: js/src/ion/Bailouts.cpp
@@ +102,5 @@
>          flags_ &= ~StackFrame::HAS_SCOPECHAIN;
> +
> +        // If the script is |argumentsHasVarBinding|, then next slot will before the
> +        // arguments object.
> +        if (script()->argumentsHasVarBinding())

nit: reformulate your comment:

   If the is has an argument binding, then skip the snapshot slots reserved to hold its value.

@@ +117,5 @@
> +        }
> +
> +        // The second slot will be an arguments object if the script needs one.
> +        if (script()->argumentsHasVarBinding()) {
> +            v = iter.read();

nit: Use a new variable named "Value argsObj" to clarify.  Rename "v" to "scopeChain" in the code above.

@@ +121,5 @@
> +            v = iter.read();
> +            JS_ASSERT(v.isObject() || v.isUndefined());
> +            if (v.isObject()) {
> +                argsObj_ = &v.toObject().asArguments();
> +                flags_ |= StackFrame::HAS_ARGS_OBJ;

nit: This sounds like initArgsObjsUnchecked.

::: js/src/ion/BaselineBailouts.cpp
@@ +510,5 @@
>      }
>  
>      // Initialize BaselineFrame::scopeChain
>      JSObject *scopeChain = NULL;
> +    ArgumentsObject *argsObj = NULL;

nit: knowing the previous comment …

@@ +521,5 @@
>          IonSpew(IonSpew_BaselineBailouts, "      Bailout_ArgumentCheck! (no valid scopeChain)");
>          iter.skip();
> +
> +        // Scripts with |argumentsHasVarBinding| have an extra slot.
> +        if (script->argumentsHasVarBinding()) {

would it be possible to extract this to a if statement dedicated to it?

@@ +558,5 @@
> +
> +        // If script maybe has an arguments object, the second slot will hold it.
> +        if (script->argumentsHasVarBinding()) {
> +            v = iter.read();
> +            JS_ASSERT(v.isObject() || v.isUndefined());

In addition, this code looks similar to what has been done in StackFrame::initFromBailout, would it be possible to refactor between the 2?

::: js/src/ion/CodeGenerator.cpp
@@ -1809,5 @@
>      CompileInfo &info = gen->info();
>  
>      // Indexes need to be shifted by one, to skip the scope chain slot.
>      JS_ASSERT(info.scopeChainSlot() == 0);
> -    static const uint32_t START_SLOT = 1;

nit: I feel that the assertion above is no longer justified here as the constant has been moved into startArgSlot.  You should probably move the assertion there in addition to the comment.

@@ +1820,5 @@
>              continue;
>  
>          // Use ReturnReg as a scratch register here, since not all platforms
>          // have an actual ScratchReg.
> +        int32_t offset = ArgToStackOffset((i - info.startArgSlot()) * sizeof(Value));

nit:  This is an operation, which is hard to follow:
  - i                        => Slot inside the resume point.
  - & - info.startArgSlot()  => Index of the argument
  - & * sizeof(Value)        => Relative displacement of the argument in a vector.
  - ArgToStackOffset(&)      => Relative displacement of the argument in the stack.

I think it would be good to explicit these operations as they are not common.  At least adding the above description my clarify what is happening.

@@ +2847,5 @@
> +    masm.addPtr(Imm32(frameSize()), temp);
> +
> +    pushArg(ToRegister(callObj));
> +    pushArg(temp);
> +    return callVM(NewIonArgumentsObjectInfo, lir);

nit: This function is copying the arguments from their location on the stack to the argument object.  Can you "weakly" assert that this is among the first operations to be executed, such as:

  JS_ASSERT(lir->mir()->block()->id() == 1);

To prevent warn any clever hacker that we can create an argument object from any location.

@@ +2860,5 @@
> +    MGetArgumentsObjectArg *mir = lir->mir();
> +    
> +    masm.loadPrivate(Address(argsObj, ArgumentsObject::getDataSlotOffset()), temp);
> +    Address argAddr(temp, ArgumentsData::offsetOfArgs() + lir->mir()->argno() * sizeof(Value));
> +    masm.loadValue(argAddr, out);

Can you generate some code to assert that the value that we load does not load an aliased argument? (aka. MagicValue(JS_FORWARD_TO_CALL_OBJECT))

and add a test case for that:

function f(x) {
  var args = arguments;
  arguments[0] = 0;
  return {
    f: function () { return x; },
    g: function () { return args[0]; }
  };
}

// Check that aliased arguments are correctly set to the callObject
for (var i = 0; i < 2000; i++)
  assertEq(f(1).f(), 0);

// Check that aliased arguments are correctly read from the callObject
for (var i = 0; i < 2000; i++)
  assertEq(f(1).g(), 0);

@@ +2874,5 @@
> +    MSetArgumentsObjectArg *mir = lir->mir();
> +    
> +    masm.loadPrivate(Address(argsObj, ArgumentsObject::getDataSlotOffset()), temp);
> +    Address argAddr(temp, ArgumentsData::offsetOfArgs() + lir->mir()->argno() * sizeof(Value));
> +    masm.storeValue(value, argAddr);

Same here, make sure that this is not the FORWARD magic value.

::: js/src/ion/CompileInfo.h
@@ +18,3 @@
>  {
> +    /* First slot is for scope chain. */
> +    unsigned slots = 1;

nit: As suggested before, assert that the scopeChain has the slot 0.

@@ +23,5 @@
> +    if (script->argumentsHasVarBinding())
> +        slots++;
> +
> +    /* If function, add arguments and this. */
> +    if (fun)

nit: and replace the previous updates of slots by:
  unsigned slots = StartArgSlots(script, fun);

@@ +56,5 @@
>      {
>          JS_ASSERT_IF(osrPc, JSOp(*osrPc) == JSOP_LOOPENTRY);
> +        nimplicit_ = 1                                      /* scope chain */
> +                   + (fun ? 1 : 0)                          /* this */
> +                   + (hasArguments() ? 1 : 0);              /* argument obj */

nit: isn't that the same as StartArgSlot, except that this is counted in CountArgSlot instead of StartArgSlot?  Can we factor these?

::: js/src/ion/Ion.cpp
@@ +1544,5 @@
>          IonSpew(IonSpew_Abort, "debugging");
>          return Method_CantCompile;
>      }
>  
> +    if (!CheckScript(script, !!osrPc)) {

nit: what about bool(osrPc) instead of !!osrPc ?  Not a big concern.

::: js/src/ion/IonAnalysis.cpp
@@ +202,1 @@
>          return true;

Good catch.

::: js/src/ion/IonBuilder.cpp
@@ +389,5 @@
>              ins->setResumePoint(current->entryResumePoint());
>      }
>  
> +    // lazyArguments should never be accessed in |argsObjAliasesFormals| scripts.
> +    if (!info().argsObjAliasesFormals() && info().hasArguments()) {

nit: check hasArguments() first, before checking if they are aliasing formals.

@@ +497,5 @@
>      JS_ASSERT(!script()->analysis()->usesScopeChain());
>      MInstruction *scope = MConstant::New(UndefinedValue());
>      current->add(scope);
>      current->initSlot(info().scopeChainSlot(), scope);
> +    JS_ASSERT(!info().needsArgsObj());

^

@@ +509,5 @@
>      IonSpew(IonSpew_Inlining, "Initializing %u arg slots", info().nargs());
>  
> +    // NB: Ion does not inline functions which |needsArgsObj|.  So using argSlot()
> +    // instead of argSlotUnchecked() below is OK
> +    JS_ASSERT(!info().needsArgsObj());

I understand that we want to be sure, but doing it twice does not bring more guarantee here ;)  (look 12 lines above)

@@ +539,5 @@
>              (void *) current->entryResumePoint(), current->entryResumePoint()->numOperands());
>  
> +    // +2 for the scope chain and |this|, maybe another +1 for arguments object slot.
> +    JS_ASSERT(current->entryResumePoint()->numOperands() ==
> +              info().nargs() + info().nlocals() + 2 + (info().hasArguments() ? 1 : 0));

nit: add  info.totalSlots() to replace this.

@@ +5073,5 @@
>          osrBlock->initSlot(slot, osrv);
>      }
>  
>      // Initialize stack.
> +    uint32_t numSlots = preheader->stackDepth() - info().endArgSlot() - info().nlocals();

nit: == preheader->stackDepth() - info().firstStackSlot();
  which can be renamed to numStackSlots.

@@ +5078,2 @@
>      for (uint32_t i = 0; i < numSlots; i++) {
>          uint32_t slot = info().stackSlot(i);

nit: and match perfectly the use case -^

@@ +5105,5 @@
>      JS_ASSERT(predecessor->stackDepth() == osrBlock->stackDepth());
>      JS_ASSERT(info().scopeChainSlot() == 0);
>      JS_ASSERT(osrBlock->scopeChain()->type() == MIRType_Object);
>  
> +    bool argsSlotAdj = info().hasArguments() ? 1 : 0;

nit: Add a comment above to mention that:

  OSR is not enabled when we have argument object, but the argument slot is still present when we optimize with lazy arguments.

Q: Wouldn't it be easier to add a fake entry in the slotTypes vector?

::: js/src/ion/IonFrameIterator-inl.h
@@ +135,5 @@
>  }
>   
>  template <AllowGC allowGC>
>  inline JSObject *
>  InlineFrameIteratorMaybeGC<allowGC>::scopeChain() const

nit: remove trailing white-space above the template line.

::: js/src/ion/Lowering.cpp
@@ +326,5 @@
> +    return defineBox(lir, ins);
> +}
> +
> +bool 
> +LIRGenerator::visitSetArgumentsObjectArg(MSetArgumentsObjectArg *ins)

nit: remove trailing white space after "bool"

::: js/src/ion/MIR.h
@@ +2049,5 @@
> +
> +  public:
> +    INSTRUCTION_HEADER(CreateArgumentsObject)
> +    static MCreateArgumentsObject *New(MDefinition *callObj)
> +    {

nit: Put the '{' on the previous line, same thing for the following New static functions.

::: js/src/ion/VMFunctions.cpp
@@ +678,5 @@
>  
> +JSObject *
> +NewIonArgumentsObject(JSContext *cx, IonJSFrameLayout *frame, HandleObject scopeChain)
> +{
> +    return ArgumentsObject::createForIon(cx, frame, scopeChain);

nit: Use directly function instead of making a trivial wrapper?  All VM functions do not have to be in VMFunctions.cpp, this is no longer the JägerMonkey era ;)

If you got a complex error message, it probably means that you have to specify that an ArgumentObject * behave like an object in VMFunctions.h

::: js/src/vm/ArgumentsObject.cpp
@@ +114,5 @@
> +            (dst++)->init(*src++);
> +
> +        /* Copy actual argument which are not contignous. */
> +        if (numFormals < numActuals) {
> +            src = frame_->argv() + 1 + numFormals; /* +1 to skip this. */

I don't understand the argument above the if.  IonMonkey arguments are contiguous on the stack, and the line above is the proof of it as it is equal to the end of the formal arguments.
(Reporter)

Comment 16

5 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #15)
> Comment on attachment 737079 [details] [diff] [review]
> Final patch for args obj support
> 
> Review of attachment 737079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am surprised, I don't see any case to handle
> MagicValue(JS_FORWARD_TO_CALL_OBJECT) in the generated code.  I would have
> expected such thing near the manipulation of the formals with arguments
> vector.

Direct handling of JSOP_GETARG will never need to deal with this, because if a particular var is forwarded to the call object, then JSOP_*ARG ops aren't emitted for it.  Instead, JSOP_*ALIASEDVAR is emitted for it.

> Q: Can you add a test case which capture a formal argument and manipulate
> the argument vector?

Good idea, will do.
(Reporter)

Comment 17

5 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #15)
> nit: This sounds like initArgsObjsUnchecked.

actually, the existing initArgsObj() on the StackFrame works, so I used that.  Baseline needs the Unchecked because it's being called on a non-stack frame that will be copied to the stack later, and the checked method peeks up the "stack" to do some asserts.

> @@ +521,5 @@
> >          IonSpew(IonSpew_BaselineBailouts, "      Bailout_ArgumentCheck! (no valid scopeChain)");
> >          iter.skip();
> > +
> > +        // Scripts with |argumentsHasVarBinding| have an extra slot.
> > +        if (script->argumentsHasVarBinding()) {
> 
> would it be possible to extract this to a if statement dedicated to it?

Either way there is going to be one conditional inside an other (the check for Bailout_ArgumentCheck).  I think this is clearer because both the scope chain logic and the argsobj logic depend on the bailout check, so it should be the outer check.

> In addition, this code looks similar to what has been done in
> StackFrame::initFromBailout, would it be possible to refactor between the 2?

It's possible, but not worthwhile.  Bailing out to interpreter is only there now because we still have to support jaeger.  Once that's gone, we will always bail out to baseline, and this bailout code can be ripped out.

> ::: js/src/ion/CompileInfo.h
> @@ +18,3 @@
> >  {
> > +    /* First slot is for scope chain. */
> > +    unsigned slots = 1;
> 
> nit: As suggested before, assert that the scopeChain has the slot 0.

We don't have access to CompileInfo here, since these static methods are used outside of Ion compile context.

> @@ +56,5 @@
> >      {
> >          JS_ASSERT_IF(osrPc, JSOp(*osrPc) == JSOP_LOOPENTRY);
> > +        nimplicit_ = 1                                      /* scope chain */
> > +                   + (fun ? 1 : 0)                          /* this */
> > +                   + (hasArguments() ? 1 : 0);              /* argument obj */
> 
> nit: isn't that the same as StartArgSlot, except that this is counted in
> CountArgSlot instead of StartArgSlot?  Can we factor these?

I factored out the 1 and the hasArguments(), and just called StartArgSlot instead.  However, factoring more than this is not that useful, and just adds tiny helper functions that are used in only one place.


> Q: Wouldn't it be easier to add a fake entry in the slotTypes vector?

The slotTypes vector is passed to |oracle->getOsrTypes()|, which fills in the types for |callee, this, args...|

Adding a fake entry in the slotTypes vector doesn't really help with that, the same adjustments would have to be done to the slotNo accesses later anyway.

> nit: Use directly function instead of making a trivial wrapper?  All VM
> functions do not have to be in VMFunctions.cpp, this is no longer the
> JägerMonkey era ;)
> 
> If you got a complex error message, it probably means that you have to
> specify that an ArgumentObject * behave like an object in VMFunctions.h

Instead of adding more incidental types to VMFunctions, I just gave the method signature as returning JSObject *, and cast the function to that signature.

> I don't understand the argument above the if.  IonMonkey arguments are
> contiguous on the stack, and the line above is the proof of it as it is
> equal to the end of the formal arguments.

Good catch.  Fixed.



All other comments addressed.  New patch going up for r? soon.
(Reporter)

Comment 18

5 years ago
Created attachment 738884 [details] [diff] [review]
Ion ArgsObj support, nbp comments addressed
Attachment #737079 - Attachment is obsolete: true
Attachment #738884 - Flags: review?(nicolas.b.pierron)
Comment on attachment 738884 [details] [diff] [review]
Ion ArgsObj support, nbp comments addressed

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

Ok, quick review, this sounds good.

::: js/src/ion/CodeGenerator.cpp
@@ +1820,5 @@
> +        // i - slot inside resume point
> +        // info.startArgSlot() - subtract offset of initial slot.
> +        // sizeof(Value) - scale by value size.
> +        // ArgToStackOffset - displacement of arg on stack.
> +        int32_t offset = ArgToStackOffset((i - info.startArgSlot()) * sizeof(Value));

nit: the comment is still not clear, I'll understand it better with:
 - aside to startArgSlot(), add "…, compute the index within the arg vector".
 - aside to sizeof(Value), add "…, compute displacement within the arg vector".

::: js/src/ion/VMFunctions.h
@@ +539,5 @@
>  bool StrictEvalPrologue(JSContext *cx, BaselineFrame *frame);
>  bool HeavyweightFunPrologue(JSContext *cx, BaselineFrame *frame);
>  
>  bool NewArgumentsObject(JSContext *cx, BaselineFrame *frame, MutableHandleValue res);
> +JSObject *NewIonArgumentsObject(JSContext *cx, IonJSFrameLayout *frame, HandleObject callObj);

nit: This function no longer exists.
Attachment #738884 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/d12788533ab7
https://hg.mozilla.org/mozilla-central/rev/f2387d9f146c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

5 years ago
Depends on: 864037
Depends on: 864002
Backed out for the various topcrash regressions.
https://hg.mozilla.org/mozilla-central/rev/1150403342b2
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: mozilla23 → ---
https://hg.mozilla.org/mozilla-central/rev/c307cb8bffec
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Duplicate of this bug: 864125
Depends on: 867439
(Reporter)

Updated

5 years ago
Depends on: 918405
You need to log in before you can comment on or make changes to this bug.