Get VTune sampling profiler integration working again

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Tools
P5
normal
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Edwin Smith, Assigned: Werner Sharp)

Tracking

(Blocks: 2 bugs)

unspecified
Q3 11 - Serrano
x86
Windows Vista
Dependency tree / graph

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tamarin)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

9 years ago
it doesn't compile anymore, and VTune 9.1 has shipped with the api built in, and updated documentation.
(Reporter)

Comment 1

9 years ago
Created attachment 381136 [details] [diff] [review]
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
Assignee: nobody → edwsmith
Attachment #381136 - Flags: review?(rreitmai)

Updated

9 years ago
Attachment #381136 - Flags: review?(rreitmai) → review+
(Reporter)

Updated

9 years ago
Summary: Get VTune integration working again → Get VTune sampling profiler integration working again
(Reporter)

Updated

9 years ago
Blocks: 496339
(Reporter)

Comment 2

9 years ago
Created attachment 382359 [details] [diff] [review]
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.
Attachment #381136 - Attachment is obsolete: true
(Reporter)

Comment 3

9 years ago
Created attachment 382563 [details] [diff] [review]
final round of x86 vtune tweaks
Attachment #382359 - Attachment is obsolete: true
Attachment #382563 - Flags: review?(rreitmai)

Comment 4

9 years ago
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.
Attachment #382563 - Flags: review?(rreitmai) → review+
(Reporter)

Comment 5

9 years ago
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.
(Reporter)

Comment 6

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

Comment 7

8 years ago
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

8 years ago
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

8 years ago
Created attachment 401982 [details] [diff] [review]
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
Attachment #382563 - Attachment is obsolete: true
Attachment #401982 - Flags: review?(rreitmai)
(Reporter)

Comment 10

8 years ago
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.
(Reporter)

Comment 11

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

Comment 12

8 years ago
(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)?
(Reporter)

Comment 13

8 years ago
none that I can think of

Comment 14

8 years ago
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.
Attachment #401982 - Flags: review?(rreitmai) → review-

Comment 15

8 years ago
Not intentional, perhaps I included more than I meant to.

Updated

8 years ago
Priority: -- → P5
Target Milestone: --- → flash10.1

Updated

8 years ago
Target Milestone: flash10.1 → flash10.2
(Reporter)

Updated

8 years ago
Assignee: edwsmith → nobody
(Assignee)

Comment 16

8 years ago
Created attachment 457319 [details] [diff] [review]
updated patch for latest code
Assignee: nobody → wsharp
Attachment #401982 - Attachment is obsolete: true
Attachment #457319 - Flags: superreview?(edwsmith)
Attachment #457319 - Flags: review?(stejohns)
(Assignee)

Updated

8 years ago
Duplicate of this bug: 547078
(Assignee)

Comment 18

8 years ago
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

8 years ago
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.
Attachment #457319 - Flags: review?(stejohns) → review+
(Reporter)

Comment 20

8 years ago
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?
Attachment #457319 - Flags: superreview?(edwsmith) → superreview+
(Reporter)

Updated

8 years ago
Attachment #457319 - Flags: feedback?(nnethercote)

Comment 21

8 years ago
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 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.
Attachment #457319 - Flags: feedback?(nnethercote) → feedback+

Comment 23

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

Comment 24

8 years ago
Created attachment 459455 [details] [diff] [review]
latest patch...fixed whitespace, added vcproj, fixed one comment

Minor changes from last one.
Attachment #457319 - Attachment is obsolete: true
Attachment #459455 - Flags: review?(stejohns)

Updated

8 years ago
Attachment #459455 - Attachment is patch: true

Updated

8 years ago
Attachment #459455 - Flags: review?(stejohns) → review+
(Assignee)

Comment 25

8 years ago
Nanojit central part: http://hg.mozilla.org/projects/nanojit-central/rev/602fdcb72884
(Assignee)

Comment 26

8 years ago
TR changes pushed:

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

Comment 27

7 years ago
nanot changes imported to tr:
http://hg.mozilla.org/tamarin-redux/rev/235d57a9648b
Whiteboard: fixed-in-nanojit, fixed-in-tamarin
(Reporter)

Updated

7 years ago
Blocks: 587645

Comment 28

7 years ago
http://hg.mozilla.org/mozilla-central/rev/f981332d3202
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Blocks: 590911
You need to log in before you can comment on or make changes to this bug.