it doesn't compile anymore, and VTune 9.1 has shipped with the api built in, and updated documentation.
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)
Summary: Get VTune integration working again → Get VTune sampling profiler integration working again
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
Created attachment 382563 [details] [diff] [review] final round of x86 vtune tweaks
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...
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
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.
Created attachment 457319 [details] [diff] [review] updated patch for latest code
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+
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.)
Created attachment 459455 [details] [diff] [review] latest patch...fixed whitespace, added vcproj, fixed one comment Minor changes from last one.
Nanojit central part: http://hg.mozilla.org/projects/nanojit-central/rev/602fdcb72884
TR changes pushed: http://hg.mozilla.org/tamarin-redux/rev/fdfcc69a0300
nanot changes imported to tr: http://hg.mozilla.org/tamarin-redux/rev/235d57a9648b
Whiteboard: fixed-in-nanojit, fixed-in-tamarin
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.