Re: [GIT PULL 0/4] perf/annotate loop detection V2, fixes

[Linus Torvalds]

On Thu, Apr 26, 2012 at 8:06 AM, Arnaldo Carvalho de Melo
<acme@xxxxxxxxxxxxx> wrote:
>
> Fixes on top of the previous perf/annotate pull request
>
> . Sometimes a jump points to an offset with no instructions, make the
>  mark jump targets function handle that, for now just ignoring such
>  jump targets, more investigation is needed to figure out how to cope
>  with that.
>
> . Handle jump targets that are outside the function, for now just don't
>  try to draw the connector arrow, right thing seems to be to mark this
>  jump with a -> (right arrow) and handle it like a callq.

Ok, this looks pretty good, but I do have a few comments:

 - the "jumping around" of the loop view is distracting. I think it's
a very useful concept and generally well done, but quite frankly,
scrolling around a function and having the loop detection jump around
as you just want to scroll down is not a good interface. It just makes
the screen flicker annoyingly.

   I think it would be better to just make it static. I'd suggest
perhaps to show the loop when you explicitly ask for it (by pressing
"space" on the branch or the target or whatever), and *perhaps* by
default just statically show any non-overlapping loops (I don't care
what the priority is: inner-most, outer-most or "first branch seen" or
whatever).

 - the instruction decode is now broken. For example, the instruction

        callq  *0x10(%rax)

   now shows up as

        callq  *10

   which is obviously completely bogus - it's even broken the
constant. Your address simplification does something horribly bad.

Btw, since "retq" has a back arrow, maybe "callq" should have a
forward-pointing arrow on it too?

Talking about instruction decode, the instructions that should really
be simplified are things like this:

 - nopl   0x0(%rax,%rax,1)

   That's a totally worthless instruction. It should either be hidden
entirely, or perhaps just called "nop5" (for 5-byte nop)

 - mov    (%r9,%rsi,1),%rax

   That ",1" is completely bogus, and I don't understand why the tools
show that idiotic format to begin with. It's pure garbage. It adds
zero information.

 - mov    0x0(%r13),%r13

   That "0x0" is more useless garbage in the same vein. It is useless,
and just shows instruction encoding details that don't matter.

 - shl    $0x3,%ecx

   What does the "0x" part here really tell us? I'd suggest dropping
it for any constant < 9, because there really is no semantic value to
it.

 - mov    0xb897d9(%rip),%ecx        # ffffffff81c7beb8 <d_hash_shift>

   I think you should use the same simplification that you use for
call (once it is fixed), and just show this as

   mov    d_hash_shift,%ecx

because that's what both a human and a compiler would have written.
The "0xb897d9(%rip)" is not adding any information outside of (again)
pointless instruction encoding details.

Give the encoding details in the 'O'ffset view, by all means.

Btw, don't get me wrong. I really like the changes. It's just that now
that the asm is almost readable, the remaining stupid default decode
format details just show up so much more clearly.

Oh, and I think the long vertical line should just be removed. It's
not really adding anything, and with the loop lines it's just
distracting. Also, it's not even a solid line: it gets colorized by
percentage, and generally doesn't look good.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/