495995 - Get VTune sampling profiler integration working again

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tools, defect, P5)

Description

[Edwin Smith]

it doesn't compile anymore, and VTune 9.1 has shipped with the api built in, and updated documentation.

Comment 1

[Edwin Smith]

Attachment: s/VTUNE/VMCFG_VTUNE, make it work with vtune sample profiling

Known limitations

* CodeAlloc tweaked to allocate 4MB at a time, in the hopes that
all methods end up contiguous.

* each method with >1 code block is not given to vtune -- time in such methods will be in VTune's Other32 module

* CallGraph support present but not tested.
* x64 support present but not tested.  (requires different VTune header/lib)
* tested with VTune 9.1_002

Comment 2

[Edwin Smith]

Attachment: makeover, simplified code, build final line table as code is jitted.

new patch with simplified makeover to the code.  Moved dependencies almost completely out of CodegenLIR, fixed bugs in line numbering.

Comment 3

[Edwin Smith]

Attachment: final round of x86 vtune tweaks

Comment 4

[Rick Reitmaier]

Comment on attachment 382563 [details] [diff] [review]
final round of x86 vtune tweaks

The original design clearly separated the capturing of the data from its use.   This way we'd have to change very little code in order to hook into another profiler or to dump the file/line/address info.  

This implementation is tightly coupled with vtune, which is not necessarily a bad thing, but we do loose out on the generality.  

If we end up writing more code to support different formats (e.g map/pdb file output) I'd hope we can avoid duplication.

'+'ing because it restores vtune support.

Comment 5

[Edwin Smith]

If we ever add another profiler, I'll factor the api so that vtune is an implementation.  My goal was to eliminate as much as possible from non-profiler code -- assembler just feeds the raw "facts" (method names, start/end, line/file, etc) to the profiler, as it learns them.

a non-vtune api would end up looking very similar to the current vtune api, just substituting vtuneXXX with profilerXXX or something.

Comment 6

[Edwin Smith]

come to think of it the Shark support will be a good test case to drive a clean factoring.  anyway...

Comment 7

[Steven Johnson]

Apparently this patch landed, but since then the code has broken and won't compile with VTUNE defined:

-- several references to gc allocation in CodegenLIR
-- cgen->jitCodePosUpdate((uintptr_t)list->code) where "list" no longer exists
-- VTune_RegisterMethod is called but no longer exists anywhere

possibly others, that's as far as I got

Comment 8

[Steven Johnson]

Actually, looking at the diffs, it appears the patch has *not* landed -- but it no longer applies cleanly. I'll see if I can mangle it into applying...

Comment 9

[Steven Johnson]

Attachment: Updated patch

Updated version of 382563 that applies to current tip -- please examine carefully, it seems to work, but it's not code I know well

Comment 10

[Edwin Smith]

in CodegenLIR.cpp:
#include "../core/vm_fops.h"

should be jit-calls.h  (file was renamed).  but this file isn't in any project files afaik, so not much pain.

Comment 11

[Edwin Smith]

other changes in CodegenLIR.cpp look like they got clobbered when merging too (moving code around in ::prologue(), at least)

Comment 12

[Steven Johnson]

(In reply to comment #11)
> other changes in CodegenLIR.cpp look like they got clobbered when merging too
> (moving code around in ::prologue(), at least)

quite possible -- this was a quick hack-n-slash to get things working again.

Any particular reason we shouldn't land this (assuming it's cleaned up)?

Comment 13

[Edwin Smith]

none that I can think of

Comment 14

[Rick Reitmaier]

Comment on attachment 401982 [details] [diff] [review]
Updated patch

-'ing since it seems like a lot of unrelated changes are in the patch.  

Are they really needed?
E.g.  newarray and astype, LIR_live, Fragment.OnDestroy etc.

Comment 15

[Steven Johnson]

Not intentional, perhaps I included more than I meant to.

Comment 16

[Werner Sharp]

Attachment: updated patch for latest code

Comment 18

[Werner Sharp]

Last patch does not include vcproj changes from VTUNE to AVMFEATURE_VTUNE because those scrambled my vcproj files and show hundreds of lines of differences.

Comment 19

[Steven Johnson]

Comment on attachment 457319 [details] [diff] [review]
updated patch for latest code

Spacing is all wonky -- you need to convert all tabs to spaces before you submit.

Comment 20

[Edwin Smith]

Comment on attachment 457319 [details] [diff] [review]
updated patch for latest code

Nothing obviously wrong, except indenting.

The nanojit patches will need to be split into a separate patch to submit to nanojit-central.

How much does vtune.cpp depend on Tamarin?  We have patches in progress for shark, oprofile, and valgrind as well, and I could imagine things in nanojit starting to get cluttered.  Its also clumsy to have nanojit depend on the VM for profiling hooks; if all the VM needs to provide is function name and file/line info, could we factor vtune support to be mostly in nanojit?  

Thoughts?

Comment 21

[Rick Reitmaier]

Seems like a good idea.  Maybe a registration API containing a list of callbacks?

Then for each line/file LIR op along with begin and end assembly we'd issue the calls if they exist.   Another nice side-effect of this approach is that it could be left in release builds, since the overhead would be rather small if no callbacks are registered (i.e. access and zero compare; and even smaller if no line/file ops are encountered).

We also might need/want a callback for labels and possibly 'trace exit' ops (need to check with TM folks).

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 457319 [details] [diff] [review]
updated patch for latest code

Seems ok to me.  I'd really appreciate some comments in LIRopcode.tbl on LIR_file/LIR_line explaining how they're used, and what their types are.  They also need to be type-checked in ValidateWriter::ins1(), currently they are ignored.

Comment 23

[Steven Johnson]

Might be nice to have lirasm insert LIR_file/LIR_line in --random mode, too. (Fine for a followup bug.)

Comment 24

[Werner Sharp]

Attachment: latest patch...fixed whitespace, added vcproj, fixed one comment

Minor changes from last one.

Comment 25

[Werner Sharp]

Nanojit central part: http://hg.mozilla.org/projects/nanojit-central/rev/602fdcb72884

Comment 26

[Werner Sharp]

TR changes pushed:

http://hg.mozilla.org/tamarin-redux/rev/fdfcc69a0300

Comment 27

[Edwin Smith]

nanot changes imported to tr:
http://hg.mozilla.org/tamarin-redux/rev/235d57a9648b

Comment 28

[Robert Sayre]

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