KDAB / hotspot

The Linux perf GUI for performance analysis.
4.16k stars 257 forks source link

Attribute inline cost to caller #586

Open lievenhey opened 10 months ago

lievenhey commented 10 months ago

From #568:

I try to explain what I've thought to understand and what I think should be the UI for that.

Running objdump shows the disassembly for the "outer" function, therefore the inlined code is part of the disassembly, possibly multiple times (= we can "see" the specific generation for this specific place). Code-wise we see a function call.

If we can deduce the place of that disassembly to be part of the "calling" place (where it is inlined) then we can directly attribute the costs there.

If we can't do it, then we still have the "original" place (source reference and/or function name) and on the other hand we have the costs in the function of "calling" it (this is something that can be seen in the caller/callee tab, also with this PR applied). Ideally we'd then be able to attribute the "summed" costs for the disassembly to the calling place -but I do recognize that in this case we'd have to match that in the code, which is not possible to always get correct because of the preprocessor work (for example macros) - we shouldn't start that path.

This leaves the question: Is there a way to deduce the "calling" source reference for functions that are inlined? If we yes, then we should attribute the total costs to that place.

milianw commented 10 months ago

I would need a clear example code base with actual and expected data. I fail to understand this purely on this description I'm afraid

lievenhey commented 10 months ago

Here is the rest of the conversation: grafik

GitMensch commented 10 months ago

Here's a bit more information in pictures:

the callee view shows (likely correct) costs of inline functions: grafik as we found, there is no guaranteed single place for inlined functions, so "Disassemble" is not active

the location view shows the cost per location, like grafik

We therefore can deduce from the caller/callee tab, that:

We can open the disassembly of the function we looked at (here: inspect_common) and see on the disassembly view the disassembly for the inlined function at the right place (which sadly does not contain any costs with the current appimage, I think it did so earlier) and in the source view we can recognize that the line with that high cost is exactly the inlined function grafik (we may have deduced that from their identical numbers)

This FR is mainly about having the inlined function costs (here 22.8) assigned as "cycles incl". Providing the detailed costs in the disassembly (again?) would be very useful.

lievenhey commented 5 months ago

I think I got something to reproduce this: a.h:

#ifndef A_H
#define A_H

int testFunc();

#endif // A_H

a.c:

#include "a.h"

static __attribute__((__always_inline__)) inline int test()
{
    /* this is from a.c */
    int loopCounter = 0;
    for (int i = 0; i < 100000000; i++) {
        loopCounter += 1;
    }
    return loopCounter;
}

int testFunc()
{
    return test();
}

main.c:

#include "a.h"
#include <stdio.h>

static int test()
{
    int counter = 0;
    /* this is form main.c */
    for (int i = 0; i < 1000000; i++) {
        counter += 1;
    }
    return counter;
}

int main()
{
    printf("test(): %d\n", test());
    printf("testfunc(): %d\n", testFunc());
    return 0;
}

compile with: gcc -g a.c main.c

Then hotspot shows this in the bottom up view: grafik But the disassembler says: grafik

GitMensch commented 5 months ago

That is no cpu cost shown in the code view (and also none in the disassembly) and you agree that cost should be shown in both, right?

milianw commented 2 months ago

https://github.com/KDAB/hotspot/issues/671 has some more info and a simple example that shows how to reproduce this issue. I'll close this ticket here as a duplicate as the other one is nicely actionable on its own. Reopen if you think this is a mistake on my part.

GitMensch commented 2 months ago

I'd say that closing this as a duplicate (or actually closing the newer one as duplicate) would be more appropriate. If #672 fixes the issue (which was clearly visible by the sample @lievenhey gave above) then I don't mind and am looking forward to get the fix released - but please check with his sample code from June 7th.

milianw commented 2 months ago

ah IIUC then the issue here is (also) about the left-hand file/line side inclusive cost being 0, whereas #672 handles the right-hand addr cost side of things?

GitMensch commented 2 months ago

yes, that's right for both my example output and the one from the sample given above, the later can be reproduced with current continuous build

$ perf annotate -s testFunc

Samples: 695  of event 'cycles:u', 4000 Hz, Event count (approx.): 701124593
testFunc  /tmp/a.out [Percent: local period]
Percent│
       │    int testFunc()
       │    {
       │      push %rbp
       │      mov  %rsp,%rbp
       │    test():
       │    int loopCounter = 0;
       │      movl $0x0,-0x4(%rbp)
       │    for (int i = 0; i < 100000000; i++) {
       │      movl $0x0,-0x8(%rbp)
       │    ↓ jmp  1c
       │    loopCounter += 1;
       │14:   addl $0x1,-0x4(%rbp)
       │    for (int i = 0; i < 100000000; i++) {
 99,71 │      addl $0x1,-0x8(%rbp)
       │1c:   cmpl $0x5f5e0ff,-0x8(%rbp)
  0,29 │    ↑ jle  14
       │    return loopCounter;
       │      mov  -0x4(%rbp),%eax
       │    testFunc():
       │    return test();
       │    }
       │      pop  %rbp
       │    ← retq

disassembly continuous improved - on the right side - after PR 672: or672