IonMonkey: Optimization levels

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 9 obsolete attachments)

WIP
105.63 KB, patch
Details | Diff | Splinter Review
17.02 KB, patch
h4writer
: review+
h4writer
: checkin+
Details | Diff | Splinter Review
7.43 KB, patch
h4writer
: review+
h4writer
: checkin+
Details | Diff | Splinter Review
4.08 KB, patch
jandem
: review+
h4writer
: checkin+
Details | Diff | Splinter Review
11.21 KB, patch
jandem
: review+
Details | Diff | Splinter Review
34.28 KB, patch
jandem
: review+
Details | Diff | Splinter Review
78.37 KB, patch
jandem
: review+
Details | Diff | Splinter Review
13.88 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.23 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.38 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.63 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.78 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.12 KB, patch
jandem
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
The idea of implementing optimizations levels has been floating around for already a long time, for different reasons and different consequences.

1) In the beginning stage of IM, it was thought that a tempered IM could be used as baseline compiler, before a dedicated baseline compiler, using the infrastructure of IM was created.

2) Raytrace inlines way to much and is background thread is initially throttled by compilation. And we loose a lot of time being stuck in baseline until that is finished. So being able to already compile a minor version that inlines less and compiles much faster interim, would help.

3) Backtracking regalloc should give some extra speed improvements, but is slower than the current used regalloc. So it can't get enabled without consequences. After the usual compile phase, the compiling thread is mostly quite. So with a higher usecount we could recompile with a higher optimization level that e.g. includes backtracking regalloc and more aggressive passes.

4) PJS compiles with the normal optimization passes. But since we are running it concurrent a better compilation/more aggressive optimization level, would make a lot of sense.

5) Enables more research/testing ...

This bug is about implementing a framework to allow this. After the framework is done, more research can be done on it (by others too).

To create this framework I implemented a prototype with two optimization passes over the last two weeks. The higher pass being the normal optimization pass and the lower optimization pass at useCount "100", without inlining, without licm/gvn. (So for idea 2). No real research has been done to pinpoint the usecount and what to disable and is totally fingerspitzengefühl.

(I only tried v8 atm)

- Without patch:
Richards: 28570
DeltaBlue: 30553
Crypto: 27085
RayTrace: 32675
EarleyBoyer: 31340
RegExp: 4099
Splay: 16420
NavierStokes: 29443
----
Score (version 7): 21634

- With patch:
Richards: 29933
DeltaBlue: 29859
Crypto: 26149
RayTrace: 38646
Should be compiling
EarleyBoyer: 30483
RegExp: 4129
Splay: 15884
NavierStokes: 29591
Score (version 7): 21926

So very big raytrace improvement. Little improvement for richards/e-b/regexp. Little regression for crypto. (I don't want to talk about Splay, since it is unreliable giving numbers)

So I'll polish the patches to land the infrastructure. This without the two optimization passes enabled. I'll land this after the infrastructure has landed and some more testing has been done (octane/ss/kraken)
(Assignee)

Updated

5 years ago
Depends on: 918278
(Assignee)

Comment 1

5 years ago
Created attachment 8334905 [details] [diff] [review]
WIP

This is mostly finished. Only --ion-eager and "ion.usecount.trigger" and --ion-uses-before-compile aren't working yet. Still doubting how to fit this best with the multiple levels.

This includes:
- splitting js_IonOptions into js_IonOptions and IonOptimizationsInfo
- add MIncUseCount again
- implement 'Recompile without invalidation'
- enable OSR recompile without invalidation
- mark scripts uninlinable in IonBuilder when not inlined yet
- remove options usesBeforeInlining/inlineUseCountRatio/usesBeforeInliningFactor/polyInlineMax
Assignee: nobody → hv1989
(Assignee)

Comment 3

5 years ago
Created attachment 8336121 [details] [diff] [review]
part1: Rename SetIonAssertGraphCoherency
Attachment #8336092 - Attachment is obsolete: true
Attachment #8336121 - Flags: review?(jdemooij)
(Assignee)

Comment 4

5 years ago
Created attachment 8336122 [details] [diff] [review]
part2: Enable compiling, while having an ionscript
Attachment #8336122 - Flags: review?(jdemooij)
(Assignee)

Comment 5

5 years ago
Created attachment 8336123 [details] [diff] [review]
patch3: Eagerly set script uninlineable
Attachment #8336123 - Flags: review?(jdemooij)
(Assignee)

Comment 6

5 years ago
Created attachment 8336124 [details] [diff] [review]
part4: Add MIncUseCount again
Attachment #8336124 - Flags: review?(jdemooij)
(Assignee)

Comment 7

5 years ago
Created attachment 8336125 [details] [diff] [review]
part5: Split IonOptions in IonOptions and IonOptimizations
Attachment #8336125 - Flags: review?(jdemooij)

Updated

5 years ago
Attachment #8336121 - Flags: review?(jdemooij) → review+
Comment on attachment 8336122 [details] [diff] [review]
part2: Enable compiling, while having an ionscript

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

Looks good. Can you post a separate patch to change the spew message in IonBuilder::build to say "Recompiling" instead of "Analyzing" if script() is recompiling?

::: js/src/jit/CodeGenerator.cpp
@@ +5875,5 @@
>      if (sps_.enabled())
>          ionScript->setHasSPSInstrumentation();
>  
> +    // We finished the new IonScript. Invalidate the current active IonScript,
> +    // so we can replace it with this newly (probably higher optimized) version.

Nit: s/newly/new

@@ +5878,5 @@
> +    // We finished the new IonScript. Invalidate the current active IonScript,
> +    // so we can replace it with this newly (probably higher optimized) version.
> +    if (HasIonScript(script, executionMode)) {
> +        // Do a normal invalidate, except don't cancel offThread compilations,
> +        // since that will cancel this compilation too.

Can we assert the existing script isRecompiling()?

::: js/src/jit/Ion.cpp
@@ +2024,5 @@
>  }
>  
>  MethodStatus
> +jit::Recompile(JSContext *cx, HandleScript script, BaselineFrame *osrFrame, jsbytecode *osrPc,
> +          bool constructing)

Nit: indentation.

@@ +2026,5 @@
>  MethodStatus
> +jit::Recompile(JSContext *cx, HandleScript script, BaselineFrame *osrFrame, jsbytecode *osrPc,
> +          bool constructing)
> +{
> +    if (script->hasIonScript() && script->ionScript()->isRecompiling())

The caller checks this too IIUC. Can we instead JS_ASSERT(script->ionScript()->isRecompiling()); ?

::: js/src/jit/Ion.h
@@ +350,5 @@
>  // Walk the stack and invalidate active Ion frames for the invalid scripts.
>  void Invalidate(types::TypeCompartment &types, FreeOp *fop,
> +                const Vector<types::RecompileInfo> &invalid, bool resetUses = true, bool cancelOffThread = true);
> +void Invalidate(JSContext *cx, const Vector<types::RecompileInfo> &invalid, bool resetUses = true, bool cancelOffThread = true);
> +bool Invalidate(JSContext *cx, JSScript *script, ExecutionMode mode, bool resetUses = true, bool cancelOffThread = true);

Nit: these lines are too long, also in the cpp file.

::: js/src/jit/IonCode.h
@@ +265,5 @@
>      // If non-null, the list of AsmJSModules
>      // that contain an optimized call directly into this IonScript.
>      Vector<DependentAsmJSModuleExit> *dependentAsmJSModules;
>  
> +    bool recompiling_;

Can you add this one after hasSPSInstrumentation_? That way we make use of some padding the compiler inserts between hasSPSInstrumentation_ and runtimeData_ I think. Also, this should have a short comment like the other members.

::: js/src/jit/VMFunctions.cpp
@@ +938,5 @@
> +        return true;
> +
> +    JS_ASSERT(script->hasIonScript());
> +
> +    Recompile(cx, script, nullptr, nullptr, isConstructing);

We should |return false;| if this returns Method_Error. I think that's the only value we have to handle here but please double check..
Attachment #8336122 - Flags: review?(jdemooij) → review+
Comment on attachment 8336123 [details] [diff] [review]
patch3: Eagerly set script uninlineable

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

::: js/src/jit/IonBuilder.cpp
@@ +3399,5 @@
>      // Try-catch within inline frames is not yet supported.
>      if (isInlineBuilder())
>          return abort("try-catch within inline frame");
> +    else
> +        script()->uninlineable = true;

Nit: "no else after return" style rule, here and below. I'd add the script()->uninlineable = true; immediately after the "return ...;" (no blank line between them) to make it clear they are related.
Attachment #8336123 - Flags: review?(jdemooij) → review+
(We could also set the |uninlineable| flag in BaselineCompiler and JS_ASSERT in IonBuilder that it's set - may help for scripts that we try to inline before Ion-compiling them.)
Comment on attachment 8336124 [details] [diff] [review]
part4: Add MIncUseCount again

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

::: js/src/jit/CodeGenerator.cpp
@@ +7875,5 @@
>  }
>  
> +typedef bool (*RecompileFn)(JSContext *);
> +static const VMFunction RecompileFnInfo =
> +    FunctionInfo<RecompileFn>(Recompile);

Nit: should fit on one line.

@@ +7884,5 @@
> +    Register useCount = ToRegister(ins->scratch());
> +
> +    masm.movePtr(ImmPtr(ins->mir()->script()->addressOfUseCount()), useCount);
> +    Address ptr(useCount, 0);
> +    masm.add32(Imm32(1), ptr);

Something like this should work I think, also for the comparison:

masm.add32(Imm32(1), AbsoluteAddress(ins->mir()->script()->addressOfUseCount())

That way you don't need the temp register.

::: js/src/jit/IonBuilder.cpp
@@ +801,5 @@
>          lazyArguments_ = MConstant::New(alloc(), MagicValue(JS_OPTIMIZED_ARGUMENTS));
>          current->add(lazyArguments_);
>      }
>  
> +    insertIncUseCount();

This is buildInline, but if we are inlining do we still need this check?

::: js/src/jit/MIR.h
@@ +8975,5 @@
>  
> +class MIncUseCount : public MNullaryInstruction
> +{
> +    JSScript *script_;
> +    uint32_t useCount_;

I was wondering if useCount_ == script->getUseCount(), but it's the threshold right? Maybe useCountThreshold_ or usesBeforeRecompile_?

@@ +8976,5 @@
> +class MIncUseCount : public MNullaryInstruction
> +{
> +    JSScript *script_;
> +    uint32_t useCount_;
> +    bool recompile_;

When will this be false? Why do we need the increment in that case?

@@ +8997,5 @@
> +
> +  public:
> +    INSTRUCTION_HEADER(IncUseCount);
> +
> +    static MIncUseCount *New(JSScript *script_, uint32_t useCount) {

Nit: add a TempAllocator &alloc argument like the other MIR instructions these days, and use new(alloc) below.
Attachment #8336124 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #11)
> Something like this should work I think, also for the comparison:
> 
> masm.add32(Imm32(1),
> AbsoluteAddress(ins->mir()->script()->addressOfUseCount())
> 
> That way you don't need the temp register.

This is less efficient especially on x64/ARM though, so maybe just ignore this comment.
(Assignee)

Comment 13

5 years ago
Created attachment 8336626 [details] [diff] [review]
part2: Enable compiling, while having an ionscript
Attachment #8336122 - Attachment is obsolete: true
Attachment #8336626 - Flags: review+
(Assignee)

Comment 14

5 years ago
Created attachment 8336673 [details] [diff] [review]
part1: Rename SetIonAssertGraphCoherency
Attachment #8336121 - Attachment is obsolete: true
Attachment #8336673 - Flags: review+
(Assignee)

Comment 15

5 years ago
Created attachment 8336680 [details] [diff] [review]
part3: Eagerly set script uninlineable

This will set uninlineable in baseline and only assert that this is indeed the case in IM.
Attachment #8336123 - Attachment is obsolete: true
Attachment #8336680 - Flags: review?(jdemooij)
(Assignee)

Comment 16

5 years ago
Created attachment 8336708 [details] [diff] [review]
part4: Add MIncUseCount again

MIncUseCount has two modes.
1) Just increase usecount
2) Increase usecount + test for threshold and OOL to recompile.

Not inlined functions will do (2) when there is a higher optimization level. Inlined functions will only do (1).

My thoughts about needing (1) for inlined functions, is because a function could be inline in 2 places, but not in a 3rd. As a result we will only count on the 3rd place to decide if we want higher optimization. As a result it could take much longer before the usecount of the function is high enough to recompile...
Attachment #8336124 - Attachment is obsolete: true
Attachment #8336708 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Attachment #8336708 - Attachment description: patch4 → part4: Add MIncUseCount again
Comment on attachment 8336680 [details] [diff] [review]
part3: Eagerly set script uninlineable

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

Thanks for doing this. This also helps Brian's off-thread IonBuilder work: see the last part of bug 938124 comment 2.

::: js/src/jit/BaselineCompiler.cpp
@@ +2495,5 @@
>  
>  bool
>  BaselineCompiler::emit_JSOP_THROW()
>  {
> +    // Ionmonkey can't inline function with JSOP_THROW.

Nit: s/function/functions

@@ +2510,5 @@
>  
>  bool
>  BaselineCompiler::emit_JSOP_TRY()
>  {
> +    // Ionmonkey can't inline function with JSOP_TRY.

Same here.
Attachment #8336680 - Flags: review?(jdemooij) → review+
Comment on attachment 8336125 [details] [diff] [review]
part5: Split IonOptions in IonOptions and IonOptimizations

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

Nice idea and patch! I like how asm.js is just another level.

::: js/src/jit/BaselineBailouts.cpp
@@ +11,5 @@
>  #include "vm/ArgumentsObject.h"
>  
>  #include "jsscriptinlines.h"
>  
> +#include "jit/Ion.h"

Nit: if we really need it, it should go right before jit/IonSpewer.h, before the *inlines. But nothing else in this file changes, so we probably don't need it?

::: js/src/jit/IonBuilder.cpp
@@ +3956,5 @@
>  
>      // Skip heuristics if we have an explicit hint to inline.
>      if (!targetScript->shouldInline) {
>          // Cap the inlining depth.
> +        if (optimizationInfo().isSmallFunction(targetScript)) {

It feels like smallFunctionMaxBytecodeLength doesn't really belong in optimizationInfo. Can we just define it as |static const uint32_t SmallFunctionMaxBytecodeLength = foo;| in this file, and keep IsSmallFunction?

@@ +5964,5 @@
> +    if (js_IonOptimizations.isLastLevel(level))
> +        return;
> +
> +    if (isInlineBuilder()) {
> +        current->add(MIncUseCount::New(script()));

As we discussed on IRC today, we should probably use the outer script + a recompile check, to make sure we're not running a hot loop in an inlined script without getting rid of the use count increment.

::: js/src/jit/IonOptions.h
@@ +49,5 @@
> +    bool forceRegisterAllocator;
> +    IonRegisterAllocator forcedRegisterAllocator;
> +    bool limitScriptSize;
> +    bool osr;
> +    uint32_t baselineUsesBeforeCompile;

This class also has baseline options, so we should probably rename it to JitOptions.h / JitOptions while you're here :)

::: js/src/jsworkers.cpp
@@ +93,5 @@
>      return true;
>  }
>  
> +bool
> +js::HasScheduled(JSContext *cx, JSScript *script)

jsworkers.cpp is also used for off-thread parsing and asm.js compilation. so IsIonCompilingOffThread would be clearer I think.

Also, where will we call this function? Do we really want to wait for compilations to finish?

::: js/src/moz.build
@@ +239,5 @@
>          'jit/IonCaches.cpp',
>          'jit/IonFrames.cpp',
>          'jit/IonMacroAssembler.cpp',
> +        'jit/IonOptimizationLevels.cpp',
> +        'jit/IonOptions.cpp',

IonOptions.cpp is not in this patch.

::: js/src/shell/js.cpp
@@ +5387,5 @@
>      }
>  
>      if (const char *str = op->getStringOption("ion-gvn")) {
> +        if (strcmp(str, "off") == 0) {
> +            jit::js_IonOptions.disableGvn = true;

I think it'd be clearer to call it enableGvn or just gvn, to avoid the double negation in !disableGvn.
Attachment #8336125 - Flags: review?(jdemooij)
Comment on attachment 8336708 [details] [diff] [review]
part4: Add MIncUseCount again

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

As we discussed on IRC, this instruction should always check the use count threshold and recompile, right? Also, if we do that, I'd slightly prefer the name MRecompileCheck because it does more than incrementing :)
Attachment #8336708 - Flags: review?(jdemooij)
(Assignee)

Comment 20

5 years ago
Created attachment 8340332 [details] [diff] [review]
part4: Add MRecompileCheck
Attachment #8336708 - Attachment is obsolete: true
Attachment #8340332 - Flags: review?(jdemooij)
(Assignee)

Comment 21

5 years ago
Created attachment 8340336 [details] [diff] [review]
part5: Split IonOptions in IonOptions and IonOptimizations

Addressed comments:
- HasScheduled was debugging code. Removed out of this patch.
- Moved smallFunctionMaxBytecodeLength back to JitOptions. I can understand that we might not want to have different ideas of small functions depending on optimization level.
- MRecompileCheck only increases topmost scripts usecount now
- IonOptions to JitOptions
Attachment #8340336 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Attachment #8336125 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 8340337 [details] [diff] [review]
part6: s/IonOptions/JitOptions
Attachment #8340337 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Attachment #8340332 - Attachment description: patch4: Add MRecompileCheck → part4: Add MRecompileCheck
Comment on attachment 8340332 [details] [diff] [review]
part4: Add MRecompileCheck

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

::: js/src/jit/MIR.h
@@ +9125,5 @@
> +{
> +    JSScript *script_;
> +    uint32_t recompileThreshold_;
> +
> +    MRecompileCheck(JSScript *script_, uint32_t recompileThreshold)

Nit: s/script_/script

@@ +9135,5 @@
> +
> +  public:
> +    INSTRUCTION_HEADER(RecompileCheck);
> +
> +    static MRecompileCheck *New(TempAllocator &alloc, JSScript *script_, uint32_t useCount) {

And here.
Attachment #8340332 - Flags: review?(jdemooij) → review+
Comment on attachment 8340337 [details] [diff] [review]
part6: s/IonOptions/JitOptions

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

Thanks.
Attachment #8340337 - Flags: review?(jdemooij) → review+
Comment on attachment 8340336 [details] [diff] [review]
part5: Split IonOptions in IonOptions and IonOptimizations

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

Nice, comments are mostly style nits, but some questions I'd like to see answered first.

::: js/src/jit/IonOptimizationLevels.cpp
@@ +12,5 @@
> +
> +namespace js {
> +namespace jit {
> +
> +//Global declaration

Nit: remove this comment, it's clear from the context that it's global.

@@ +78,5 @@
> +    switch (level) {
> +      case OPTIMIZATION_DONTCOMPILE:
> +        return OPTIMIZATION_NORMAL;
> +      case OPTIMIZATION_NORMAL:
> +        return OPTIMIZATION_NORMAL;

Asking for the next level of the highest level doesn't make much sense and is probably a bug in most cases. I think it's clearer to have a JS_ASSERT(!isLastLevel(level)); at the top of this method. isLastLevel can be |return level == constant|.

::: js/src/jit/IonOptimizationLevels.h
@@ +17,5 @@
> +{
> +    OPTIMIZATION_DONTCOMPILE,
> +    OPTIMIZATION_NORMAL,
> +    OPTIMIZATION_ASMJS,
> +    OPTIMIZATION_COUNT

Nit: we have other enums with all-uppercase values, but new style is Optimization_DontCompile, Optimization_AsmJS, etc I think (see for example MIRType_Int32).

@@ +173,5 @@
> +class OptimizationInfos
> +{
> +  private:
> +    OptimizationInfo infos_[OPTIMIZATION_COUNT - 1];
> +    uint32_t defaultUseCount_;

Nit: defaultUsesBeforeCompile_

I think this (and setEagerCompilation) belongs in IonOptions though, with the other overrides. Then OptimizationInfo::usesBeforeCompile() can check this value, like we handle the other overrides.

::: js/src/jit/JitOptions.cpp
@@ +11,5 @@
> +
> +namespace js {
> +namespace jit {
> +
> +//Global declaration

Nit: can remove this comment.

::: js/src/jit/MIR.h
@@ +9128,5 @@
>  
> +    // Increase the usecount of the provided script upon execution and test if
> +    // the usecount surpasses the threshold. Upon hit it will recompile the
> +    // top active script (i.e. the topmost script this check was backed in.
> +    // Not the passed script_).

Nit: s/backed/baked, and move this comment above the class definition to match the rest of this file.

But is this comment correct? I think we want to invalidate the (outer) IonScript, not the inlined script: the inlined script may not even have an IonScript.

::: js/src/jit/MIRGenerator.h
@@ +28,5 @@
>  namespace jit {
>  
>  class MBasicBlock;
>  class MIRGraph;
>  class MStart;

Nit: You can remove the IonOptimizationLevels.h include if you forward-declare OptimizationInfo here:

class OptimizationInfo;

::: js/src/jit/VMFunctions.cpp
@@ +14,5 @@
>  #include "jit/JitCompartment.h"
>  #include "vm/ArrayObject.h"
>  #include "vm/Debugger.h"
>  #include "vm/Interpreter.h"
> +#include "jsworkers.h"

Nit: Correct #include order is like this (we have a python script that checks this on tinderbox and will turn your build orange):

#include "jit/VMFunctions.h"

#include "jsworkers.h"   <---

#include "builtin/ParallelArray.h"
#include "builtin/TypedObject.h"
#include "frontend/BytecodeCompiler.h"

::: js/src/jsworkers.h
@@ +219,5 @@
>  bool
>  StartOffThreadAsmJSCompile(ExclusiveContext *cx, AsmJSParallelTask *asmData);
>  
> +bool
> +HasScheduled(JSContext *cx, JSScript *script);

Nit: can remove this too.
Attachment #8340336 - Flags: review?(jdemooij)
(Assignee)

Comment 26

5 years ago
Created attachment 8341018 [details] [diff] [review]
part5: Split IonOptions in IonOptions and IonOptimizations

(In reply to Jan de Mooij [:jandem] from comment #25)
> Asking for the next level of the highest level doesn't make much sense and
> is probably a bug in most cases. I think it's clearer to have a
> JS_ASSERT(!isLastLevel(level)); at the top of this method. isLastLevel can
> be |return level == constant|.

Yes, makes sense

> I think this (and setEagerCompilation) belongs in IonOptions though, with
> the other overrides. Then OptimizationInfo::usesBeforeCompile() can check
> this value, like we handle the other overrides.

Yeah IonOptions is the default place for such things. Don't know what I was thinking ;).

> But is this comment correct? I think we want to invalidate the (outer)
> IonScript, not the inlined script: the inlined script may not even have an
> IonScript.

I meant the outermost script. So the "script" the "IonScript" is currently running. Topmost referred to ionBuilder->callerBuilder_. So the ionBuilder without a callerBuilder_. I think the confusion is because on the stack the topmost script is indeed the inlined script. I updated the text, hopefully so it is easier to understand?
Attachment #8340336 - Attachment is obsolete: true
Attachment #8341018 - Flags: review?(jdemooij)
Comment on attachment 8341018 [details] [diff] [review]
part5: Split IonOptions in IonOptions and IonOptimizations

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

Thanks for making these changes. Beautiful!

::: js/src/jit/JitOptions.cpp
@@ +11,5 @@
> +
> +namespace js {
> +namespace jit {
> +
> +//Global declaration

Nit: can remove this comment

::: js/src/jit/JitOptions.h
@@ +10,5 @@
> +#include "jit/IonTypes.h"
> +
> +#ifdef JS_ION
> +
> +// TODO: explanation

Nit: add the explanation or remove this comment.

::: js/src/jit/MIR.h
@@ +9122,5 @@
>  };
>  
> +// Increase the usecount of the provided script upon execution and test if
> +// the usecount surpasses the threshold. Upon hit it will recompile the
> +// outermost script (i.e. the not the inlined script).

Nit: s/the not the/not the
Attachment #8341018 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 28

5 years ago
Created attachment 8342219 [details] [diff] [review]
part7: Use logic in UsesBeforeCompile to get actuall needed usecount

This should be the last part needed before starting to land.
Attachment #8342219 - Flags: review?(jdemooij)
(Assignee)

Comment 29

5 years ago
Created attachment 8342221 [details] [diff] [review]
part7: Use logic in UsesBeforeCompile to get actual needed usecount

Oops forgot to move some consts
Attachment #8342219 - Attachment is obsolete: true
Attachment #8342219 - Flags: review?(jdemooij)
Attachment #8342221 - Flags: review?(jdemooij)
(Assignee)

Comment 30

5 years ago
Created attachment 8342338 [details] [diff] [review]
part8: Enable reschedule same optimizationLevel if osrPc is different

Did some performance testing to make sure the patches don't alter performance as is. These patches should only lay down the infrastructure to have optimization levels, without altering the behaviour, yet.

I noticed the patches disabled recompiles of the same optimization level when osrPc differs. So rectified this. Differences are now in the noise for me for octane1.0/2.0 / SS / Kraken.

Note: these patches do actually change one behaviour. When needed to recompile a script due to osrPc mismatch using the background compiler, we only throw the script away when the IonScript finishes. (We used to do that before starting a compile and run in Baseline in the meantime.).
Attachment #8342338 - Flags: review?(jdemooij)
(Assignee)

Comment 31

5 years ago
Comment on attachment 8342221 [details] [diff] [review]
part7: Use logic in UsesBeforeCompile to get actual needed usecount

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

::: js/src/jit/Ion.cpp
@@ -2707,5 @@
>      MOZ_ASSUME_UNREACHABLE("No such execution mode");
>  }
>  
> -uint32_t
> -jit::UsesBeforeIonCompile(JSScript *script, jsbytecode *pc)

Also remove definition in Ion.h
Comment on attachment 8342221 [details] [diff] [review]
part7: Use logic in UsesBeforeCompile to get actual needed usecount

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

::: js/src/jit/IonBuilder.cpp
@@ +6017,5 @@
>      // Add recompile check to recompile when the usecount reaches the usecount
>      // of the next optimization level.
>      OptimizationLevel nextLevel = js_IonOptimizations.nextLevel(curLevel);
>      const OptimizationInfo *info = js_IonOptimizations.get(nextLevel);
> +    uint32_t useCount = info->usesBeforeCompile(topBuilder->script());

Can we pass pc here as well? usesBeforeCompile uses a different threshold for loops depending on their depth, so that we try to enter outer loops via OSR instead of inner loops. How does this work when we recompile? What osr-pc do we use? Reusing the one from the existing script seems dangerous when there are multiple loops: we may never reach our original osrPc again.
Attachment #8342221 - Flags: review?(jdemooij) → review+
Comment on attachment 8342338 [details] [diff] [review]
part8: Enable reschedule same optimizationLevel if osrPc is different

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

Good catch.
Attachment #8342338 - Flags: review?(jdemooij) → review+

Updated

5 years ago
Duplicate of this bug: 946424
Comment on attachment 8336680 [details] [diff] [review]
part3: Eagerly set script uninlineable

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

::: js/src/jit/IonBuilder.cpp
@@ +3451,5 @@
>      if (analysis().hasTryFinally())
>          return abort("Has try-finally");
>  
>      // Try-catch within inline frames is not yet supported.
> +    JS_ASSERT(script()->uninlineable && !isInlineBuilder());

Drive-by comment.  This is more readable as:

JS_ASSERT_IF(script()->uninlineable, !isInlineBuilder());

@@ +3562,5 @@
>  IonBuilder::ControlStatus
>  IonBuilder::processThrow()
>  {
>      // JSOP_THROW can't be compiled within inlined frames.
> +    JS_ASSERT(script()->uninlineable && !isInlineBuilder());

Same as above.

@@ +9078,5 @@
>      // required, then it means that any aliased argument set can never be observed, and
>      // the frame does not actually need to be updated with the new arg value.
>      if (info().argumentsAliasesFormals()) {
> +        // JSOP_SETARG with magic arguments within inline frames is not yet supported.
> +        JS_ASSERT(script()->uninlineable && !isInlineBuilder());

Same as above.
(Assignee)

Updated

5 years ago
Depends on: 947603
(Assignee)

Comment 36

5 years ago
(In reply to Jan de Mooij [:jandem] from comment #8)
> Looks good. Can you post a separate patch to change the spew message in
> IonBuilder::build to say "Recompiling" instead of "Analyzing" if script() is
> recompiling?

Needinfo myself to do this. I forgot this.
(Assignee)

Updated

5 years ago
Flags: needinfo?(hv1989)
(Assignee)

Comment 37

5 years ago
Created attachment 8344589 [details] [diff] [review]
part9: Spew 'recompiling' and optimization level
Attachment #8344589 - Flags: review?(jdemooij)
Flags: needinfo?(hv1989)
(Assignee)

Updated

5 years ago
Attachment #8336626 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8336673 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8336680 - Flags: checkin+

Updated

5 years ago
Attachment #8344589 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 38

5 years ago
(In reply to Kannan Vijayan [:djvj] from comment #35)
> > +    JS_ASSERT(script()->uninlineable && !isInlineBuilder());
> 
> Drive-by comment.  This is more readable as:
> 
> JS_ASSERT_IF(script()->uninlineable, !isInlineBuilder());

But it doesn't test the same! At that point script()->unlinineable should always be true!. That's what we are asserting.
This was also causing pretty frequent Android M3 failures like the below (confirmed they started on this push and ended after being backed out).
https://tbpl.mozilla.org/php/getParsedLog.php?id=31896506&tree=Mozilla-Inbound
Depends on: 949878
The Hf problem looks like it'll go away when bug 949878 lands: https://tbpl.mozilla.org/?tree=Try&rev=096ebef4f000
(Assignee)

Comment 43

5 years ago
Created attachment 8347950 [details] [diff] [review]
part10: Fixes round 1

- Brian added some extra asserts in bug 932982 and those were hitting with this bug. We agreed that first invalidating, before doing "FinishCompilation" should fix these.
- Second fix is to fix when we would enable optimization levels that "isIonEnabled" could be toggled during running in IonMonkey. As a result we could schedule a compile when we shouldn't.
Attachment #8347950 - Flags: review?(jdemooij)
(Assignee)

Comment 44

5 years ago
Created attachment 8347951 [details] [diff] [review]
Part11: Fixes round 2

Brian his patch. He found it and created the patch. So I'm leaving him as author. Mismatch of osrpc could happen.
Attachment #8347951 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Attachment #8347950 - Attachment description: part10: → part10: Fixes round 1

Updated

5 years ago
Attachment #8347950 - Flags: review?(jdemooij) → review+

Updated

5 years ago
Attachment #8347951 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 47

5 years ago
Created attachment 8350083 [details] [diff] [review]
Part12: Fix ffos regression

So we can't set ION_COMPILING_SCRIPT if we aren't background compiling. It is used to know if we are background compiling.
Attachment #8350083 - Flags: review?(jdemooij)
Comment on attachment 8350083 [details] [diff] [review]
Part12: Fix ffos regression

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

This is what we did before and it fixes the regression, so this looks good.
Attachment #8350083 - Flags: review?(jdemooij) → review+
Depends on: 953410
You need to log in before you can comment on or make changes to this bug.