[Interpreter] Add a bytecode annotate tool.

[Interpreter] Add a bytecode annotate tool.

Adds a tool which enables annotation of the disassembly of bytecode handlers
based on perf output.

BUG=4899
LOG=N

Committed: https://crrev.com/24709a62ce7ad456ab264ae732a5103dc101b159
Cr-Commit-Position: refs/heads/master@{#36145}

[rmcilroy, 2016-05-03 14:29:21]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2016-05-03 14:31:53]

rmcilroy@chromium.org changed reviewers:
+ jarin@chromium.org, ssanfilippo@chromium.org

[rmcilroy, 2016-05-03 14:31:54]

This is a slightly scrappy tool for annotating a bytecode handler's disassembly
with perf counts (equivalent of "perf annotate" if we had the JITed code was in
the .so).

Stefano, please take a look at the tool
Jaro, please take a look at the minor tweak to run-perf.sh and for OWNER's
stamp.

[Jarin, 2016-05-06 08:51:41]

lgtm for run-perf.sh

[rmcilroy, 2016-05-09 09:59:33]

Ping Stefano?

[Stefano Sanfilippo, 2016-05-09 11:06:26]

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
File tools/ignition/linux_perf_bytecode_annotate.py (right):

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:58: if
symbol_and_offset.startswith("BytecodeHandler:" + bytecode_name):
nit. Consider hoisting "BytecodeHandler:" + bytecode_name.

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:93: return offsets.pop(0) if
offsets else -1
nit. Popping the last element is more efficient, consider using:

  offsets = sorted(offsets_count, reverse=True)
  def next_offset():
    return offsets.pop() if offsets else -1

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:102: print "{:>8d} ({:>3d}%)
".format(count, percentage),
Please use format string "{:>5.1f}" and 100.0 as multiplier to print percentages
with one decimal point.

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:156: d8_path = root_path +
"/out/" + program_options.arch + ".debug/d8"
nit. A relative path from the cwd and string interpolation appear to give a more
readable line:

  d8_path = "../../out/{}.debug/d8".format(program_options.arch)

WDYT?

[Stefano Sanfilippo, 2016-05-10 11:24:05]

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
File tools/ignition/linux_perf_bytecode_annotate.py (right):

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:93: return offsets.pop(0) if
offsets else -1
On 2016/05/09 11:06:26, Stefano Sanfilippo wrote:
> nit. Popping the last element is more efficient, consider using:
> 
>   def next_offset():
>     return offsets.pop() if offsets else -1
> 

Thinking about it, None instead of -1 better conveys the idea that there is no
next_offset, saving us the assumption that offsets are positive. Please see it
fits your use case.

[rmcilroy, 2016-05-10 11:37:17]

Comments addressed, PTAL.

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
File tools/ignition/linux_perf_bytecode_annotate.py (right):

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:58: if
symbol_and_offset.startswith("BytecodeHandler:" + bytecode_name):
On 2016/05/09 11:06:26, Stefano Sanfilippo wrote:
> nit. Consider hoisting "BytecodeHandler:" + bytecode_name.

Done.

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:93: return offsets.pop(0) if
offsets else -1
On 2016/05/09 11:06:26, Stefano Sanfilippo wrote:
> nit. Popping the last element is more efficient, consider using:
> 
>   offsets = sorted(offsets_count, reverse=True)
>   def next_offset():
>     return offsets.pop() if offsets else -1
> 

Done.

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:102: print "{:>8d} ({:>3d}%)
".format(count, percentage),
On 2016/05/09 11:06:26, Stefano Sanfilippo wrote:
> Please use format string "{:>5.1f}" and 100.0 as multiplier to print
percentages
> with one decimal point.

Done.

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:156: d8_path = root_path +
"/out/" + program_options.arch + ".debug/d8"
On 2016/05/09 11:06:25, Stefano Sanfilippo wrote:
> nit. A relative path from the cwd and string interpolation appear to give a
more
> readable line:
> 
>   d8_path = "../../out/{}.debug/d8".format(program_options.arch)
> 
> WDYT?

Agree for string interpolation, but we can't use a relative path from cwd since
we aren't necessarily running the tool from the directory it is located in.

[Stefano Sanfilippo, 2016-05-10 12:25:36]

One comment to address, otherwise LGTM. Thanks.

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
File tools/ignition/linux_perf_bytecode_annotate.py (right):

https://codereview.chromium.org/1945673002/diff/20001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:156: d8_path = root_path +
"/out/" + program_options.arch + ".debug/d8"
On 2016/05/10 11:37:16, rmcilroy wrote:
> On 2016/05/09 11:06:25, Stefano Sanfilippo wrote:
> > nit. A relative path from the cwd and string interpolation appear to give a
> more
> > readable line:
> > 
> >   d8_path = "../../out/{}.debug/d8".format(program_options.arch)
> > 
> > WDYT?
> 
> Agree for string interpolation, but we can't use a relative path from cwd
since
> we aren't necessarily running the tool from the directory it is located in.

Agreed. I didn't consider that at first.

https://codereview.chromium.org/1945673002/diff/40001/tools/ignition/linux_pe...
File tools/ignition/linux_perf_bytecode_annotate.py (right):

https://codereview.chromium.org/1945673002/diff/40001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:103: percentage = 100 * count /
total
Please change 100 to 100.0, otherwise the result will be integer.

[rmcilroy, 2016-05-10 14:09:03]

https://codereview.chromium.org/1945673002/diff/40001/tools/ignition/linux_pe...
File tools/ignition/linux_perf_bytecode_annotate.py (right):

https://codereview.chromium.org/1945673002/diff/40001/tools/ignition/linux_pe...
tools/ignition/linux_perf_bytecode_annotate.py:103: percentage = 100 * count /
total
On 2016/05/10 12:25:36, Stefano Sanfilippo wrote:
> Please change 100 to 100.0, otherwise the result will be integer.

Done.

[rmcilroy, 2016-05-10 14:09:12]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-05-10 14:09:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
ssanfilippo@chromium.org
Link to the patchset: https://codereview.chromium.org/1945673002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-10 14:09:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1945673002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1945673002/60001

[commit-bot: I haz the power, 2016-05-10 15:02:55]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-05-10 15:04:42]

Description was changed from

==========
[Interpreter] Add a bytecode annotate tool.

Adds a tool which enables annotation of the disassembly of bytecode handlers
based on perf output.

BUG=4899
LOG=N
==========

to

==========
[Interpreter] Add a bytecode annotate tool.

Adds a tool which enables annotation of the disassembly of bytecode handlers
based on perf output.

BUG=4899
LOG=N

Committed: https://crrev.com/24709a62ce7ad456ab264ae732a5103dc101b159
Cr-Commit-Position: refs/heads/master@{#36145}
==========

[commit-bot: I haz the power, 2016-05-10 15:04:43]

Patchset 3 (id:??) landed as
https://crrev.com/24709a62ce7ad456ab264ae732a5103dc101b159
Cr-Commit-Position: refs/heads/master@{#36145}