Closed
Bug 873522
Opened 12 years ago
Closed 12 years ago
IonMonkey: Integrate with perf performance tool on linux
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: nmatsakis, Assigned: nmatsakis)
References
Details
Attachments
(1 file, 3 obsolete files)
11.62 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #751086 -
Flags: review?(dvander)
Assignee | ||
Updated•12 years ago
|
Assignee: general → nmatsakis
Comment 2•12 years ago
|
||
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+
Assignee | ||
Comment 4•12 years ago
|
||
> 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.
Updated•12 years ago
|
Summary: Integrate ion with perf performance tool on linux → IonMonkey: Integrate with perf performance tool on linux
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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".
Assignee | ||
Comment 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
Carrying over r+ from dvander.
Remove stray benchmarking files that snuck into that patch.
Attachment #752345 -
Attachment is obsolete: true
Attachment #752348 -
Flags: review+
Assignee | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 13•12 years ago
|
||
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)
Comment 14•12 years ago
|
||
(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)
Assignee | ||
Comment 15•12 years ago
|
||
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.
Description
•