Closed
Bug 495995
Opened 16 years ago
Closed 15 years ago
Get VTune sampling profiler integration working again
Categories
(Tamarin Graveyard :: Tools, defect, P5)
Tracking
(Not tracked)
RESOLVED
FIXED
Q3 11 - Serrano
People
(Reporter: edwsmith, Assigned: wsharp)
References
Details
(Whiteboard: fixed-in-nanojit, fixed-in-tamarin)
Attachments
(1 file, 5 obsolete files)
54.45 KB,
patch
|
stejohns
:
review+
|
Details | Diff | Splinter Review |
it doesn't compile anymore, and VTune 9.1 has shipped with the api built in, and updated documentation.
Reporter | ||
Comment 1•16 years ago
|
||
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•16 years ago
|
Attachment #381136 -
Flags: review?(rreitmai) → review+
Reporter | ||
Updated•16 years ago
|
Summary: Get VTune integration working again → Get VTune sampling profiler integration working again
Reporter | ||
Comment 2•16 years ago
|
||
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•16 years ago
|
||
Attachment #382359 -
Attachment is obsolete: true
Attachment #382563 -
Flags: review?(rreitmai)
Comment 4•16 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•16 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•16 years ago
|
||
come to think of it the Shark support will be a good test case to drive a clean factoring. anyway...
Comment 7•16 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•16 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•16 years ago
|
||
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•16 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•16 years ago
|
||
other changes in CodegenLIR.cpp look like they got clobbered when merging too (moving code around in ::prologue(), at least)
Comment 12•16 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•16 years ago
|
||
none that I can think of
Comment 14•16 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•16 years ago
|
||
Not intentional, perhaps I included more than I meant to.
Updated•16 years ago
|
Priority: -- → P5
Target Milestone: --- → flash10.1
Reporter | ||
Updated•15 years ago
|
Assignee: edwsmith → nobody
Assignee | ||
Comment 16•15 years ago
|
||
Assignee: nobody → wsharp
Attachment #401982 -
Attachment is obsolete: true
Attachment #457319 -
Flags: superreview?(edwsmith)
Attachment #457319 -
Flags: review?(stejohns)
Assignee | ||
Comment 18•15 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•15 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•15 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•15 years ago
|
Attachment #457319 -
Flags: feedback?(nnethercote)
Comment 21•15 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 22•15 years ago
|
||
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•15 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•15 years ago
|
||
Minor changes from last one.
Attachment #457319 -
Attachment is obsolete: true
Attachment #459455 -
Flags: review?(stejohns)
Updated•15 years ago
|
Attachment #459455 -
Attachment is patch: true
Updated•15 years ago
|
Attachment #459455 -
Flags: review?(stejohns) → review+
Assignee | ||
Comment 25•15 years ago
|
||
Nanojit central part: http://hg.mozilla.org/projects/nanojit-central/rev/602fdcb72884
Assignee | ||
Comment 26•15 years ago
|
||
TR changes pushed:
http://hg.mozilla.org/tamarin-redux/rev/fdfcc69a0300
Reporter | ||
Comment 27•15 years ago
|
||
nanot changes imported to tr:
http://hg.mozilla.org/tamarin-redux/rev/235d57a9648b
Whiteboard: fixed-in-nanojit, fixed-in-tamarin
Comment 28•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•