884473 - OdinMonkey: Integrate with perf performance tool on linux

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Using code from bug 873522 and bug 873128 (and other bugs related to vtune) should be enough for making a perf map file that could help analyzing perf reports of asm.js code.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1: function integration - WIP

Sean, I tried to reuse the more to avoid code copy, but I feel like these should be relatively independent too. Do you think I should factor more these codes?

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 764373 [details] [diff] [review]
Part 1: function integration - WIP

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

Keeping Perf and VTune code separate is fine. I don't think we should worry too much about the quality of debugging code -- if it gives the right information to the tool and isn't too egregious, it's good enough.

As a side comment, I'm not personally in favor of breaking up arguments to methods or constructors across multiple lines. Generally speaking, it reads better to have as few lines as possible, as long as those lines don't become too clever.

::: js/src/ion/AsmJSLink.cpp
@@ +13,5 @@
>  #include "AsmJSModule.h"
>  #include "frontend/BytecodeCompiler.h"
>  
> +#ifdef JS_ION_PERF
> +#include "PerfSpewer.h"

nit: should be "# include", with the space.

@@ +477,5 @@
> +SendFunctionsToPerf(JSContext *cx, AsmJSModule &module)
> +{
> +    if (!PerfFuncEnabled()) {
> +        return true;
> +    }

No need for {}

@@ +482,5 @@
> +
> +    PerfSpewer perfSpewer;
> +
> +    unsigned long base = (unsigned long) module.functionCode();
> +    const char *filename = module.profiledFileName();

Tracking the filename separately doesn't seem necessary, based on the VTune patch from Bug 873128:

If we have |const AsmJSModule::PostLinkFailureInfo &info = module.postLinkFailureInfo();|,
then the filename is |const AsmJSModule::PostLinkFailureInfo &info = module.postLinkFailureInfo();|.

@@ +500,5 @@
> +
> +        unsigned lineno = func.lineno;
> +        unsigned columnIndex = func.columnIndex;
> +
> +        perfSpewer.AsmJSWriteProfile(start, size, filename, lineno, columnIndex, method_name);

This function might read better as |writeAsmJSProfile()|.

@@ +528,5 @@
>          return false;
>  #endif
>  
> +#if defined(JS_ION_PERF)
> +    if (!SendFunctionsToPerf(cx, module))

If this is what you were talking about regarding sharing code, this separation is fine.

::: js/src/ion/PerfSpewer.cpp
@@ +132,5 @@
> +{
> +    if (!fp_)
> +        return;
> +
> +    if (PerfFuncEnabled()) {

if (!PerfFuncEnabled())
    return;

Comment 3

[Sean Stangl [:sstangl]]

Ahem, "then the filename is |const_cast<char *>(info.scriptSource_->filename())|".

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Sean Stangl [:sstangl] from comment #2)
> As a side comment, I'm not personally in favor of breaking up arguments to
> methods or constructors across multiple lines. Generally speaking, it reads
> better to have as few lines as possible, as long as those lines don't become
> too clever.

Fair enough. I preferred to keep a separation at least for the fprintf call, as it helps to figure out the number of arguments to the format string (missing one argument provokes a segfault).

> > +#ifdef JS_ION_PERF
> > +#include "PerfSpewer.h"
> 
> nit: should be "# include", with the space.
> 
> @@ +477,5 @@
> > +SendFunctionsToPerf(JSContext *cx, AsmJSModule &module)
> > +{
> > +    if (!PerfFuncEnabled()) {
> > +        return true;
> > +    }
> 
> No need for {}

Fixed all the nits.
> 
> @@ +482,5 @@
> > +
> > +    PerfSpewer perfSpewer;
> > +
> > +    unsigned long base = (unsigned long) module.functionCode();
> > +    const char *filename = module.profiledFileName();
> 
> Tracking the filename separately doesn't seem necessary, based on the VTune
> patch from Bug 873128:
> 
> If we have |const AsmJSModule::PostLinkFailureInfo &info =
> module.postLinkFailureInfo();|,
> then the filename is |const AsmJSModule::PostLinkFailureInfo &info =
> module.postLinkFailureInfo();|.
> 
Cool! That makes it even easier.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1: function integration

I'm now working on Block granularity integration.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - basic blocks integration

This patch provides a working basic blocks integration.

Due to the way the line numbers are found by using the next ParseNode, it happens sometimes that the next ParseNode is null (for instance, in a statements list) and therefore the basic block doesn't get line / column informations.

Also, the AsmJSPerfSpewer functions are not defined unless the JS_ION_PERF is on. When compiling without the --enable-perf flag, the compiler infers that the functions are not actually called (because of the PerfSpewer.h preprocessing prologue), so there is no compilation error. I tried with g++ and clang and didn't run into any particular issue. However, I will launch a try build, to be sure that it compiles everywhere.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

All try builds succeeded:
- without JS_ION_PERF: https://tbpl.mozilla.org/?tree=Try&rev=a3307b428f3f
- with JS_ION_PERF: https://tbpl.mozilla.org/?tree=Try&rev=048eb6be5371

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: As discussed with sstangl, put a warning message when no env variable is given

Comment 9

[Sean Stangl [:sstangl]]

Comment on attachment 765467 [details] [diff] [review]
Part 1: function integration

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

Looks good :)

::: js/src/ion/AsmJSModule.h
@@ +297,5 @@
> +            startCodeOffset(start),
> +            endCodeOffset(end),
> +            lineno(line),
> +            columnIndex(column)
> +        {}

nit: should be "{ }" to match Ion style

@@ +509,5 @@
>          return profiledFunctions_[i];
>      }
>  #endif
> +#ifdef JS_ION_PERF
> +    bool trackPerfProfiledFunction(JSAtom *name, unsigned startCodeOffset, unsigned endCodeOffset, unsigned line, unsigned column) {

nit: 100 column limit

::: js/src/ion/PerfSpewer.cpp
@@ +127,5 @@
>      return true;
>  }
>  
>  void
> +PerfSpewer::writeAsmJSProfile(unsigned long base, unsigned long size, const char *filename, unsigned lineno, unsigned colIndex, const char *funcName)

nit: This line looks like it exceeds the 100-column limit.

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 766872 [details] [diff] [review]
Part 2 - basic blocks integration

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

::: js/src/ion/AsmJS.cpp
@@ +2350,5 @@
> +            blk->setColumnIndex(column);
> +        }
> +#else
> +        (void) pn;  // avoid unused warning
> +        (void) blk;

Do these actually cause unused warnings? Normally, unused errors aren't reported for function arguments, at least with GCC.

@@ +2364,3 @@
>          mirGraph().addBlock(*block);
>          (*block)->setLoopDepth(loopDepth);
> +

nit: no need for newline

@@ +4379,5 @@
> +
> +    ParseNode *elseBlockStmt = NULL;
> +    // The second block given to branchAndStartThen contains either the else statement if
> +    // there is one, or the join block; so we need to give the next statement accordingly.
> +    if (!(elseBlockStmt = elseStmt))

I'd strongly prefer that we avoid using assignation during comparison:
if (elseStmt != NULL) elseBlockStmt = elseStmt else elseBlockStmt = nextStmt;

::: js/src/ion/CodeGenerator.cpp
@@ +2486,5 @@
> +    else
> +        perfSpewer = &perfSpewer_;
> +#else
> +    perfSpewer = &perfSpewer_;
> +#endif

This could just be:

#ifdef JS_ION_PERF
PerfSpewer *perfSpewer = &perfSpewer_;
if (gen->compilingAsmJS())
    perfSpewer = &gen->perfSpewer();
#endif

and then wrap the checks to PerfFooEnabled() with #ifdef JS_ION_PERF.

Comment 11

[Sean Stangl [:sstangl]]

Comment on attachment 767537 [details] [diff] [review]
As discussed with sstangl, put a warning message when no env variable is given

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

Thanks!

::: js/src/ion/PerfSpewer.cpp
@@ +35,5 @@
>          const char *env = getenv("IONPERF");
>          if (env == NULL) {
>              PerfMode = PERF_MODE_NONE;
> +            fprintf(stderr, "Warning: you haven't defined the IONPERF env variable.");
> +            fprintf("Perf mapping functions will be deactivated.\n");

How about "Warning: JIT perf reporting requires IONPERF set to \"block\" or \"func\"."?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 1: function integration - fixed nits

Thanks sstangl for the review!
Fixed the nits, carrying over r+.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - basic blocks integration - fixed nits + bug

Thanks for the review! Indeed, no need to cancel the unused parameters apparently...
I fixed more nits in PerfSpewer.h about the max line columns count policy.

Also found a bug in PerfSpewer.cpp: endBasicBlock doesn't make any check on the existing file while startBasicBlock; as a result, endBasicBlock can try to bind a label of a record which is not present in the table (for the first block).

Finally, I also removed the !PerfBlockEnabled() in CodeGenerator.cpp as these are redundant with the one already present in {start,end}BasicBlock functions in PerfSpewer.cpp. If you have any comment about that, let me know.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 3: warning message

I changed the message accordingly, which indeed avoids the user to try random options that wouldn't work and display the help message.

Carrying over r+.

Comment 15

[Sean Stangl [:sstangl]]

Comment on attachment 768469 [details] [diff] [review]
Part 2 - basic blocks integration - fixed nits + bug

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

Both of those changes look good.

::: js/src/ion/AsmJS.cpp
@@ +3865,5 @@
>      if (!CheckExpr(f, elseExpr, Use::Normal, &elseDef, &elseType))
>          return false;
>  
>      f.pushPhiInput(elseDef);
> +    if (!f.joinIfElse(thenBlocks, elseExpr)) // next statement is actually not the else expr, but this is the closest stmt to the next one that is directly reachable

nit: column limit

::: js/src/ion/AsmJSLink.cpp
@@ +512,5 @@
> +
> +static bool
> +SendBlocksToPerf(JSContext *cx, AsmJSModule &module)
> +{
> +    if(!PerfBlockEnabled())

nit: "if ("

::: js/src/ion/PerfSpewer.cpp
@@ +287,5 @@
> +                filename,
> +                funcName);
> +    }
> +}
> +# endif // defined (JS_ION_PERF)

nit: no space after #

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks Sean for the review!
Rebased + fixed last nits.

For the record, this could be enabled with --enable-perf on ./configure.
Then when launching the shell, setting the env variable IONPERF={func,block,none} will enable one of the possible perf mapping modes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4918c20e91c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e2e4816d800
https://hg.mozilla.org/integration/mozilla-inbound/rev/b33cdd107405

Comment 17

[Douglas Crosher [:dougc]]

Have been giving this a try and it has not been working out well.  The map files are corrupt on the ARM and the process crashes.  The x64 and x86 have issues too.  Noticed that the process runs out of open file descriptors.  This does not appear to be a new problem caused by these patches, and they are an improvement, so perhaps they should still be landed and the approached reworking in a new bug.

An alternative approach being explored is to open the files for a much limited scope, just while appending to the file.  If thread safety is an issue then this should make it more practical to use a lock to synchronise access to the file.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Douglas Crosher [:dougc] from comment #17)
> Have been giving this a try and it has not been working out well. The map
> files are corrupt on the ARM and the process crashes.

Unfortunately, I didn't have a chance to test on ARM. If there any modifications you can do to make it work, that would be awesome!

> The x64 and x86 have
> issues too.  Noticed that the process runs out of open file descriptors. 
> This does not appear to be a new problem caused by these patches, and they
> are an improvement, so perhaps they should still be landed and the
> approached reworking in a new bug.
> 
> An alternative approach being explored is to open the files for a much
> limited scope, just while appending to the file.  If thread safety is an
> issue then this should make it more practical to use a lock to synchronise
> access to the file.

I tried it several times locally and never encountered any issues on x64, but I haven't tested with parallel compilation. I think :sstangl has also tested it locally on his machine. Moreover, there has also been a try build with the flag defined by hand, and all tests passed.

Could you please precise which options you have used, on which platform?
If issues show up with a particular script, could you also join it?

Comment 19

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/4918c20e91c9
https://hg.mozilla.org/mozilla-central/rev/7e2e4816d800
https://hg.mozilla.org/mozilla-central/rev/b33cdd107405

Comment 20

[Douglas Crosher [:dougc]]

Might have narrowed down the function name corruption issue seen on the ARM (B2G).  If the name is copied into a malloc'ed allocation when it is gathered, and if this alone is stored in the module, then the corruption and crash do not occur.

Could really use some help from someone with a command of the allocation strategies.  The function name is a JSAtom and collected in trackPerfProfiledFunction and stored in an AsmJSModule and it is later read and used in LinkAsmJS.

There is a note on AsmJSModule 'this means that AsmJSModule must be GC-safe.', but not sure what this means?

Is the name allocated on a GCed heap, or explicitly allocated and freed?  If it's on a GC'ed heap then should it be reachable in the AsmJSModule?  If it's being explicitly freed before it is being used then is the solution to make a copy?

Are the memory management patterns used in the code documented, apart from in the source?

Comment 21

[Luke Wagner [:luke]]

(In reply to Douglas Crosher [:dougc] from comment #20)
Sorry, I missed your question.

Yes, JSAtoms are allocated on the GC heap.  That means they need to be marked during GC to keep them alive and indeed ProfiledFunction::name is not!  This is pretty easy to do though: there is already an AsmJSModule::trace function that marks the other atoms stored in the AsmJSModule.  I'll file a new bug and cc you.