Closed Bug 495995 Opened 16 years ago Closed 15 years ago

Get VTune sampling profiler integration working again

Categories

(Tamarin Graveyard :: Tools, defect, P5)

x86
Windows Vista
defect

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)

it doesn't compile anymore, and VTune 9.1 has shipped with the api built in, and updated documentation.
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)
Attachment #381136 - Flags: review?(rreitmai) → review+
Summary: Get VTune integration working again → Get VTune sampling profiler integration working again
Blocks: 496339
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
Attached patch final round of x86 vtune tweaks (obsolete) — Splinter Review
Attachment #382359 - Attachment is obsolete: true
Attachment #382563 - Flags: review?(rreitmai)
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+
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.
come to think of it the Shark support will be a good test case to drive a clean factoring. anyway...
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
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...
Attached patch Updated patch (obsolete) — Splinter Review
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)
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.
other changes in CodegenLIR.cpp look like they got clobbered when merging too (moving code around in ::prologue(), at least)
(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)?
none that I can think of
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-
Not intentional, perhaps I included more than I meant to.
Priority: -- → P5
Target Milestone: --- → flash10.1
Target Milestone: flash10.1 → flash10.2
Assignee: edwsmith → nobody
Attached patch updated patch for latest code (obsolete) — Splinter Review
Assignee: nobody → wsharp
Attachment #401982 - Attachment is obsolete: true
Attachment #457319 - Flags: superreview?(edwsmith)
Attachment #457319 - Flags: review?(stejohns)
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 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+
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+
Attachment #457319 - Flags: feedback?(nnethercote)
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+
Might be nice to have lirasm insert LIR_file/LIR_line in --random mode, too. (Fine for a followup bug.)
Minor changes from last one.
Attachment #457319 - Attachment is obsolete: true
Attachment #459455 - Flags: review?(stejohns)
Attachment #459455 - Attachment is patch: true
Attachment #459455 - Flags: review?(stejohns) → review+
Whiteboard: fixed-in-nanojit, fixed-in-tamarin
Blocks: 587645
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 590911
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: