873128 - OdinMonkey: Feed source line number information to VTune

Status: RESOLVED WONTFIX

(Core :: JavaScript: WebAssembly, enhancement, P5)

Description

[Sean Stangl [:sstangl]]

OdinMonkey's VTune instrumentation is currently lacking source file and line number information.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: WIP patch

WIP patch from a while ago, which maps byte offset to (line, column) in CodeGenerator::visitSourceNote(). I plan to hook the information up to VTune when I'm back in the office and can test.

Comment 2

[Joe Olivas]

Attachment: WIP v1

Takes Sean's previous patch and hooks it into the VTune data. Right now, it still looks to be off by one line, but it's a start.

Comment 3

[Joe Olivas]

Attachment: WIP v1

Minor fix to previous

Comment 4

[Joe Olivas]

Attachment: Reporting to VTune

Getting the patch actually updated this time.

Comment 5

[Sean Stangl [:sstangl]]

Attachment: Maybe fix off-by-one line error.

This may fix the line numbering, by making it so that each LSourceNote is visited after the instruction it is bound to, instead of before: VTune apparently uses bounds in the opposite direction I expected it to.

Comment 6

[Sean Stangl [:sstangl]]

Attachment: Maybe fix off-by-one-line error. [actual patch]

Comment 7

[Joe Olivas]

Yes, it appears your fix works; at least I can make sense of most of the lines that are in bullet.js and copy.js from the emscripten test suite.

There are some lines which aren't mapped to any asm. Is this expected?

Comment 8

[Douglas Crosher [:dougc]]

Attachment: Rebased, with minor fix.

Minor fix to visitSourceNote for the case in which the sourceLineVector_ is empty.  Also rebased.

Bug 873522 also needs asm.js source info for perf and currently crashes on asm.js code, and perhaps there is an opportunity to refactor.

Comment 9

[Douglas Crosher [:dougc]]

Attachment: Fix the opcode offest passed to vtune, and rework to handle the last instructions in blocks.

The opcode offset passed to vtune seemed to be wrong, pointing to the start of the next instruction, rather than the last byte of the previous instruction.

The strategy of adding LIR SourceNote instructions was problematic for the last instruction in blocks and not working for these.  This patch tries a different approach.

Comment 10

[Douglas Crosher [:dougc]]

Attachment: Rebase.

Comment 11

[Sean Stangl [:sstangl]]

I was attempting to rebase the patch from Comment 10 on top of the recent Perf changes, but it looks like at least one file is missing (defining LineNumberInfo).

Comment 12

[Douglas Crosher [:dougc]]

Attachment: Fix include files.

Some of the include files seem to have been remove in another patch.  Perhaps this helps.

Comment 13

[Douglas Crosher [:dougc]]

Attachment: Rebased, WIP.

Here's my WIP in case it helps.  Note that there is an unresolved issue with the saving of a pointer to a JSAtom for the function name.  On the ARM this causes corruption (similar perf profiling support), and I have seem some crashes on the x86 and x64 too using vtune which this might have contributed to.  There is a hack workaround that malloc's storage for the name, but I intend to study up on the GC code and fix it appropriately - help would be appreciated.

Comment 14

[Douglas Crosher [:dougc]]

Support for noting more dynamic regions is being added for the 'perf' support, and this would help using vtune too.  For example: exit and entry trampolines etc, baseline code, asm.js entry and exits regions.

Comment 15

[Douglas Crosher [:dougc]]

Attachment: Rebase.

Just a rebase.  Compiles. Not tested.  Assumes that the 'perf' support patch of bug 893363 has been applied.

Comment 16

[Douglas Crosher [:dougc]]

I'm going to abandon this approach because:

* there is block level source info in the 'perf' support and perhaps it would better to use this for vtune.

* its not clear that the event counts are accurate to the instruction level.  Even if they were this accurate, vtune allows a side-by-side view of the source and the assembler code and gives the instruction level counts that can be manually related back to code in a block.

There is one more rebase in the pipeline that will be submitted after some other patches land.

Comment 17

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 18

[Lars T Hansen [:lth]]

No action for six years and neither contributer is working on this any longer.