873522 - IonMonkey: Integrate with perf performance tool on linux

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

This is a (somewhat rough) patch to integrate ion compilation with the perf tool on linux. 

If you pass IONFLAGS=perf-func, it will emit files into /tmp that perf uses to map anonymous memory so that you can see which JavaScript functions are taking up time. Each function is identified by its function number (corresponding with iongraph) and the filename and linenumber of the JSScript*.

If you pass IONFLAGS=perf-block, it will allocate at a basic block granularity. Each block is identified by its function number (corresponding with iongraph), basic block #, and also filename and line number.

Limitations:
- OOL code is not fully accounted for
- If either flag is set, we *never free any generated code*. This is because I do not want to reuse addresses as that could confuse perf's after-the-fact accounting.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Perf integration

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 751086 [details] [diff] [review]
Perf integration

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

::: js/src/ion/CodeGenerator.cpp
@@ +2367,5 @@
>                  return false;
>          }
>  
> +        if (IonSpewEnabled(IonSpew_PerfBlock))
> +            perfSpewer_.startBasicBlock(current->mir(), masm);

I am sad hold this patch on a sanity aspect, but I mention the same thing to Brian before for the PCCount integration.

The goal of generateBody should remain generating the stream of instruction of the function, not having to deal with the instrumentation which comes aside to it.

I agree that we need to at-least monitor once at each location, but this sould some-how be fold into a data structure named CodeGenerator::Instrumentation, which dispatch the work to perf / pccount / whatever is enabled at the time.

::: js/src/ion/Ion.cpp
@@ +496,5 @@
>  
>      // Code buffers are stored inside JSC pools.
>      // Pools are refcounted. Releasing the pool may free it.
> +    if (IonPerfEnabled())
> +        return; // horrible hack: don't want to reuse code addresses

If you want to do that, then perf should be a shell preference instead of an environment variable.
Otherwise this might be a hardly detectable source of memory leak which can be done by users.

::: js/src/ion/PerfSpewer.cpp
@@ +103,5 @@
> +
> +            JS_ASSERT(cur <= blockStart);
> +            if (cur < blockStart) {
> +                fprintf(fp_,
> +                        "%lx %lx Func%2d-Block?(%s:%d)\n",

nit: "%s:%d: …"

This is used by many tools for tracking locations and finding the corresponding files.
Do the same thing for the 3 other fprintf.

Comment 3

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

Comment on attachment 751086 [details] [diff] [review]
Perf integration

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

Cool!

::: js/src/ion/CodeGenerator.cpp
@@ +2390,5 @@
>          if (masm.oom())
>              return false;
> +
> +        if (IonSpewEnabled(IonSpew_PerfBlock))
> +            perfSpewer_.endBasicBlock(masm);

Since this is specific to codegen, and seems to cover the whole body (not just a block), can you name it something like "startGeneratingCode" / "finishGeneratingCode"?

Anyway, instrumenting here seems fine.

::: js/src/ion/Ion.cpp
@@ +495,5 @@
>      JS_POISON(code_, JS_FREE_PATTERN, bufferSize_);
>  
>      // Code buffers are stored inside JSC pools.
>      // Pools are refcounted. Releasing the pool may free it.
> +    if (IonPerfEnabled())

We ran into the same problem with VTune :( Could you whitespace and comment this just a bit so it's more obvious what is going on? Like:

    // NOTE: Some performance tools are not capable of deregistering code addresses.
    // As a hack, we just leak code addresses if one of these tools is in use.
    if (IonPerfEnabled())
        return;

    // Code buffers are stored inside JSC pools.
    // Pools are refcounted. Releasing the pool may free it.

    ....

::: js/src/ion/IonSpewer.h
@@ +137,5 @@
>  void EnableChannel(IonSpewChannel channel);
>  void DisableChannel(IonSpewChannel channel);
>  void EnableIonDebugLogging();
>  
> +static inline bool IonPerfEnabled() {

nit: { on newline

@@ +178,5 @@
>  { }
>  static inline void EnableIonDebugLogging()
>  { }
>  
> +static inline bool IonPerfEnabled() {

nit: { on newline

Comment 4

[Niko Matsakis [:nmatsakis]]

> Since this is specific to codegen, and seems to cover the whole body (not just a block), can > you name it something like "startGeneratingCode" / "finishGeneratingCode"?

That particular function is specific to a single block. The idea is to generate a label for recording the start and end of each basic block. There is also a mode that generates one entry per function, because sometimes getting data at the level of a basic block is *too* detailed.

Comment 5

[Sean Stangl [:sstangl]]

This patch looks like it only provides for creation of perf mapping when DEBUG is defined, but defining DEBUG changes the performance characteristics of both the generated code and the compiler.

It would be better to have a way to toggle map generation independently of DEBUG. For VTune, for example, we have the configuration-time toggle --enable-vtune, which defines MOZ_VTUNE.

Comment 6

[Sean Stangl [:sstangl]]

Comment on attachment 751086 [details] [diff] [review]
Perf integration

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

::: js/src/ion/IonSpewer.cpp
@@ +245,5 @@
>              "  pools      Literal Pools (ARM only for now)\n"
>              "  cacheflush Instruction Cache flushes (ARM only for now)\n"
>              "  logs       C1 and JSON visualization logging\n"
>              "  trace      Generate calls to js::ion::Trace() for effectful instructions\n"
> +            "  perf       Generate perf maps (*also leaks all generated code*)\n"

Note that this line is incorrect: the options are now "perf-func" and "perf-block".

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Perf integration

Carrying over r+ from dvander.

Revised version of the patch:
- adopts nbp's suggested output format
- moves to a configure option (--enable-perf) and IONPERF=func, IONPERF=block
- a few other misc bug fixes I encountered in practice

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Perf integration

Carrying over r+ from dvander.

Fix output to use npb's format when using function-level profiling.

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: Perf integration

Carrying over r+ from dvander.

Remove stray benchmarking files that snuck into that patch.

Comment 10

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee743b1a83e

Try run: https://tbpl.mozilla.org/?tree=Try&rev=85869e841076

Comment 11

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Two "unused-variable" build warnings introduced by this checkin:
{
js/src/ion/PerfSpewer.cpp:27:13: warning: 'PerfChecked' defined but not used [-Wunused-variable]

js/src/ion/PerfSpewer.cpp:28:17: warning: 'PerfMode' defined but not used [-Wunused-variable]
}

Are those vars going to be used in a later patch?

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8ee743b1a83e

Comment 13

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Needinfo for comment 11.

(Niko, if those variables aren't going to be used, maybe you could just push a followup patch to drop them?)

Comment 14

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Two "unused-variable" build warnings introduced by this checkin:
> {
> js/src/ion/PerfSpewer.cpp:27:13: warning: 'PerfChecked' defined but not used
> [-Wunused-variable]
> 
> js/src/ion/PerfSpewer.cpp:28:17: warning: 'PerfMode' defined but not used
> [-Wunused-variable]
> }
> 
> Are those vars going to be used in a later patch?

Looks like this was addressed in bug 877706 (which moved one of the variables into an #ifdef) and bug 883106 (which added usages of the other one).  Canceling needinfo.

Comment 15

[Niko Matsakis [:nmatsakis]]

dholbert -- sorry, I've been meaning to write that same thing for a while now and I keep forgetting!