807853 - Add parallel compilation mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

This patch adds in the parallel compilation mode, which consists of:
- several new MIR/LIR ops;
- an analysis to check that parallel execution is safe;
- the ParallelCompilationMode outer framework that compiles the transitive closure of all functions that are being called as a unit.

There is a known bug or two in this code that haven't been fixed yet.  I will add separate bugs for those issues.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 1: Infrastructure for parallel bailouts, spew, and tracing.

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 2: Loading current slice, checking over recursion or interrupt

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 3: Write guards, unsafe set element

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 4: Parallel allocation

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 5: Specialized inlining for misc intrinsics (ThrowError, Dump)

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 6: Handle func calls in par code

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 7: Add parallel compilation mode

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 7: Permit flat string comparison in parallel mode

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: Patch 8: Add parallel compilation mode

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701077 [details] [diff] [review]
Patch 1: Infrastructure for parallel bailouts, spew, and tracing.

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

::: js/src/Makefile.in
@@ +312,4 @@
>  		ValueNumbering.cpp \
>  		RangeAnalysis.cpp \
>  		VMFunctions.cpp \
> +		ParFunctions.cpp \

nit: ParallelFunctions.cpp

::: js/src/ion/CodeGenerator.cpp
@@ +1196,4 @@
>  }
>  
>  bool
> +CodeGenerator::maybePropagateParallelBailout()

It would be a little cleaner to have something like MErrorCheck, though it would be more code. So this seems fine, I'd just change the name to "checkForParallelBailout".

::: js/src/ion/ParFunctions.h
@@ +16,5 @@
> +namespace ion {
> +
> +void ParallelAbort(JSScript *script);
> +
> +void Trace(uint32_t bblock, uint32_t lir, uint32_t execModeInt,

Since "trace" is a pretty overloaded term, could we qualify it somehow? Maybe TraceLIR?

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +514,5 @@
> +{
> +    if (!oolParallelAbort) {
> +        oolParallelAbort = new OutOfLineParallelAbort(parallelBailoutIndex++);
> +        if (!addOutOfLineCode(oolParallelAbort))
> +            return false;

Since this is a small TempObject, you can consider the allocation to be infallible.

@@ +529,5 @@
> +    return codegen->visitOutOfLineParallelAbort(this);
> +}
> +
> +bool
> +CodeGeneratorShared::maybeCallTrace(uint32_t blockIndex, LInstruction *lir,

s/maybeCallTrace/traceLIR/ ? (the "maybe" is implicit on profiling/debugging stuff.)

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +35,5 @@
>  {
>      js::Vector<OutOfLineCode *, 0, SystemAllocPolicy> outOfLineCode_;
>      OutOfLineCode *oolIns;
> +    OutOfLineParallelAbort *oolParallelAbort;
> +    uint32_t parallelBailoutIndex;

nit: Suffix with _ for these (also a comment about what BailoutIndex is would be good).

@@ +310,5 @@
> +    virtual bool visitOutOfLineParallelAbort(OutOfLineParallelAbort *ool) = 0;
> +    bool maybeCallTrace(uint32_t blockIndex, LInstruction *lir, const char *bailoutName = NULL);
> +
> +  protected:
> +    OutOfLineParallelAbort *outOfLineParallelAbort;

What's the difference between this one and the oolParallelAbort above?

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +316,5 @@
>  CodeGeneratorX86Shared::bailout(const T &binder, LSnapshot *snapshot)
>  {
> +    // There has got to be an easier way!
> +    CompileInfo &info = snapshot->mir()->block()->info();
> +    switch (info.executionMode()) {

Hah. You could store the mode in the CG I guess. I think this is okay though, I'd just get rid of the switch and have if (mode == Parallel...) {

@@ +1363,5 @@
>  
> +bool
> +CodeGeneratorX86Shared::visitOutOfLineParallelAbort(OutOfLineParallelAbort *ool)
> +{
> +    JS_ASSERT(current);

no need to assert here, since current-> will NULL crash

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701078 [details] [diff] [review]
Patch 2: Loading current slice, checking over recursion or interrupt

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

::: js/src/ion/CodeGenerator.cpp
@@ +1657,5 @@
> +// the event of a bailout, adjust the stack by that much without
> +// notifying masm.  SaveLiveRegs helps with this process by encapsulating
> +// the code to save the amount of data and to fix the stack along the
> +// bailout path.
> +class SaveLiveRegs

An easier solution to this would be to request a temporary register or an output register for the instruction. Or a fixed temporary of ReturnReg even. Then you can move the result to there and not have to worry, we'll guarantee it won't be in the live set.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701080 [details] [diff] [review]
Patch 3: Write guards, unsafe set element

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

::: js/src/ion/IonBuilder.cpp
@@ +4415,5 @@
> +    // In parallel execution, we never require write barriers.  See
> +    // forkjoin.cpp for more information.
> +    switch (info().executionMode()) {
> +      case SequentialExecution: break;
> +      case ParallelExecution: needsBarrier = false; break;

nit: newlines for each statement

::: js/src/ion/MCallOptimize.cpp
@@ +883,5 @@
> +
> +        int arrayType;
> +        if (!oracle->elementAccessIsDenseArray(obj, id) &&
> +            !oracle->elementAccessIsTypedArray(obj, id, &arrayType))
> +            return InliningStatus_NotInlined;

nit: braces here

::: js/src/ion/MIR.h
@@ +3852,4 @@
>  {
>      bool needsBarrier_;
>      MIRType elementType_;
> +    bool racy_; // if true, exempted from normal data race req. during par. exec.

It looks like this isn't used in this patch; haven't further in the queue yet so maybe it is later.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701081 [details] [diff] [review]
Patch 4: Parallel allocation

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

::: js/src/ion/CodeGenerator.cpp
@@ +2195,5 @@
> +    emitParAllocateGCThing(resultReg, parSliceReg, tempReg1, tempReg2, templateObj);
> +
> +    // TO INVESTIGATE: does ! lir->slots()->isRegister() imply that
> +    // there is no slots array at all?  And also, do we need to
> +    // store a NULL explicitly, or is this memory already zeroed?

right - and you don't need to store NULL explicitly, that's done as part of copying the templateObj, I believe.

@@ +2197,5 @@
> +    // TO INVESTIGATE: does ! lir->slots()->isRegister() imply that
> +    // there is no slots array at all?  And also, do we need to
> +    // store a NULL explicitly, or is this memory already zeroed?
> +
> +    if (lir->slots() && lir->slots()->isRegister()) {

|slots| will always be non-NULL here.

@@ +2346,5 @@
> +    // but don't bother to save the objReg.
> +    if (!saveSet.has(CallTempReg0))
> +        saveSet.add(CallTempReg0);
> +    if (!saveSet.has(CallTempReg1))
> +        saveSet.add(CallTempReg1);

You can use addUnchecked to avoid the |if|. You could add takeUnchecked too.

@@ +3503,5 @@
> +        // we make space on the stack and pass the arguments that way.
> +        // (Also, outside of the VM call mechanism, it's very hard to
> +        // pass in a Value to a C function!)
> +        masm.bind(&indexWouldExceedCapacity);
> +        //masm.breakpoint();

nit: remove breakpoint

@@ +3505,5 @@
> +        // pass in a Value to a C function!)
> +        masm.bind(&indexWouldExceedCapacity);
> +        //masm.breakpoint();
> +
> +        SaveLiveRegs regs(*this, ins);

Hrm... It's probably less ideal to have an extra temporary in this instruction, even if it was parallel-mode-only. Would it be possible to instead just restoreLive() in the abort case and then not worry about the fixStack stuff? If the only reason not to do that is some masm assert, that's fixable.

::: js/src/ion/IonMacroAssembler.cpp
@@ +345,5 @@
> +    // temporary registers.
> +    //
> +    // Subtle: I wanted to reuse `result` for one of the temporaries,
> +    // but the register allocator was assigning it to the same
> +    // register as `threadContextReg`.  Then we overwrite that

Odd - judging by the policies the LIR uses, that should never happen. Even if "threadContextReg" is dead after the instruction, there should be no overlap unless you specified use-at-start or must-reuse-input.

::: js/src/ion/LIR-Common.h
@@ +275,5 @@
> +    LIR_HEADER(ParNew);
> +
> +    LParNew(const LAllocation &parSlice,
> +            const LDefinition &temp1,
> +            const LDefinition &temp2) {

nit: brace on newline, here and for the other multi-line signatures in this file

@@ +2248,4 @@
>      }
>  };
>  
> +// TODO: this class should not require so many temporaries.

nit: remove :TODO:

::: js/src/ion/MCallOptimize.cpp
@@ +1000,5 @@
> +    // par. mode we use inlined MIR.
> +    ExecutionMode executionMode = info().executionMode();
> +    switch (executionMode) {
> +      case SequentialExecution: return inlineDenseArrayForSequentialExecution(argc);
> +      case ParallelExecution: return inlineDenseArrayForParallelExecution(argc);

nit: newline after labels

@@ +1014,5 @@
> +    return InliningStatus_NotInlined;
> +}
> +
> +IonBuilder::InliningStatus
> +IonBuilder::inlineDenseArrayForParallelExecution(uint32_t argc)

DenseArray -> NewDenseArray for these functions

::: js/src/ion/MIR.cpp
@@ +1689,5 @@
>  bool
> +MNewObject::shouldUseVM() const
> +{
> +    JSObject *o = templateObject();
> +    return o->hasSingletonType() || o->hasDynamicSlots();

nit: use templateObject_ instead of a temporary variable

@@ +1697,5 @@
> +MNewArray::shouldUseVM() const
> +{
> +    uint32_t cnt = count();
> +
> +    JS_ASSERT(cnt < JSObject::NELEMENTS_LIMIT);

nit: use count() or count_ directly instead of a temporary variable

::: js/src/ion/MIRGraph.h
@@ +607,5 @@
> +    // calling convention for parallel code, we obtain the current
> +    // slice from thread-local storage.  This helper method will
> +    // lazilly insert an MParSlice instruction in the entry block and
> +    // return the definition.
> +    MDefinition *parSlice();

Probably a better place for this is in IonBuilder, in the top-level build() function. I'd just create it up-front and let DCE remove unused ones. Then you can just cache the MDefinition in IonBuilder.

::: js/src/ion/MOpcodes.h
@@ +81,3 @@
>      _(NewStringObject)                                                      \
> +    _(ParNew)                                                               \
> +    _(ParNewDenseArray)                                                     \

At the end of this whole patch queue, it'd be nice to have all the Par opcodes together.

::: js/src/ion/ParFunctions.cpp
@@ +163,5 @@
> +                                              &args->value, 1) == JSObject::ED_OK);
> +}
> +
> +JSObject *
> +ion::ParExtendArray(ForkJoinSlice *slice, JSObject *array, uint32_t length)

Should this be a bool return?

::: js/src/vm/SelfHosting.cpp
@@ +166,4 @@
>  }
>  
>  JSBool
> +js::intrinsic_DenseArray(JSContext *cx, unsigned argc, Value *vp)

s/DenseArray/NewDenseArray/

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701082 [details] [diff] [review]
Patch 5: Specialized inlining for misc intrinsics (ThrowError, Dump)

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

::: js/src/ion/Lowering.cpp
@@ +1492,5 @@
>  
>  bool
> +LIRGenerator::visitParDump(MParDump *ins)
> +{
> +    return add(new LParDump(useFixed(ins->value(), CallTempReg0)));

You need useBoxFixed() here since the input is a Value.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701094 [details] [diff] [review]
Patch 7: Permit flat string comparison in parallel mode

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

::: js/src/ion/CodeGenerator.cpp
@@ +2817,5 @@
> +    masm.branch32(Assembler::Equal, ReturnReg, Imm32(ParCompareUnknown), bail);
> +
> +    if (op == JSOP_NE || op == JSOP_STRICTNE) {
> +        masm.xor32(Imm32(1), ReturnReg);
> +    }

nit: no braces needed

::: js/src/ion/ParFunctions.h
@@ +35,4 @@
>  
>  JSObject *ParExtendArray(ForkJoinSlice *slice, JSObject *array, uint32_t length);
>  
> +enum ParCompareResult { ParCompareNe = false, ParCompareEq = true, ParCompareUnknown = 2 };

nit: newline for each enum name

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701095 [details] [diff] [review]
Patch 8: Add parallel compilation mode

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

review of everything but ParallelArrayAnalysis.cpp

::: js/src/ion/Ion.cpp
@@ +1655,5 @@
> +    if (!builder->build())
> +        return builder->abortReason();
> +    builder->clearForBackEnd();
> +
> +    // XXX: no idea if bhackett's parallel compilation thing is safe here

nit: remove XXX:

@@ +2145,4 @@
>  }
>  
>  bool
> +ion::Invalidate(JSContext *cx, UnrootedScript script, ExecutionMode mode, bool resetUses)

General question: why doesn't invalidation just invalidate both the attached Ion scripts?

@@ +2199,5 @@
>      }
>  
> +    if (script->hasParallelIonScript()) {
> +        FinishInvalidationOf(fop, script, &script->parallelIon);
> +    }

nit: no braces needed here and above

@@ +2222,2 @@
>  
>      CancelOffThreadIonCompile(cx->compartment, script);

Is the rest of this function needed now that the other ForbidCompilation is in play?

::: js/src/ion/IonCaches.cpp
@@ +668,4 @@
>  
>      // Need to set correct framePushed on the masm so that exit frame descriptors are
>      // properly constructed.
> +    masm.setFramePushed(script->ionScript()->frameSize());

What was the reason for this change?

::: js/src/ion/MIRGraph.cpp
@@ +162,5 @@
> +MBasicBlock::NewParBailout(MIRGraph &graph, CompileInfo &info,
> +                           MBasicBlock *pred, jsbytecode *entryPc)
> +{
> +    MBasicBlock *block = MBasicBlock::New(graph, info, pred, entryPc, NORMAL);
> +    if (block) {

nit: invert to if (!block) ...

@@ +754,5 @@
> +size_t
> +MBasicBlock::getSuccessorIndex(MBasicBlock *block) const
> +{
> +    JS_ASSERT(lastIns());
> +    for (size_t i = 0; i < numSuccessors(); i++)

nit: braces on multi-line for-loop

::: js/src/ion/ParallelArrayAnalysis.cpp
@@ +1,1 @@
> +#include <stdio.h>

nit: MPL2 header above

::: js/src/ion/ParallelArrayAnalysis.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=4 sw=4 et tw=79:
> + *
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

nit: you want the new MPL2 header.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 701095 [details] [diff] [review]
Patch 8: Add parallel compilation mode

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

::: js/src/ion/ParallelArrayAnalysis.cpp
@@ +20,5 @@
> +using parallel::SpewCompile;
> +
> +typedef uint32_t typeset_t;
> +
> +static inline typeset_t typeset(MIRType type) {

nit: style for global statics is:

static inline typeset_t
Typeset(MIRType type)
{

@@ +311,5 @@
> +             script->getUseCount(), js_IonOptions.usesBeforeCompileParallel);
> +        return true;
> +    }
> +
> +    // TODO: Have worklist use an auto hash set or something.

nit: remove :TODO:

@@ +439,5 @@
> +    // refer to the MPassArg nodes generated for the call to foo().
> +    // But the call to foo() is dead and has been removed, leading to
> +    // an inconsistent IR and assertions at codegen time.
> +
> +    MConstant *udef = NULL;

Since you don't have an OSR block, I'd just create+insert this unconditionally at the top, and replace everywhere (is there any reason not to?)

@@ +494,5 @@
> +    // This block is no longer reachable.
> +    block->unmark();
> +
> +    // Determine the best PC to use for the bailouts we'll be creating.
> +    jsbytecode *pc = ins->trackedPc();

trackedPc will be going away very shortly (it should be gone already but I forgot to land the patch). Looks okay to use block->pc() instead.

@@ +557,5 @@
> +
> +bool
> +ParallelArrayVisitor::visitLambda(MLambda *ins) {
> +    if (ins->fun()->hasSingletonType() ||
> +        types::UseNewTypeForClone(ins->fun())) {

nit: move second condition up, or put { on newline

@@ +569,5 @@
> +    return true;
> +}
> +
> +bool
> +ParallelArrayVisitor::visitNewObject(MNewObject *newInstruction) {

nit: function brace on newline, here and for surrounding functions

Comment 18

[Niko Matsakis [:nmatsakis]]

Relating to the use of SaveLiveRegs as well as this comment:

```
::: js/src/ion/ParFunctions.cpp
@@ +163,5 @@
> +                                              &args->value, 1) == JSObject::ED_OK);
> +}
> +
> +JSObject *
> +ion::ParExtendArray(ForkJoinSlice *slice, JSObject *array, uint32_t length)

Should this be a bool return?
```

The reason that this is not a bool return is to make code generation easier.  By returning `array` or NULL, the caller can just overwrite the register used to store `array` with the `ReturnReg` and then restore live registers (except for `array`).  I have converted StoreElementHole to use the same trick in place of that SaveLiveRegs hack, as follows:

```
        // The use of registers here is somewhat subtle.  We need to
        // save and restore the volatile registers but we also need to
        // preserve the ReturnReg. Normally we'd just add a constraint
        // to the regalloc, but since this is the slow path of a hot
        // instruction we don't want to do that.  So instead we push
        // the volatile registers but we don't save the register
        // `object`.  We will copy the ReturnReg into `object`.  The
        // function we are calling (`ParPush`) agrees to either return
        // `object` unchanged or NULL.  This way after we restore the
        // registers, we can examine `object` to know whether an error
        // occurred.
        RegisterSet saveSet(ins->safepoint()->liveRegs());
        saveSet.maybeTake(object);

        masm.PushRegsInMask(saveSet);
        masm.reserveStack(sizeof(ParPushArgs));
        masm.storePtr(object, Address(StackPointer, offsetof(ParPushArgs, object)));
        masm.storeConstantOrRegister(value, Address(StackPointer,
                                                    offsetof(ParPushArgs, value)));
        masm.movePtr(StackPointer, CallTempReg0);
        masm.setupUnalignedABICall(1, CallTempReg1);
        masm.passABIArg(CallTempReg0);
        masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, ParPush));
        masm.freeStack(sizeof(ParPushArgs));
        masm.movePtr(ReturnReg, object);
        masm.PopRegsInMask(saveSet);
        masm.branchTestPtr(Assembler::Zero, object, object, bail);
        masm.jump(ool->rejoin());
```

I also added more comments to try and make this more clear.  Dvander, does this seem reasonable?

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yeah, definitely - thanks for explaining.

Comment 20

[Niko Matsakis [:nmatsakis]]

I've gone through and integrated your comments, for the most part, and will be refreshing the patches.  Here are a few responses to various points:

> Probably a better place for this is in IonBuilder, in the top-level
> build() function. I'd just create it up-front and let DCE remove
> unused ones. Then you can just cache the MDefinition in IonBuilder.

This is in reference to the code to insert parSlice() near the top of the graph.  I originally had the design as you suggest, but the problem was that DCE would run and remove the MParSlice because it was not needed---then the ParallelArrayAnalysis would come along and insert instructions that required it.  I considered making the MParSlice immune to DCE, but it seems like it might be useful to purge it for very small functions where it might not be required, since it is a TLS lookup that is not free.  But perhaps this micro-optimization is not worth the trouble.

> Since you don't have an OSR block, I'd just create+insert this 
> unconditionally at the top, and replace everywhere (is there any reason not to?)

This is in reference to the undefined constant that we use to replace various operands in the case of removing code that is unsafe for parallel execution.  In fact, what you describe is precisely what the code is attempting to do---that is, it looks for the START instruction at the entry to the function and inserts the udef after that.  I did not want to insert it unconditionally at the front of the entry basic block because that would also go before the parameters, and I think I encountered some problems doing that.  I can add a comment to clarify, if nothing else.

> ::: js/src/ion/IonCaches.cpp
> @@ +668,4 @@
> >  
> >      // Need to set correct framePushed on the masm so that exit frame descriptors are
> >      // properly constructed.
> > +    masm.setFramePushed(script->ionScript()->frameSize());
> What was the reason for this change?

I am not sure, it is possible this was a merge failure.  I'll have to check with Shu.

> General question: why doesn't invalidation just invalidate 
> both the attached Ion scripts?

I *think* that there is no reason except that I was adapting the existing invalidation code, which operated on one JSScript at a time, in what seemed the most straightforward way.  But I can't think of a good reason not to invalidate both versions of a script simultaneously.

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 701095 [details] [diff] [review]
Patch 8: Add parallel compilation mode

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

::: js/src/ion/IonCaches.cpp
@@ +668,4 @@
>  
>      // Need to set correct framePushed on the masm so that exit frame descriptors are
>      // properly constructed.
> +    masm.setFramePushed(script->ionScript()->frameSize());

Regardless of the reason for it, it seems potentially flawed.  I am hitting assertion failures in my build of Firefox because in one case this script returns false from hasIonScript()

Comment 22

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 701095 [details] [diff] [review]
Patch 8: Add parallel compilation mode

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

::: js/src/ion/IonCaches.cpp
@@ +668,4 @@
>  
>      // Need to set correct framePushed on the masm so that exit frame descriptors are
>      // properly constructed.
> +    masm.setFramePushed(script->ionScript()->frameSize());

Its looking to me more and more like a merge failure injected at
  commit 2dd73c16ea635ca55074d214b524b8c8520f8d17

Comment 23

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 701095 [details] [diff] [review]
Patch 8: Add parallel compilation mode

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

::: js/src/ion/IonCaches.cpp
@@ +668,4 @@
>  
>      // Need to set correct framePushed on the masm so that exit frame descriptors are
>      // properly constructed.
> +    masm.setFramePushed(script->ionScript()->frameSize());

Its looking to me more and more like a merge failure injected at
  commit 2dd73c16ea635ca55074d214b524b8c8520f8d17

Comment 24

[Niko Matsakis [:nmatsakis]]

Attachment: Already reviewed contents.

This contains content that did not change significantly from previous patches.  Therefore, carrying over r+.

Comment 25

[Niko Matsakis [:nmatsakis]]

Attachment: Add rooting to per-thread data

Comment 26

[Niko Matsakis [:nmatsakis]]

Attachment: Change how interrupt in par mode works

An optimization: use the runtime->interrupt flag to signal interrupts in parallel code and inline the code that checks for this flag on backedges.  This does mean that we will signal interrupts when we hit parallel bailouts, which implies that we will call the user operation callback in that case.  I figured it was ok to invoke the user operation callback more often since we also run the callback in the case that a GC is requested, which is an internal JS event that is not under the user's control.

Comment 27

[Terrence Cole [:terrence]]

Comment on attachment 709976 [details] [diff] [review]
Add rooting to per-thread data

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

Please update CheckStackRoots in gc/Verifier.cpp as well. It shouldn't be too much different from the update to gc/RootMarking.cpp.

Comment 28

[Niko Matsakis [:nmatsakis]]

Attachment: Add rooting to per-thread data

Comment 29

[Terrence Cole [:terrence]]

Comment on attachment 710050 [details] [diff] [review]
Add rooting to per-thread data

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

Thanks.

::: js/src/gc/Verifier.cpp
@@ +196,5 @@
>      return false;
>  }
>  
> +static void
> +GatherRooters(Vector< Rooter, 0, SystemAllocPolicy> &rooters,

No space between < and Rooter.

@@ +259,1 @@
>      Vector< Rooter, 0, SystemAllocPolicy> rooters;

And fix the one I missed when we added this code too, while you are here.

Comment 30

[Niko Matsakis [:nmatsakis]]

Pushed to inbound after incorporating feedback:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80a21124ddbd

Prior try run:
https://tbpl.mozilla.org/?tree=Try&rev=5e11a26de747

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/80a21124ddbd