IonMonkey: Integrate with perf performance tool on linux

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

unspecified
mozilla24
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Posted patch Perf integration (obsolete) — Splinter Review
Attachment #751086 - Flags: review?(dvander)
Assignee: general → nmatsakis
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 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
Attachment #751086 - Flags: review?(dvander) → review+
> 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.
Summary: Integrate ion with perf performance tool on linux → IonMonkey: Integrate with perf performance tool on linux
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 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".
Posted patch Perf integration (obsolete) — Splinter Review
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
Attachment #751086 - Attachment is obsolete: true
Attachment #752342 - Flags: review+
Posted patch Perf integration (obsolete) — Splinter Review
Carrying over r+ from dvander.

Fix output to use npb's format when using function-level profiling.
Attachment #752342 - Attachment is obsolete: true
Attachment #752345 - Flags: review+
Carrying over r+ from dvander.

Remove stray benchmarking files that snuck into that patch.
Attachment #752345 - Attachment is obsolete: true
Attachment #752348 - Flags: review+
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?
https://hg.mozilla.org/mozilla-central/rev/8ee743b1a83e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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?)
Flags: needinfo?(nmatsakis)
Depends on: 876366
(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.
Flags: needinfo?(nmatsakis)
dholbert -- sorry, I've been meaning to write that same thing for a while now and I keep forgetting!
You need to log in before you can comment on or make changes to this bug.