555345 - Add profiling capability for JIT-generated code

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tools, enhancement, P3)

Description

[William Maddox]

Attachment: Profiling support for JIT-generated code

This patch extends the vprof facility to support event counting and value profiling in generated code.  Aggregate statistics are charged to the location in the JIT that gave rise to the code.

The macros JIT_EVENT(event_name) and JIT_VALUE(value_name, lir_for_value) generate a call to the profiler, and may be placed anywhere where it would be appropriate to invoke 'callIns()' to generate a call to a void function.

Comment 1

[William Maddox]

Attachment: Profiling support for JIT-generated code

Previously attached an older version of the patch -- here's the right one.

Comment 2

[Edwin Smith]

Comment on attachment 435317 [details] [diff] [review]
 Profiling support for JIT-generated code 

Question: how important is it for these counters to be fast?  if we're just gathering behavior stats, speed isn't critical and the callouts seem fine.

marking R- until issues below are addressed, but overall I think these are minor and the probe facility will be quite useful.

From the looks of it, initJitProfileValue() and jitProfileValue() are not really JIT specific, except that they split the init logic from the value-update logic.  since these live in vprof.cpp, can we give them more accurate names and remove JIT from the descriptions, or clarify expected JIT usage in one comment?

if the code in initJitProfileValue is simply duplicated from profileValue(), can we refactor?

Looks like un-intended changes to _nvprof() and _hprof(), re-adding support for varadic macros.

nit: I would put the convenience wrappers in jit-calls.h just before the FUNCTION() definitions, since they're just one liners and possibly subject to change if we change the JIT_EVENT macros in CodegenLIR.cpp.

Comment 3

[William Maddox]

(In reply to comment #2)
> (From update of attachment 435317 [details] [diff] [review])
> Question: how important is it for these counters to be fast?  if we're just
> gathering behavior stats, speed isn't critical and the callouts seem fine.

The speed was adequate for my purposes in gathering data on the execution frequency of the optimized paths in my JIT arithmetic work.  The intent is to extend the existing vprof machinery in the most natural way.  I considered adding inline counter code, which would have been sufficient for the purpose at hand, but would have introduced another profiling infrastructure that was less powerful and extensible, e.g., value profiling would be significant additional effort and would not "fall out" as it does here.  Additionally, there would be another format for profile dump info, and probably another switch or build option to control it.

> From the looks of it, initJitProfileValue() and jitProfileValue() are not
> really JIT specific, except that they split the init logic from the
> value-update logic.  since these live in vprof.cpp, can we give them more
> accurate names and remove JIT from the descriptions, or clarify expected JIT
> usage in one comment?

It is not obvious that the separation is useful elsewhere, hence I didn't try to generalize it.  I'll revisit the issue.

> Looks like un-intended changes to _nvprof() and _hprof(), re-adding support
> for varadic macros.

This was actually a deliberate change to align the interfaces for all of the the profiling variants.  Is there any reason why _nvprof avoided variadic macros while _hprof did not?
 
> nit: I would put the convenience wrappers in jit-calls.h just before the
> FUNCTION() definitions, since they're just one liners and possibly subject to
> change if we change the JIT_EVENT macros in CodegenLIR.cpp.

An earlier version was actually written this way, but I changed it, because I thought it was sloppy to put non-inline, non-template functions in .h files, but indeed the compiler/linker will do the right thing and it does move code that is more tightly bound to the JIT than to profiling infrastructure into proximity with the code that depends on it.

Comment 4

[Edwin Smith]

(In reply to comment #3)
> > Looks like un-intended changes to _nvprof() and _hprof(), re-adding support
> > for varadic macros.
> 
> This was actually a deliberate change to align the interfaces for all of the
> the profiling variants.  Is there any reason why _nvprof avoided variadic
> macros while _hprof did not?

Probably because _nvprof() was encountered on a broken compiler, and _hprof() wasn't.  The fix to align api's would be to strip the varadic-ness from all of them.  Seems orthagonal to this patch.

> > nit: I would put the convenience wrappers in jit-calls.h just before the
> > FUNCTION() definitions, since they're just one liners and possibly subject to
> > change if we change the JIT_EVENT macros in CodegenLIR.cpp.
> 
> An earlier version was actually written this way, but I changed it, because I
> thought it was sloppy to put non-inline, non-template functions in .h files,
> but indeed the compiler/linker will do the right thing and it does move code
> that is more tightly bound to the JIT than to profiling infrastructure into
> proximity with the code that depends on it.

jit-calls.h is a special case, it has lots of code you'd normally expect to see in .cpp files.  It is (and will be) only included by CodegenLIR.cpp.   The separation is partly a historical accident and partly an attempt to compartmentalize helper functions that are dedicated to jit-use only.

Comment 5

[William Maddox]

Attachment: Add support for JIT profiling and clean up ugliness in vprof

> From the looks of it, initJitProfileValue() and jitProfileValue() are not
> really JIT specific, except that they split the init logic from the
> value-update logic.  since these live in vprof.cpp, can we give them more
> accurate names and remove JIT from the descriptions, or clarify expected JIT
> usage in one comment?

The code duplication was largely out of the desire to add the needed functionality without getting too cozy with some code I thought was rather clumsy.  Here, in this patch, I've bitten the bullet and separated creation of a profile record from the recording of the data across the board and cleaned up a bunch of other stuff that was sticking in my craw.  In particular, the time profiling support has been rewritten so that it no longer needs to work around the old initialization semantics nor does it need to pass information via a static variable.  I updated the uses to conform to the API changes.

Comment 6

[Edwin Smith]

Comment on attachment 440420 [details] [diff] [review]
Add support for JIT profiling and clean up ugliness in vprof

nit: tabs in jit-calls.h  (to fix:  utils/fixtabs core/jit-calls.h).  Local convention in tamarin/nanojit is no spaces between function name and (), for example: void jitProfileEvent (void* id).  (I realize vprof.* isn't formatted well, we should fix vprof at some point, maybe after this goes in)

The original author (Moh at intel) said the reason the macros used conditional expressions instead of if/else, was to allow _vprof() and _nvprof() to be used in an expression context in C++ code. however, I see that the old code in those macros was wrapped in { }, so not sure how it could have worked.  Plus, I don't think we've ever used nvprof/hprof/vprof in an expression context, so its not very important that we preserve that ability.  (you can copy this into a comment for posterity).

the code looks fine to the naked eye, assuming it runs fine, this should be good to go.

Comment 7

[William Maddox]

Attachment: Revised patch with minor whitespace adjustments and commentary

Revised per Edwin's comments.

Comment 8

[Lars T Hansen]

Comment on attachment 440420 [details] [diff] [review]
Add support for JIT profiling and clean up ugliness in vprof

Re variadic macros, they don't work with our current Symbian tool chain, and that's just a fact of life - we should do without them when we can, if necessary by having macros M1(x), M2(x,y), and so on - ugly but unavoidable at present.

Comment 9

[William Maddox]

Pushed to tamarin-redux:

http://hg.mozilla.org/tamarin-redux/rev/793005655b8a

Comment 10

[William Maddox]

Attachment: Add support for type tag histograms in JIT-generated code

This patch extends the vprof api with _jhprof_init and _jhprof macros allowing a histogram profile record to be constructed separately from the recording of values, as we provided previously for value profiling with _jvprof_init, _jvprof, etc.  These are used to define a JIT_TAGVAL macro in CodeGenLIR.cpp for profiling atom tag values.

Comment 11

[Edwin Smith]

Comment on attachment 447691 [details] [diff] [review]
Add support for type tag histograms in JIT-generated code

varadic macros are known to not work on symbian, so as long as this compiles safely on symbian (with probes disabled) it looks fine.

Comment 12

[William Maddox]

Attachment: Subset of original patch that should have been pushed to nanojit-central

The original JIT profiling patch was a coordinated set of changes to both the vprof infrastructure (vprof/vprof.h, vprof/vprof.cpp) and CodegenLIR.  The vprof changes should have been pushed separately to nanojit-central, and were apparently clobbered during an import from nj to tr.  This patch to nj only should restore those changes.

Comment 13

[Rick Reitmaier]

Comment on attachment 449092 [details] [diff] [review]
Subset of original patch that should have been pushed to nanojit-central

rubber stamp since its been r+ previously.

Comment 14

[William Maddox]

Attachment: Subset of type tag histogram patch that must land first in nanojit-central

Since vprof is imported from nanojit-central, a subset of the type tag histogram patch must land there first.  This should have been two separate patches, but I did not realize that vprof as well as nanojit were imported, and the oversight slipped through the original review.

Comment 15

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/8ab576d1484c

Comment 16

[William Maddox]

Pushed to nanojit-central:

http://hg.mozilla.org/projects/nanojit-central/rev/10b865fa5d41

Comment 17

[William Maddox]

Pushed to nanojit-central:

http://hg.mozilla.org/projects/nanojit-central/rev/939ade8f3ee9

Comment 18

[William Maddox]

Pused to TR:

http://hg.mozilla.org/tamarin-redux/rev/54d9cd1eb2c3

Comment 19

[Edwin Smith]

imported nanojit-central to TR:
http://hg.mozilla.org/tamarin-redux/rev/df50abdabb92
http://hg.mozilla.org/tamarin-redux/rev/288279bd3166

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cc6944a634c3