781602 - Support multiple ion compilation modes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

For work on Rivertrail, we need to be able to compile in "parallel mode", which implies different behavior in some cases.  Currently this is being used to handle bailouts differently (included in the patch) but later there will need to be other changes as well that are mode dependent.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: First attempt at a patch.

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to permit multiple compilation modes.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to support multiple compilation modes.

I uploaded the wrong file previously.

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Tweaked version that works with debug builds

I realized I had forgotten to include the changes to debug sections in the previous patch.

Comment 5

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

Comment on attachment 677610 [details] [diff] [review]
Tweaked version that works with debug builds

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

::: js/src/jsgc.cpp
@@ +5766,5 @@
>          /* Discard Ion caches. */
> +        for (EACH_COMPILE_MODE(cmode)) {
> +            if (script->hasIonScript(cmode))
> +                script->ions[cmode]->purgeCaches(c);
> +        }

ion::PurgeCaches(JSScript*) - but do parallel compilations have ICs?

::: js/src/jsinterpinlines.h
@@ +1004,5 @@
>      bool useIon_;
>  #endif
>  
>    public:
> +    FastInvokeGuard(JSContext *cx, const Value &fval, CompileMode cmode)

Is this ever constructed with cmode == Parallel?

::: js/src/jsmemorymetrics.cpp
@@ +230,5 @@
>  # ifdef JS_ION
> +        for (EACH_COMPILE_MODE(cmode)) {
> +            if (script->hasIonScript(cmode))
> +                cStats->ionData +=
> +                    script->ions[cmode]->sizeOfIncludingThis(rtStats->mallocSizeOf);

Let's hide these details as well, maybe ion::MemoryUsed(JSScript *)?

::: js/src/jsscript.cpp
@@ +1899,5 @@
>      mjit::ReleaseScriptCode(fop, this);
>  # ifdef JS_ION
> +    for (EACH_COMPILE_MODE(cmode)) {
> +        if (hasIonScript(cmode))
> +            ion::IonScript::Destroy(fop, ions[cmode]);

For cases like these, I'd change Destroy/Trace to take a JSScript, and have it be responsible for figuring out which members to test and how to to test them.

::: js/src/jsscript.h
@@ +546,5 @@
>          return needsArgsObj() && !strictModeCode;
>      }
>  
> +    /* Information attached by Ion: there is (potentially) one script per mode */
> +    js::ion::IonScript *ions[js::COMPILE_MODE_MAX];

Are there situations where we'd want a script to have both a parallel and a sequential compilation? If not, I think we should limit to one IonScript attached to the JSScript. The array strikes me as a limited abstraction, since we could imagine other compilation modes not necessarily having an IonScript, and we're probably not going to have additional compilation modes anytime soon.

If this is something we want (I'd be interested in the justification), I'd just split this into two fields. We might duplicate and have to rename some accessors, but it'll be much clearer and a little more flexible.

For example, cases like:

  callee->canIonCompile(js::COMPILE_MODE_SEQ)

Would stay as "canIonCompile", reducing code motion, and new functions like "canUseRiverTrail"  could be added and used for where appropriate.

Comment 6

[Niko Matsakis [:nmatsakis]]

Regarding the decision to permit multiple IonScript's per JSScript:

It is certainly possible to have functions that execute in both parallel and sequential mode.  The various parallel APIs simply take a closure, and this closure could be invoked in multiple contexts.  

The parallel mode compilation cannot currently be used in a sequential context, because it assumes that appropriate parallel state is available.  For example, it performs memory allocation in thread-local arenas.  Even if you could execute the parallel version, it is prone to bailout in cases where the sequential version would not bailout.  It is also plausible to have parallel mode disabled (perhaps it has historically bailed out or something like that) and sequential mode enabled.

In the future, we will grow additional parallel compile modes.  Certainly we will soon have a mode for vectorized compilation, for example, and perhaps eventually GPU-based (though this might be an IonScript).  These modes may co-exist with non-vectorized parallel mode for the same reasons that parallel mode might co-exist with sequential mode.

Does that all make sense?

Regarding the other comments:

- Parallel modes don't have ICs now, but I think that is something that may make sense in the future.  It would vastly expand the scope of things that can be executed in parallel.

- FastInvokeGuard is not (today) used with parallel mode.  Perhaps it will never be.  But where convenient I tried to write code to work in both modes.

- I can certainly refactor for cases where we always want to affect all modes.

Comment 7

[Niko Matsakis [:nmatsakis]]

I just noticed your last comment regarding using two different fields rather than a single "ions" array.  The advantage of an array over separate fields, it seems to me, is that you can easily have code that operates generically over any IonScript that may be attached to a single JSScript.  The RecompilationInfo / invalidation code is an example.  It also means that code like this will continue to work seamlessly as we add more possible modes.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 677610 [details] [diff] [review]
Tweaked version that works with debug builds

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

::: js/src/ion/Bailouts.cpp
@@ +558,5 @@
>      JSScript *script = GetBailedJSScript(cx);
>  
> +    // According to nbp, this path is going away.  So I just
> +    // leave it using COMPILE_MODE_SEQ by default, not sure
> +    // if that's 100% correct. - nmatsakis

I am not sure we understood correctly each others.  It is bad to invalidate all CompilerOutputs from a script.  What this function do is invalidating the running Ion code (the one from which we just bailed out).

I am not a big fan of such function, because this mean that a constraint is not handled by the invalidation system, and thus might cause more trouble to maintain.  I openened Bug 808448 to cover this issue.

As long as you are not using any of the 3 guardshape using Bailout_Invalidate, you should be fine with assuming this function only deals with sequential mode.  These are setgname, getgname and TestCommonPropFunc (which is used by getprop and setprop).

::: js/src/jsinfer.cpp
@@ +2500,5 @@
>      mjit::JITScript *jit = co->script->getJIT(co->constructing, co->barriers);
>      bool hasJITCode = jit && jit->chunkDescriptor(co->chunkIndex).chunk;
>  
>  # if defined(JS_ION)
> +    hasJITCode |= !!co->script->hasAnyIonScript();

This need to be precise, we are dealing with one CompilerOutput, not any.

::: js/src/jsinfer.h
@@ +1221,2 @@
>      bool pendingRecompilation : 1;
>      uint32_t chunkIndex:28;

Can you reduce the chunkIndex accordingly, or just remove the bit field on the chunkIndex.

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: Revised version using separate fields for seq, par modes.

Comment 10

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

Comment on attachment 679055 [details] [diff] [review]
Revised version using separate fields for seq, par modes.

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

I like this patch much better, thanks!

::: js/src/ion/CodeGenerator.cpp
@@ +796,5 @@
>      masm.reserveStack(unusedStack);
>      return true;
>  }
>  
> +static int32_t ionOffset(CompileMode cmode) {

nit:

static inline int32
OffsetOfIonScript(CompileMode ...

::: js/src/ion/CompileInfo.h
@@ +22,5 @@
> +    SequentialCompilation,
> +
> +    // JavaScript code to be executed in parallel worker threads,
> +    // e.g. by ParallelArray
> +    ParallelCompilation

I just realized, "ParallelCompilation" may be really confusing with off-thread compilation in play. Should we commit to just strictly separating these terms, or find a new name for one or the other?

::: js/src/ion/CompileModeInlines.h
@@ +71,5 @@
> +    return false;
> +}
> +
> +static inline bool Disabled(JSScript *script,
> +                                      CompileMode cmode) {

You can fit the two argument names on the same line for everything in this file.

@@ +80,5 @@
> +    JS_ASSERT(false);
> +    return false;
> +}
> +
> +static inline types::CompilerOutput::Kind CompilerOutputKind(CompileMode cmode) {

nit: the starting { for the functions in this file should be on a new line

::: js/src/ion/Ion.cpp
@@ +1968,5 @@
> +        ion::IonScript::Destroy(fop, script->parallelIon);
> +}
> +
> +void
> +ion::TraceIonScripts(JSTracer* trc, JSScript *script) {

starting { for all these functions should be on a new line

::: js/src/jsinfer.h
@@ +1212,5 @@
>   * compilation. The compiler output is build by the AutoEnterCompilation.
>   */
>  struct CompilerOutput
>  {
> +    enum Kind { MethodJIT, Ion, ParallelIon };

nit: 
    enum Kind {
        MethodJIT,
        Ion,
        ParallelIon
    };

(Makes it easier to modify the enum)

::: js/src/jsscript.h
@@ +576,5 @@
>          JS_ASSERT(hasIonScript());
>          return ion;
>      }
>  
> +    void disableIonCompilation() {

Is this called from anywhere other than ion::Disable? It probably deserves a big warning (which the assert covers, but should be explained) - only ion::Disable is allowed to change a possibly non-NULL script->ion because it could break incremental GC.

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Add new execution mode, compiler and parallel, for ion-compiled scripts.

Addressed nits.  This is the version that was submitted to try server.

Comment 12

[Mozilla RelEng Bot]

Try run for 74ebe71146d3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=74ebe71146d3
Results (out of 57 total builds):
    success: 32
    warnings: 9
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-74ebe71146d3

Comment 13

[Mozilla RelEng Bot]

Try run for cc63c752ba65 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cc63c752ba65
Results (out of 214 total builds):
    success: 179
    warnings: 24
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cc63c752ba65

Comment 14

[Mozilla RelEng Bot]

Try run for cc63c752ba65 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cc63c752ba65
Results (out of 215 total builds):
    success: 179
    warnings: 24
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cc63c752ba65

Comment 15

[Mozilla RelEng Bot]

Try run for cf10fdc87941 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cf10fdc87941
Results (out of 306 total builds):
    success: 264
    warnings: 32
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cf10fdc87941

Comment 16

[Mozilla RelEng Bot]

Try run for 0d639bf28ce8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0d639bf28ce8
Results (out of 308 total builds):
    success: 270
    warnings: 31
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0d639bf28ce8

Comment 17

[Mozilla RelEng Bot]

Try run for 0d639bf28ce8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0d639bf28ce8
Results (out of 309 total builds):
    success: 270
    warnings: 31
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0d639bf28ce8

Comment 18

[Niko Matsakis [:nmatsakis]]

Attachment: Final version of multiple compilation mode patch.

This includes changes suggested by dvander as well as fixes for various problems encountered in the try server.

Comment 19

[Niko Matsakis [:nmatsakis]]

Final successful try run is here: https://tbpl.mozilla.org/?tree=Try&rev=646353f79b53

Comment 20

[Mozilla RelEng Bot]

Try run for 646353f79b53 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=646353f79b53
Results (out of 313 total builds):
    success: 292
    warnings: 20
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-646353f79b53

Comment 21

[Mozilla RelEng Bot]

Try run for 646353f79b53 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=646353f79b53
Results (out of 314 total builds):
    success: 292
    warnings: 20
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-646353f79b53

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7a2c6d72de

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a7a2c6d72de