807828 - Refactor allocation instructions in preparation for Parallel JS

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Refactors the IonMonkey allocation instructions to share a common codepath.  This same path is then used to implement parallel allocation (coming in a subsequent patch).

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Refactor various allocation instructions to use a common path.

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 677609 [details] [diff] [review]
Refactor various allocation instructions to use a common path.

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

Stealing review.

I'm unsure of the impetus behind the patch -- if allocations for parallel arrays need to use a different allocation mechanism, why can't they just use a different allocation mechanism without consolidating the other sequential paths? Will all run-time allocations be forced to go through this SeqAllocMode class -- will we have any room to have future allocation paths behave differently?

Note that this patch does change the behavior of much of the allocation code -- instead of bailing out to a NewArray-specific handler, all allocation paths now share the logic from visitCreateThis() that only calls out in the case of an empty freelist, then always performs object initialization inline. This may have an effect on performance -- possibly even positive -- but we must measure before landing. Microbenchmarks will likely be most useful.

But in general, the patch is fine with comments addressed -- I'm just not sure why it's necessary, whether it's intended to tie our hands to this modal system, and what effects it will have on allocation performance.

::: js/src/ion/CodeGenerator.cpp
@@ +1457,5 @@
>  
>      return true;
>  }
>  
> +class SeqAllocMode

This class doesn't match IonMonkey style -- please refer to existing classes to determine brace positioning, indentation, and function call structure.

@@ +1489,5 @@
> +    }
> +};
> +
> +template <typename T>
> +bool

template <typename T> bool

@@ +1492,5 @@
> +template <typename T>
> +bool
> +CodeGenerator::initNewGCThing(T allocMode,
> +                              JSObject *templateObject,
> +                              Register objReg) {

all on one line, brace on next line

@@ +1518,4 @@
>  bool
>  CodeGenerator::visitNewArray(LNewArray *lir)
>  {
> +    JS_ASSERT(gen->info().compileMode() == COMPILE_MODE_SEQ);

Could instantiating a SeqAllocMode check such an assertion instead?

@@ +1521,5 @@
> +    JS_ASSERT(gen->info().compileMode() == COMPILE_MODE_SEQ);
> +
> +    if (lir->mir()->shouldUseVM()) {
> +        return visitNewArrayCallVM(lir);
> +    }

One-line if statements must exclude braces.

::: js/src/ion/CodeGenerator.h
@@ +26,5 @@
>  class OutOfLineCache;
>  class OutOfLineStoreElementHole;
>  class OutOfLineTypeOfV;
>  class OutOfLineLoadTypedArray;
> +class OutOfLineParNew;

Probably should not be included in this patch.

@@ +214,5 @@
>  
> +    template <typename T>
> +    bool initNewGCThing(T allocMode,
> +                        JSObject *templateObject,
> +                        Register objReg);

All on one line, except "template <typename T>".

::: js/src/ion/Ion.cpp
@@ +757,5 @@
>  namespace js {
>  namespace ion {
>  
> +bool
> +OptimizeMIR(MIRGenerator *mir)

"OptimizeMIR" makes it sound like IonBuilder has already been run. I don't understand why CompileBackEnd() is being redefined to split graph logic into two phases -- it seems an unnecessary distinction.

@@ +921,5 @@
> +    return true;
> +}
> +
> +LIRGraph *
> +GenerateLIR(MIRGenerator *mir)

"GenerateLIR" makes it sound like this function does not perform register allocation.

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

This logic should not be attached to the MIR -- it should be as physically close to the codegen as possible. Static functions in CodeGenerator.cpp would be fine.

Ditto for MNewArray::shouldUseVM().

Comment 3

[Niko Matsakis [:nmatsakis]]

The only purpose of the patch is to avoid code duplication.  I could certainly "unfactor" it out.  At the time when I made these changes, I discussed them with you on IRC and was under the impression that the various distinct paths were considered out of date and that the correct model was the one which I implemented, but perhaps I misunderstood.  Is there an easy way for me to measure the performance impact?

Regarding the other comments:

- The split to |OptimizeMIR()| and |GenerateLIR()| as well as the addition of |shouldUseVM()| were introduced to support a later parallel compilation pass.  Probably I should not have put them as part of this patch, actually.  The reason that optimization and LIR generation is split is that, in parallel compilation mode, we do extra work in between those two steps.  The reason that the |shouldUseVM()| helper was introduced is so that the parallel array analysis can figure out whether this is an allocation path that can be handled simply or not.  

Would it be better if I moved the definition of |shouldUseVM()| method into the code generator?  Alternatively, I just could just define a function.  In any case, we will need to be able to figure out whether the VM should be used when doing the parallel analysis.

Comment 4

[Sean Stangl [:sstangl]]

Attachment: Regression microbenchmark

The attached microbenchmark regresses from 212ms -> 290ms with application of the patch in this bug.

Comment 5

[Niko Matsakis [:nmatsakis]]

Although I did not observe the regression locally (in fact, I observed increased performance), I have backed out this change locally and simply preserved the existing paths.