1147403 - Export IonMonkey graph with a Debugger hook.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

We should spew the compilation result in a format which can be processed by computers / humans.

This idea is not new:
 (1) It was suggested by Jim in June, as a way to produce a pseudo static analysis framework for addons/devtools.
 (2) It was also suggested by Hannes in September (euro-js-meetup), as a way to present JS-optimized version of the code to JS developers.
 (3) It was suggested by Benjamin in Bug 1092544 and I in September (euro-js-meetup), as a way to improve our test infrastructure.

(1) concerns the first phase of the compilation, and it might be triggered eagerly.
(2) concerns the last MIR phase of the compilation.
(3) concerns all MIR/LIR phases of the compilation, such that we can assert optimization results.

Currently the JS engine is capable to produce a JSON output for the MIR graph of all compilation phases, but it has multiple limitations to be used for all the previous use cases.

I think we should re-use the JSON output:
 - fix the off-main-thread compilation issue.
 - write in a buffer, and append in a file if required.
 - attach the buffer at the end of the IonScript if a compartment flag is enabled.
 - follow-up: remove the pretty-print aspect of the JSON spewer.
 - follow-up: compress these buffers.

Then we should make a testing function (3) to return this JSON output, such that we can write test cases with it.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Move Sprinter into its own header and add a FILE & LifoAlloc variants. r=

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - IonMonkey: Use GenericPrinter& instead of FILE* for *::dump functions.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Make IonSpewer work during off-thread compilation.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Add Bprinter class to copy LSprinter content to a pre-allocated buffer.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: part 5 - Append compilation graphs to IonScript when the conmpartment flag is set.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: part 6 - Add Debugger.Script.compilationGraph, to extract the graph of the latest valid Ion compilation.

This patch add the current JSON output of the compiler to the debugger.

This is not enabled by default when the Debugger is turned on, as the memory
consumption of the JSON is between 6x - 15x larger than the rest of the
IonScript content.

This patch lacks documentation for the moment, as I waiting for review
comments before making a last patch to update the documentation.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8594819 [details] [diff] [review]
part 6 - Add Debugger.Script.compilationGraph, to extract the graph of the latest valid Ion compilation.

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

The API as is seems limited, since it'll only return the IR graph associated with the current IonScript, if any.

What do you think about a hook, let's call it Debugger.Script.prototype.onIonCompile, that passes in JSON object on every successful Ion compilation? No need to save the save the string on the IonScript itself, and the user of the Debugger can do what it wants with recording every compilation.

::: js/src/jscompartment.h
@@ +365,5 @@
>          IsDebuggee = 1 << 0,
>          DebuggerObservesAllExecution = 1 << 1,
>          DebuggerObservesAsmJS = 1 << 2,
> +        DebuggerNeedsDelazification = 1 << 3,
> +        DebuggerObservesCompilationGraph = 1 << 4

Nit: could you move DebuggerObservesCompilationGraph to be above DebuggerNeedsDelazification?

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8593495 [details] [diff] [review]
part 1 - Move Sprinter into its own header and add a FILE & LifoAlloc variants. r=

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

Requesting re-review since I want to give myself to opportunity to have a second look at Printer.cpp.

::: js/src/vm/Printer.h
@@ +20,5 @@
> +// This class is useful to make generic printers which can work either with a
> +// file backend, with a buffer allocated with an ExclusiveContext or a link-list
> +// of chunks allocated with a LifoAlloc.
> +class GenericPrinter
> +{

Lets share some more things into GenericPrinter!
-> reportOutOfMemory(), hadOutOfMemory(), reportedOOM, printf() are similar for all Printers...

Also I think "put(const char* s)" should be similar and just call "put(const char* s, size_t len)" in all cases.
I know 'Fprinter::put' is more specialized, but since we have quite small strings it won't make a difference.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8593496 [details] [diff] [review]
part 2 - IonMonkey: Use GenericPrinter& instead of FILE* for *::dump functions.

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

::: js/src/jit/MIRGraph.cpp
@@ +1479,3 @@
>  {
>  #ifdef DEBUG
> +    FILE* fp = stderr;

For consistency:
Can you add dumpStack(GenericPrinter &out) and add the body of this function there.
And add a function "dumpStack()" which just create Fprinter(stderr) and calls the above function.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8594143 [details] [diff] [review]
part 4 - Add Bprinter class to copy LSprinter content to a pre-allocated buffer.

Removing review flag, as I might revise this patch since Shu's review suggest another approach.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8594144 [details] [diff] [review]
part 5 - Append compilation graphs to IonScript when the conmpartment flag is set.

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

Removing review flag, as I might revise this patch since Shu's review suggest another approach.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8593497 [details] [diff] [review]
part 3 - Make IonSpewer work during off-thread compilation.

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

First round. Didn't look at all corner cases, since all the changes in surrounding code etc... That way I can see the final form before r+'ing.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +7764,5 @@
>          // In the case of the change-heap function, no MIR is produced.
>          if (!mir)
>              continue;
>  
> +

Style nit: Don't add a newline here.

::: js/src/jit/JSONSpewer.cpp
@@ +22,1 @@
>  }

Remove empty destructor

::: js/src/jit/JitSpewer.cpp
@@ +42,5 @@
> +        MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
> +      public:
> +        OutputLock(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM);
> +        ~OutputLock();
> +    };

The default way is to have: AutoLockIonSpewer (not nested inside IonSpewer).
And having lock/unlock functions in IonSpewer.

@@ +67,5 @@
> +        incrementalLogging_ = incremental;
> +    }
> +    bool getIncrementalLogging() {
> +        return incrementalLogging_;
> +    }

s/Incremental/Async

@@ +365,5 @@
>              "  cacheflush Instruction Cache flushes (ARM only for now)\n"
>              "  range      Range Analysis\n"
>              "  unroll     Loop unrolling\n"
>              "  logs       C1 and JSON visualization logging\n"
> +            "  ilogs      Incremental C1 and JSON logging\n"

As discussed on irc:
logs-sync       Same as logs, but flushes between each consecutive pass (sync-only).

@@ +427,5 @@
>          EnableChannel(JitSpew_CacheFlush);
>      if (ContainsFlag(env, "logs"))
> +        EnableIonDebugLogging(false);
> +    if (ContainsFlag(env, "ilogs"))
> +        EnableIonDebugLogging(true);

Please refrain of using boolean params:
EnableIonDebugSyncLogging();
EnableIonDebugASyncLogging();

::: js/src/jit/JitSpewer.h
@@ +102,5 @@
>  static const int NULL_ID = -1;
>  
>  #ifdef DEBUG
>  
> +class GraphSpewer

IonCompilationSpewer

@@ +135,5 @@
>  
> +      public:
> +        Scope(MIRGenerator* mir)
> +            : mir_(mir)
> +        { }

Should this call: beginFunction ??

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> ::: js/src/jit/JitSpewer.h
> @@ +102,5 @@
> >  static const int NULL_ID = -1;
> >  
> >  #ifdef DEBUG
> >  
> > +class GraphSpewer
> 
> IonCompilationSpewer

This class spew only the graph, not the rest of the spew made during the compilation.  Also, it is used for AsmJS and Ion compilation, but it can also be used to spew the MIR graph made by baseline for running the analysis.

So calling it IonCompilationSpewer sounds too restrictive, when the sole purpose of this class is to spew the MIR and LIR graphs.

Knowing that this class is already under the js::jit:: namespace, I think naming it js::jit::GraphSpewer should be clear enough.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> @@ +135,5 @@
> >  
> > +      public:
> > +        Scope(MIRGenerator* mir)
> > +            : mir_(mir)
> > +        { }
> 
> Should this call: beginFunction ??

I thought this was a good idea, but I realized that doing so would imply that we are no longer able to report IonBuilder failures to build the SSA graph for a function.

Comment 15

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Move Sprinter into its own header and add a FILE & LifoAlloc variants.

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Make IonSpewer work during off-thread compilation.

Comment 17

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Extract the printer from the serializer.

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: part 5 - Add Debugger::onIonCompilation hook.

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: part 6 - Remove GetJitContext from serializing functions.

Comment 20

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Extract the printer from the serializer.

Delta:
 - Remove unused headers (fix circular dependecy reported by check-style)

Comment 21

[Nicolas B. Pierron [:nbp]]

Attachment: part 7 - Fix inIon, only reset the counter when the function is executed. r=

This patch fix the test case, to check at every iteration instead of the
first one, and fix the inIon predicate used by this test case in order to
avoid a gczeal issue with code which is being compiled but never executed.

The issue was with --ion-eager --ion-offthread-compile=off, the code got
successfully compiled, but when Debugger testing function got executed in
the same compartment, then a GC will discard the compilation.  But by using
setIonScript, we reseted the counter too early, and never ran any code where
inIon() == true.  This caused an infinite loop.

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 8599904 [details] [diff] [review]
part 5 - Add Debugger::onIonCompilation hook.

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

This looks like it'll be really fun to play with!

In addition to the comments, what do you think about the following changes? I think they'll improve the API ergonomics:

- Make onIonCompilation live on Debugger.Script instead of Debugger itself.
- Have CodeGenerator do the JSONParse and pass onIonCompilation a JSObject of the JSON.

Also, please document the method in js/src/doc/Debugger/. The Debugger API is one of the few well-documented pieces of SM. :)

::: js/src/jit-test/tests/debug/Debugger-onIonCompilation.js
@@ +66,5 @@
> +    test();         // Wait until the next Ion compilation.
> +  }
> +}
> +
> +// With the compilation graph inhibited, we should have no output.

I don't understand. What should have no output? The onIonCompilation hook is still being called.

::: js/src/jit/CodeGenerator.h
@@ +55,5 @@
>    public:
>      bool generate();
>      bool generateAsmJS(AsmJSFunctionLabels* labels);
>      bool link(JSContext* cx, CompilerConstraintList* constraints);
> +    void dispatchCompilationHook(JSContext* cx, HandleScript script);

I don't think this method is needed. Since Debugger::onIonCompilation is inline already, we can call that directly and let it handle the fast reject.

::: js/src/jit/Ion.cpp
@@ +479,5 @@
>          }
>      }
>  
> +    if (success) {
> +        // Without AutoEnterAnalysis scope.

Ah, good call.

::: js/src/jit/MIR.h
@@ -752,5 @@
>      }
>  
>      void setVirtualRegister(uint32_t vreg) {
>          virtualRegister_ = vreg;
> -#ifdef DEBUG

Why was this ifdef removed?

::: js/src/vm/Debugger-inl.h
@@ +66,5 @@
> +
> +    for (Debugger* dbg = cx->runtime()->debuggerList.getFirst(); dbg; dbg = dbg->getNext()) {
> +        if (dbg->getHook(OnIonCompilation))
> +            dbg->fireOnIonCompilationHook(cx, script, graph);
> +    }

This function isn't quite right and could be improved.

We shouldn't be dispatching for every Debugger in the runtime. If possible, this should call Debugger::dispatchHook. That function handles dispatching on cx->global(). It also handles mutation of the hooks since we're re-entering JS.

Debugger::dispatchHook returns a JSTrapStatus, but you can make fireOnIonCompilationHook handle exceptions as you do now and assert that dispatchHook here always returns JSTRAP_CONTINUE.

::: js/src/vm/Debugger.h
@@ +590,5 @@
>      /*
> +     * Receive a "jit compilation" event from the engine. A Jit compilation with
> +     * the given summary just got linked.
> +     */
> +    void fireOnIonCompilationHook(JSContext* cx, HandleScript script, jit::MIRGraph& graph);

Could this hook (and the JS hook) take a JSObject of the JSON object itself, and not expose the compiler-internal MIRGraph to Debugger?

Comment 23

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #22)
> - Make onIonCompilation live on Debugger.Script instead of Debugger itself.

Ok, I will check to see if I can do that easily.

> - Make onIonCompilation live on Debugger.Script instead of Debugger itself.

Can we set a hook on Debugger.Script.prototype.onIonCompilation, and make it used when any script is compiled?  Otherwise, as I understand it, we would have to set a new hook each time a new script is created, right?

> - Have CodeGenerator do the JSONParse and pass onIonCompilation a JSObject
> of the JSON.

The problem I see with having the JSObject done by the CodeGenerator, is that this will change the allocation made by the compartment which is being analyzed.

Also, I do not exactly know how this would play with a remote debugger which is hooked on a Phone, as we would be doing the processing of the JSON / JSObject on the remote computer.

I guess I can do the serialization as part of the CodeGenerator, and give the serialized string to the debugger, to let it do the JSONParse for each Debugger instance.  But then, in case of a remote debugger, we would be parsing JSON, serializing JSObjects, and deserializing JSObjects?

I think a JSString of the JSON might be a better approach.  What do you think?

> ::: js/src/jit-test/tests/debug/Debugger-onIonCompilation.js
> @@ +66,5 @@
> > +    test();         // Wait until the next Ion compilation.
> > +  }
> > +}
> > +
> > +// With the compilation graph inhibited, we should have no output.
> 
> I don't understand. What should have no output? The onIonCompilation hook is
> still being called.

Sounds like an outdated comment. I will fix it.

> ::: js/src/jit/MIR.h
> @@ -752,5 @@
> >      }
> >  
> >      void setVirtualRegister(uint32_t vreg) {
> >          virtualRegister_ = vreg;
> > -#ifdef DEBUG
> 
> Why was this ifdef removed?

This ifdef has to be removed, otherwise we have issue when we serialize the MIR graph.  The problem is that the virtualRegister_ field is in an union with the MDefinition pointer to the dependency_ field.  The isLowered flag is used to inform the JSONSpewer that we should not spew the dependency_->id(), which causes a SEGV.

This is also a modif that I always make when I want to enable the Spew in optimized builds.

Comment 24

[Nicolas B. Pierron [:nbp]]

Attachment: part 0 - Replace contextual information of dispatchHook by lambdas.

This patch is a refactoring patch which is a part of comment 22 fixes,
concerning the use of Debugger::dispatchHook.

The current Debugger::dispatchHook function is made such that only a few
which are listed in an assertion are dispatched.  The problem with this
approach is that the dispatchHook function has to handle all the arguments
which are used by the hooks, and use the same predicate for all the hooks.

Currently, 2 other functions have their own specialized copy of the
dispatchHook function.  In order to avoid adding a 3rd copy, this patch
strip the variable parts of dispatchHook and move them into lambda functions
given as argument.

Comment 25

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Move Sprinter into its own header and add a FILE & LifoAlloc variants.

Delta:
 - Use the same alignment for the allocated length as the LifoAlloc
   allocator function.  Doing so prevent us wasting space due to multiple
   consecutive small allocations which are not multiple of LIFO_ALLOC_ALIGN
   (== 8), and ensure that we benefit more frequently (all the time) from
   the merging code.

Comment 26

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8599902 [details] [diff] [review]
part 3 - Make IonSpewer work during off-thread compilation.

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

::: js/src/jit/JitSpewer.cpp
@@ +78,5 @@
> +{
> +  private:
> +    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
> +  public:
> +    AutoLockIonSpewerOutput(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM);

self-nit: I added an explicit keyword, as enforced by the style guide.

Comment 27

[Nicolas B. Pierron [:nbp]]

Attachment: part 5 - Add Debugger::onIonCompilation hook.

Deltas:
 - Replace the CodeGenerator by calls to Debugger::onIonCompilation.
 - Add a static function in Ion.cpp to prepare the arguments of
   Debugger::onIonCompilation. We have to serialize the MIRGraph before
   risking any GC which might invalidate any pointers. The AutoEnterAnalysis
   section prevents any GC, which ensure that the MIRGraph is safe to read.
   As a side-benefit, this removes the MIRGraph from the Debugger files.
 - Use the new dispatchHook (part 0) for slowPathOnIonCompilation function.
 - Give a list of scripts instead of one script to the debugger. Each script
   correspond to the index of each block in the graphs.

Comment 28

[Hannes Verschore [:h4writer]]

Comment on attachment 8599901 [details] [diff] [review]
part 1 - Move Sprinter into its own header and add a FILE & LifoAlloc variants.

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

::: js/src/vm/Printer.cpp
@@ +513,5 @@
> +int
> +LSprinter::put(const char* s, size_t len)
> +{
> +    size_t origLen = len;
> +    if (unused_ && tail_) {

unused_ > 0

@@ +521,5 @@
> +        len -= minLen;
> +        s += minLen;
> +    }
> +
> +    if (!len)

len == 0

Comment 29

[Hannes Verschore [:h4writer]]

Comment on attachment 8604675 [details] [diff] [review]
part 1 - Move Sprinter into its own header and add a FILE & LifoAlloc variants.

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

Please also apply the review comments in the previous post.

::: js/src/vm/Printer.cpp
@@ +527,5 @@
> +
> +    static_assert(sizeof(Chunk) % js::detail::LIFO_ALLOC_ALIGN == 0,
> +                  "The Chunk size is no longer a multiple of LIFO_ALLOC_ALIGN,\n"
> +                  "we should add it in the computation of allocLength.");
> +    size_t allocLength = AlignBytes(len, js::detail::LIFO_ALLOC_ALIGN);

Instead of the static assert. Why not do "AlignBytes(len+sizeof(Chunk))" already?
I think that would be better.

Comment 30

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #29)
> Comment on attachment 8604675 [details] [diff] [review]
> part 1 - Move Sprinter into its own header and add a FILE & LifoAlloc
> variants.
> 
> Please also apply the review comments in the previous post.

Done.

> Instead of the static assert. Why not do "AlignBytes(len+sizeof(Chunk))"
> already?
> I think that would be better.

Indeed this is better, I wanted to avoid the subtraction of sizeof(Chunk), but in fact I can move this subtraction to the else branch, which is less used as we are rarely mixing printers & allocations.

Comment 31

[Hannes Verschore [:h4writer]]

Comment on attachment 8599902 [details] [diff] [review]
part 3 - Make IonSpewer work during off-thread compilation.

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

Last time the "scope" name/function also bothered me.
1) Can you rename it to "AutoSpewFunction"
2) Make that the constructor calls (*mir)->graphSpewer().beginFunction(XXX); so it is balanced with the deconstructor (which calls endFunction())
3) Can you move it out of the jit::GraphSpewer class.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +7692,5 @@
>  
>      *mir = f.extractMIR();
>      (*mir)->initMinAsmJSHeapLength(m.minHeapLength());
> +    (*mir)->graphSpewer().init(&(*mir)->graph(), nullptr);
> +    (*mir)->graphSpewer().beginFunction(nullptr);

remove the "beginFunction" here.

@@ +7770,5 @@
>  
>          int64_t before = PRMJ_Now();
>  
>          JitContext jcx(m.cx(), &mir->alloc());
> +        jit::GraphSpewer::Scope spewerScope(mir);

This will now call the beginFunction

::: js/src/jit/JitSpewer.cpp
@@ +137,1 @@
>      EnableChannel(JitSpew_IonLogs);

Ah I forgot what this did before and why we only do this for sync logging.

Can you rename JitSpew_IonLogs to JitSpew_IonSyncLogs.

@@ +382,5 @@
>              "  cacheflush Instruction Cache flushes (ARM only for now)\n"
>              "  range      Range Analysis\n"
>              "  unroll     Loop unrolling\n"
>              "  logs       C1 and JSON visualization logging\n"
> +            "  logs-sync  Same as logs, but flushes between each pass (sync-only).\n"

(sync. compiled functions only)

::: js/src/vm/HelperThreads.cpp
@@ +1053,5 @@
>                              asmData->mir->compartment,
>                              &asmData->mir->alloc());
>  
>          int64_t before = PRMJ_Now();
> +        jit::GraphSpewer::Scope spewerScope(asmData->mir);

This will now call the beginFunction. This looks fine at first sight.

Comment 32

[Hannes Verschore [:h4writer]]

Comment on attachment 8599905 [details] [diff] [review]
part 6 - Remove GetJitContext from serializing functions.

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

::: js/src/jit/Ion.cpp
@@ +459,3 @@
>      bool success = false;
>      if (codegen) {
> +        JitContext jctx(cx, &builder->alloc());

As far as I can tell this looks correct. If unsure, please ask somebody else to double-check this.

Comment 33

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8604679 [details] [diff] [review]
part 5 - Add Debugger::onIonCompilation hook.

Note: Oops, I forgot to add the documentation.

Comment 34

[Nicolas B. Pierron [:nbp]]

Attachment: part 5 - Add Debugger::onIonCompilation hook.

Delta:
 - Add Debugger for onIonCompilation documentation.

Comment 35

[Shu-yu Guo [:shu]]

Comment on attachment 8604140 [details] [diff] [review]
part 0 - Replace contextual information of dispatchHook by lambdas.

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

Nice refactor.

::: js/src/vm/Debugger.cpp
@@ +688,5 @@
>  Debugger::slowPathOnDebuggerStatement(JSContext* cx, AbstractFramePtr frame)
>  {
>      RootedValue rval(cx);
> +    JSTrapStatus status = dispatchHook(
> +        cx, [](Debugger* dbg) -> bool { return dbg->getHook(OnDebuggerStatement); },

Style nit: I'd rather have the lambda on its own line.

@@ +1332,5 @@
>          handleUncaughtException(ac, true);
>  }
>  
>  /* static */ JSTrapStatus
> +Debugger::dispatchHook(JSContext* cx, std::function<bool (Debugger*)> enabled,

|enabled| is a poor name, how about |hookIsEnabled|?

@@ +1333,5 @@
>  }
>  
>  /* static */ JSTrapStatus
> +Debugger::dispatchHook(JSContext* cx, std::function<bool (Debugger*)> enabled,
> +                       std::function<JSTrapStatus (Debugger*)> fire)

Similarly, how about |fireHook|?

@@ +1380,5 @@
> +        cx,
> +        [script](Debugger* dbg) -> bool {
> +            return dbg->observesNewScript() && dbg->observesScript(script);
> +        },
> +        [=](Debugger* dbg) -> JSTrapStatus {

Why is this = instead of &?

@@ +1754,5 @@
>      MOZ_ASSERT(hook == OnNewPromise || hook == OnPromiseSettled);
>      RootedValue rval(cx);
>  
> +    JSTrapStatus status = dispatchHook(
> +        cx, [hook](Debugger* dbg) -> bool { return dbg->getHook(hook); },

Style nit: lambda on its own line

@@ +1770,3 @@
>      // Promise hooks are infallible and we ignore errors from uncaught
>      // exceptions by design.
> +    MOZ_ASSERT(status == JSTRAP_CONTINUE);

I think this assertion will fail. Previously other JSTRAP_STATUS values like JSTRAP_{RETURN,THROW} were ignored, not converted into JSTRAP_CONTINUE. Make the fire lambda above always return JSTRAP_CONTINUE?

::: js/src/vm/Debugger.h
@@ +555,5 @@
>      static bool slowPathOnLogAllocationSite(JSContext* cx, HandleObject obj, HandleSavedFrame frame,
>                                              int64_t when, GlobalObject::DebuggerVector& dbgs);
>      static void slowPathPromiseHook(JSContext* cx, Hook hook, HandleObject promise);
> +    static JSTrapStatus dispatchHook(JSContext* cx, std::function<bool (Debugger*)> enabled,
> +                                     std::function<JSTrapStatus (Debugger*)> fire);

Not a big deal, but is it too much trouble to take a template param instead of std::function?

Comment 36

[Shu-yu Guo [:shu]]

Comment on attachment 8604807 [details] [diff] [review]
part 5 - Add Debugger::onIonCompilation hook.

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

r=me with comments addressed and that async logging question answered.

::: js/src/doc/Debugger/Debugger.md
@@ +258,5 @@
> +        frequent modifications.
> +
> +    `scripts`
> +    :   Array of [`Debugger.Script`][scripts] associated with each
> +        block of the [SSA form][ssa-form] used in the <i>json</i> string.

"Associate" is vague. How about "for a block at `mir.blocks[i]` or `lir.blocks[i]` in the JSON, `scripts[i]` is the `Debugger.Script` containing that block's code."?

@@ +261,5 @@
> +    :   Array of [`Debugger.Script`][scripts] associated with each
> +        block of the [SSA form][ssa-form] used in the <i>json</i> string.
> +
> +    This method's return value is ignored.
> +

I think all your uses of <i></i> here actually could be backticks ` `, since they're property names.

::: js/src/jit/Ion.cpp
@@ +400,5 @@
> +//
> +// This function must be called in the same AutoEnterAnalysis section as the
> +// CodeGenerator::link. Failing to do so might leave room to interleave other
> +// allocations which can invalidate any JSObject / JSFunction referenced by the
> +// MIRGraph.

Please comment that the return value for this function isn't to indicate error, but to indicate whether Debugger::onIonCompilation should be called.

@@ +418,5 @@
> +        return false;
> +    }
> +
> +    // Collect the list of scripts which are inlined in the MIRGraph.
> +    for (jit::MBasicBlockIterator block(graph.begin()); block != graph.end(); block++)

Does this iterator iterate in ascending order of block id?

@@ +517,3 @@
>      RootedScript script(cx, builder->script());
>  
> +    // see PrepareForDebuggerOnIonCompilationHook

Style nit: capitalize See.

@@ +1739,5 @@
>                  continue;
>              }
>          }
>  
> +        // see PrepareForDebuggerOnIonCompilationHook

Style nit: capitalize See.

@@ +2057,5 @@
>  
>          return AbortReason_NoAbort;
>      }
>  
> +    // see PrepareForDebuggerOnIonCompilationHook

Style nit: capitalize See.

::: js/src/jit/JitSpewer.cpp
@@ +197,3 @@
>      // as this is useful in case of failure during the compilation. On the other
>      // hand, it is recommended to disabled off main thread compilation.
> +    if (!getAsyncLogging() && !firstFunction_) {

Why do we need to lock without async logging? I don't understand this change.

::: js/src/vm/Debugger.h
@@ +590,5 @@
>  
>      /*
> +     * Receive a "jit compilation" event from the engine. A Jit compilation with
> +     * the given summary just got linked.
> +     */

Style nit: consistent capitalization of JIT.

Comment 37

[Jan de Mooij [:jandem]]

Comment on attachment 8600954 [details] [diff] [review]
part 7 - Fix inIon, only reset the counter when the function is executed. r=

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

::: js/src/builtin/TestingFunctions.cpp
@@ +1547,5 @@
>      }
>  
> +    ScriptFrameIter iter(cx);
> +    if (iter.isIon()) {
> +        // reset the counter of the IonScript's script.

Nit: s/reset/Reset

@@ +1552,5 @@
> +        JitFrameIterator jitIter(cx);
> +        ++jitIter;
> +        jitIter.script()->resetWarmUpResetCounter();
> +    } else {
> +        // Check if we miss multiple attempts at compiling the innermost script.

Nit: missed

Comment 38

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #35)
> Comment on attachment 8604140 [details] [diff] [review]
> part 0 - Replace contextual information of dispatchHook by lambdas.
> 
> @@ +1333,5 @@
> >  }
> >  
> >  /* static */ JSTrapStatus
> > +Debugger::dispatchHook(JSContext* cx, std::function<bool (Debugger*)> enabled,
> > +                       std::function<JSTrapStatus (Debugger*)> fire)
> 
> |enabled| is a poor name, how about |hookIsEnabled|?
> Similarly, how about |fireHook|?

Sounds good.

> @@ +1380,5 @@
> > +        cx,
> > +        [script](Debugger* dbg) -> bool {
> > +            return dbg->observesNewScript() && dbg->observesScript(script);
> > +        },
> > +        [=](Debugger* dbg) -> JSTrapStatus {
> 
> Why is this = instead of &?

The idea was simple, the JSContext*, and the HandleScript can be copied without any issues.
> ::: js/src/vm/Debugger.h
> @@ +555,5 @@
> >      static bool slowPathOnLogAllocationSite(JSContext* cx, HandleObject obj, HandleSavedFrame frame,
> >                                              int64_t when, GlobalObject::DebuggerVector& dbgs);
> >      static void slowPathPromiseHook(JSContext* cx, Hook hook, HandleObject promise);
> > +    static JSTrapStatus dispatchHook(JSContext* cx, std::function<bool (Debugger*)> enabled,
> > +                                     std::function<JSTrapStatus (Debugger*)> fire);
> 
> Not a big deal, but is it too much trouble to take a template param instead
> of std::function?

To be honest, the type of the function is the same for all instances. Thus using template will only obscure the type of the argument. I think typing manually functions is better for getting nicer error messages.

If we are looking for better performances, then we should probably specialize the dispatchHook function with the Hook as a template parameter of the function, and avoid using lambdas.

(In reply to Shu-yu Guo [:shu] from comment #36)
> Comment on attachment 8604807 [details] [diff] [review]
> part 5 - Add Debugger::onIonCompilation hook.
> 
> ::: js/src/doc/Debugger/Debugger.md
> @@ +258,5 @@
> > +        frequent modifications.
> > +
> > +    `scripts`
> > +    :   Array of [`Debugger.Script`][scripts] associated with each
> > +        block of the [SSA form][ssa-form] used in the <i>json</i> string.
> 
> "Associate" is vague. How about "for a block at `mir.blocks[i]` or
> `lir.blocks[i]` in the JSON, `scripts[i]` is the `Debugger.Script`
> containing that block's code."?

Sounds better. :)

> @@ +418,5 @@
> > +        return false;
> > +    }
> > +
> > +    // Collect the list of scripts which are inlined in the MIRGraph.
> > +    for (jit::MBasicBlockIterator block(graph.begin()); block != graph.end(); block++)
> 
> Does this iterator iterate in ascending order of block id?

Yes, which is the same loop as done in the JSONSpewer::spewMIR and JSONSpewer::spewLIR.

> ::: js/src/jit/JitSpewer.cpp
> @@ +197,3 @@
> >      // as this is useful in case of failure during the compilation. On the other
> >      // hand, it is recommended to disabled off main thread compilation.
> > +    if (!getAsyncLogging() && !firstFunction_) {
> 
> Why do we need to lock without async logging? I don't understand this change.

Oh, I folded this in the wrong patch.  This was suppose to amend a typo in part 3.

To make it short, currently we have IONFLAGS=logs which only works with --ion-offthread-compile=off. With these patches, we have IONFLAGS=logs, which works out of the main thread, and IONFLAGS=logs-sync which works with --ion-offthread-compile=off.

The "logs" mode has one issue which is that all phases are spewed at once. Thus "logs-sync" mode is still interesting when debugging Ion compilation critical failures, as this give us a step-by-step view before the failure.  In which case, we want to spew everything as we go, thus we want to spew the comma which separates 2 functions, as soon as we know that we are spewing a new function.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/443b1a2a084f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1abceaf0f6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a4b4c31c94
https://hg.mozilla.org/integration/mozilla-inbound/rev/615c601284e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee01e148887
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ab1f789052
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb1dcf35e04
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1018d31e591

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/55bbf00eb10a

https://treeherder.mozilla.org/logviewer.html#?job_id=9884677&repo=mozilla-inbound

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Also confirmed to have caused frequent Mulet mochitest/reftest timeouts. And widespread talos timeouts across multiple platforms.

Comment 42

[Nicolas B. Pierron [:nbp]]

Attachment: fixup part 5 - Move the AutoUnlockHelperThreadState sooner, including before the AutoEnterAnalysis scope.

This patch would be folded in part 5.

This modification follows the modification of the part 5, where the
AutoEnterAnalysis RAII is moved from the top-level of the function body to
only surround the codegen->link call.

This modification unlock the helper thread before the AutoEnterAnalysis
RAII, as its destructor might cause an invalidation which implies that it
might need to lock the helper threads to prevent races.  Leading to dead
locks, when the helper thread lock was already acquired by the same thread.

Comment 43

[Shu-yu Guo [:shu]]

Comment on attachment 8607115 [details] [diff] [review]
fixup part 5 - Move the AutoUnlockHelperThreadState sooner, including before the AutoEnterAnalysis scope.

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

The complication of locking code worries me. Your patch seems okay to me, but I think we can do this simpler and more obviously correct.

How about: revert to the old way of locking/AutoEnterAnalysis, and accumulate all the finished builders we need to fire onIonCompilation for, then fire all those hooks *after* we exit the scope for both AutoLockHelperThreadState and AutoEnterAnalysis?

So, something like:

{
  AutoLockHelperThreadState lock;
  AutoEnterAnalysis analysis(cx);
  BuildersToFireDebuggerHookFor list;

  while (true) {
    // finish builders and accumulate data into list
  }
}

for (i = 0; i < list.length; i++) {
  fireOnIonCompile(list[i]);
}

Comment 44

[Nicolas B. Pierron [:nbp]]

(In reply to PTO until 05/25 from comment #43)
> Comment on attachment 8607115 [details] [diff] [review]
> fixup part 5 - Move the AutoUnlockHelperThreadState sooner, including before
> the AutoEnterAnalysis scope.
> 
> Review of attachment 8607115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The complication of locking code worries me. Your patch seems okay to me,
> but I think we can do this simpler and more obviously correct.
> 
> How about: revert to the old way of locking/AutoEnterAnalysis, and
> accumulate all the finished builders we need to fire onIonCompilation for,
> then fire all those hooks *after* we exit the scope for both
> AutoLockHelperThreadState and AutoEnterAnalysis?
> 
> So, something like:
> 
> {
>   AutoLockHelperThreadState lock;
>   AutoEnterAnalysis analysis(cx);
>   BuildersToFireDebuggerHookFor list;
> 
>   while (true) {
>     // finish builders and accumulate data into list
>   }
> }
> 
> for (i = 0; i < list.length; i++) {
>   fireOnIonCompile(list[i]);
> }

I thought of that, the problem is that we would have to hold multiple vectors of JSScript, and the LifoAlloc used by IonBuilders.  Thus doing so implies that we have to run FinishOffThreadBuilder  after within this last for loop.

Comment 45

[Nicolas B. Pierron [:nbp]]

Attachment: fixup Bug 1147403 part 5 - Restore previous locking method, and factor code.

I left the previous fixup as a possibility, as I am not satisifed with this
approach.

This approach implies that we have to extract the scripts of the builder,
and the json spew, before we do the onIoncompilation calls.

We cannot safely generate the rooted list of script out-side the
AutoEnterAnalysis section, and we need to have leave this section to be able
to call back to the Debugger hooks.

Thus not calling straight after the codegen->link() call, implies that we
have to keep a vector of vector of rooted scripts, to give as argument of
the onIonCompilation hook.  This patch does so by creating one array, and by
making slices out of it.

Also, as we terminate the builders before the calls into the Debugger, we
have to create a new LifoAlloc to store the result of the compiled graphs.

The only potential thing which is interesting in this patch is the fact that
LinkCodeGen and LinkBackgroundCodeGen are extracted and shared.  Otherwise I
think the previous approach is better.

Comment 46

[Shu-yu Guo [:shu]]

Comment on attachment 8610151 [details] [diff] [review]
fixup Bug 1147403 part 5 - Restore previous locking method, and factor code.

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

Nice refactoring!

I hear your about the extra book-keeping, but I still feel it's more important to make the locking code more obviously correct. The debugger hooks' perf don't matter, so the extra work here isn't a big deal IMO.

Comment 47

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=334573583461
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3495190d92f7

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4182b3d593
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb79c2d279eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e87475c7ecc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e2d95665e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/898150157374
https://hg.mozilla.org/integration/mozilla-inbound/rev/960bbe18c960
https://hg.mozilla.org/integration/mozilla-inbound/rev/396f78269a37
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3ab5214edd

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/021356105e99

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f4182b3d593
https://hg.mozilla.org/mozilla-central/rev/bb79c2d279eb
https://hg.mozilla.org/mozilla-central/rev/e87475c7ecc2
https://hg.mozilla.org/mozilla-central/rev/19e2d95665e8
https://hg.mozilla.org/mozilla-central/rev/898150157374
https://hg.mozilla.org/mozilla-central/rev/960bbe18c960
https://hg.mozilla.org/mozilla-central/rev/396f78269a37
https://hg.mozilla.org/mozilla-central/rev/bb3ab5214edd
https://hg.mozilla.org/mozilla-central/rev/021356105e99