From mboxrd@z Thu Jan 1 00:00:00 1970 From: Milian Wolff Subject: Re: [PATCH 0/7] generate full callchain cursor entries for inlined frames Date: Wed, 24 May 2017 15:42:59 +0200 Message-ID: <4449663.nJV0hUTq9C@milian-kdab2> References: <20170518193411.22380-1-milian.wolff@kdab.com> <20170522090643.GA20009@sejong> <1535446.SrsFzc0SgI@milian-kdab2> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart5254200.YPyYFa5Yj9"; micalg="sha256"; protocol="application/pkcs7-signature" Return-path: In-Reply-To: <1535446.SrsFzc0SgI@milian-kdab2> Sender: linux-kernel-owner@vger.kernel.org To: Milian Wolff Cc: Namhyung Kim , Linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, kernel-team@lge.com List-Id: linux-perf-users.vger.kernel.org --nextPart5254200.YPyYFa5Yj9 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" On Wednesday, May 24, 2017 1:46:04 PM CEST Milian Wolff wrote: > On Monday, May 22, 2017 11:06:43 AM CEST Namhyung Kim wrote: > > Hi Milian, > > > > On Thu, May 18, 2017 at 10:05:36PM +0200, Milian Wolff wrote: > > > On Donnerstag, 18. Mai 2017 21:34:04 CEST Milian Wolff wrote: > > > > This series of patches completely reworks the way inline frames are > > > > handled. Instead of querying for the inline nodes on-demand in the > > > > individual tools, we now create proper callchain nodes for inlined > > > > frames. The advantages this approach brings are numerous: > > > > > > > > - less duplicated code in the individual browser > > > > - aggregated cost for inlined frames for the --children top-down list > > > > - various bug fixes that arose from querying for a srcline/symbol > > > > based > > > > on > > > > > > > > the IP of a sample, which will always point to the last inlined > > > > frame > > > > instead of the corresponding non-inlined frame > > > > > > > > - overall much better support for visualizing cost for heavily-inlined > > > > C++ > > > > > > > > code, which simply was confusing and unreliably before > > > > > > > > - srcline honors the global setting as to whether full paths or > > > > basenames > > > > > > > > should be shown > > > > > > > > For comparison, below lists the output before and after for `perf > > > > script` > > > > > > > and `perf report`. The example file I used to generate the perf data is: > > > And of course shortly after sending this patch series I notice the first > > > issues ;-) The new behavior shows confusing results for `-g function` > > > because match_chain uses sym->start. I fixed this locally to compare the > > > actual > > > > > function name if either of the two symbols is an inlined fake symbol: > > Why not making the fake symbol has start addr of the sample IP and > > length of 1. The histogram sort code also compares the sym->start > > which might confuse the output of the children mode too IMHO. > > I can try that out, thank you for the suggestion. But I think it can easily > break in different ways. I.e. when the same inline function gets used at > different IPs, it should actually be considered to be the same function when > we group/merge/aggregate. I updated the `match_chain` function accordingly, > to do a symname / srcline comparison on inlined frames, instead of relying > on the symbol start/end. I think using the IP for the fake symbols won't be > more reliable here, don't you think? > > In the end, I think we'll always have to special-case inlined fake symbols > when we aggregate data, since the sym start/end is always going to be some > arbitrary value that may or may not be what we want it to be. Doing the > explicit comparison on e.g. srcline/symname is always going to be the most > reliable option, as it also directly results in a proper aggregation based > on the strings that the user will see in the end. I haven't yet tried it out, but I think I can come up with a way to break your approach easily. Assume the following pseudo-code: void tail() { instr1; // IP1 instr2; // IP2 } void mid() { tail(); } void main() { mid(); } Now, assume both `tail` and `mid` get inlined into `main`. If we get one sample each for both IP1 and IP2, we want the following merged structure if we merge based on symbol: sym | incl | self main | 2 | 0 mid | 2 | 0 tail | 2 | 2 If we would give the inlined fake-symbols a start of the IP, i.e. either IP1 or IP2, then we would end up with this (unexpected) behavior instead: sym | incl | self main | 2 | 0 mid | 1 | 0 mid | 1 | 0 tail | 1 | 1 tail | 1 | 1 The reason is that the fake symbols for the inlined frames would be considered to be different functions since their start/end are not equal. This is "wrong" in my eyes - we really have to do symbol name comparisons for inlined frames, and also include srcline if that is desired. If you think the above is not a valid assessment, I'll try to change my patch series to use the IP + 1 trick you suggest. But I really don't think it's going to work. Cheers -- Milian Wolff | milian.wolff@kdab.com | Software Engineer KDAB (Deutschland) GmbH&Co KG, a KDAB Group company Tel: +49-30-521325470 KDAB - The Qt Experts --nextPart5254200.YPyYFa5Yj9 Content-Type: application/pkcs7-signature; name="smime.p7s" Content-Disposition: attachment; filename="smime.p7s" Content-Transfer-Encoding: base64 MIAGCSqGSIb3DQEHAqCAMIACAQExDzANBglghkgBZQMEAgEFADCABgkqhkiG9w0BBwEAAKCCDEIw ggXmMIIDzqADAgECAhBqm+E4O/8ra58B1dm4p1JWMA0GCSqGSIb3DQEBDAUAMIGFMQswCQYDVQQG EwJHQjEbMBkGA1UECBMSR3JlYXRlciBNYW5jaGVzdGVyMRAwDgYDVQQHEwdTYWxmb3JkMRowGAYD VQQKExFDT01PRE8gQ0EgTGltaXRlZDErMCkGA1UEAxMiQ09NT0RPIFJTQSBDZXJ0aWZpY2F0aW9u IEF1dGhvcml0eTAeFw0xMzAxMTAwMDAwMDBaFw0yODAxMDkyMzU5NTlaMIGXMQswCQYDVQQGEwJH QjEbMBkGA1UECBMSR3JlYXRlciBNYW5jaGVzdGVyMRAwDgYDVQQHEwdTYWxmb3JkMRowGAYDVQQK ExFDT01PRE8gQ0EgTGltaXRlZDE9MDsGA1UEAxM0Q09NT0RPIFJTQSBDbGllbnQgQXV0aGVudGlj YXRpb24gYW5kIFNlY3VyZSBFbWFpbCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB AL6znlesKHZ1QBbHOAOY08YYdiFQ8yV5C0y1oNF9Olg+nKcxLqf2NHbZhGra0D00SOTq9bus3/mx gUsg/Wh/eXQ0pnp8tZ8XZWAnlyKMpjL+qUByRjXCA6RQyDMqVaVUkbIr5SU0RDX/kSsKwer3H1pT /HUrBN0X8sKtPTdGX8XAWt/VdMLBrZBlgvnkCos+KQWWCo63OTTqRvaq8aWccm+KOMjTcE6s2mj6 RkalweyDI7X+7U5lNo6jzC8RTXtVV4/Vwdax720YpMPJQaDaElmOupyTf1Qib+cpukNJnQmwygjD 8m046DQkLnpXNCAGjuJy1F5NATksUsbfJAr7FLUCAwEAAaOCATwwggE4MB8GA1UdIwQYMBaAFLuv fgI9+qbxPISOre44mOzZMjLUMB0GA1UdDgQWBBSCr2yM+MX+lmF86B89K3FIXsSLwDAOBgNVHQ8B Af8EBAMCAYYwEgYDVR0TAQH/BAgwBgEB/wIBADARBgNVHSAECjAIMAYGBFUdIAAwTAYDVR0fBEUw QzBBoD+gPYY7aHR0cDovL2NybC5jb21vZG9jYS5jb20vQ09NT0RPUlNBQ2VydGlmaWNhdGlvbkF1 dGhvcml0eS5jcmwwcQYIKwYBBQUHAQEEZTBjMDsGCCsGAQUFBzAChi9odHRwOi8vY3J0LmNvbW9k b2NhLmNvbS9DT01PRE9SU0FBZGRUcnVzdENBLmNydDAkBggrBgEFBQcwAYYYaHR0cDovL29jc3Au Y29tb2RvY2EuY29tMA0GCSqGSIb3DQEBDAUAA4ICAQB4XLKBKDRPPO5fVs6fl1bsj6JrF/bz9kkI BtTYLzXN30D+03Hj6OxCDBEaIeNmsBhrJmuubvyE7HtoSmR809AgcYboW+rcTNZ/8u/Hv+GTrNI/ AhqX2/kiQNxmgUPt/eJPs92Qclj0HnVyy9TnSvGkSDU7I5Px+TbO+88G4zipA2psZaWeEykgzClZ lPz1FjTCkk77ZXp5cQYYexE6zeeN4/0OqqoAloFrjAF4o50YJafX8mnahjp3I2Y2mkjhk0xQfhNq bzlLWPoT3m7j7U26u7zg6swjOq8hITYc3/np5tM5aVyu6t99p17bTbY7+1RTWBviN9YJzK8HxzOb XYWBf/L+VGOYNsQDTxAk0Hbvb1j6KjUhg7fO294F29QIhhmiNOr84JHoy+fNLpfvYc/Q9EtFOI5I SYgOxLk3nD/whbUe9rmEQXLp8MB933Ij474gwwCPUpwv9mj2PMnXoc7mbrS22XUSeTwxCTP9bcmU dp4jmIoWfhQm7X9w/Zgddg+JZ/YnIHOwsGsaTUgj7fIvxqith7DoJC91WJ8Lce3CVJqb1XWeKIJ8 4F7YLXZN0oa7TktYgDdmQVxYkZo1c5noaDKH9Oq9cbm/vOYRUM1cWcef20Wkyk5S/GFyyPJwG0fR 1nRas3DqAf4cXxMiEKcff7PNa4M3RGTqH0pWR8p6EjCCBlQwggU8oAMCAQICEAf6KCF9+1doL2oE OTPysLwwDQYJKoZIhvcNAQELBQAwgZcxCzAJBgNVBAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1h bmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAYBgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMT0w OwYDVQQDEzRDT01PRE8gUlNBIENsaWVudCBBdXRoZW50aWNhdGlvbiBhbmQgU2VjdXJlIEVtYWls IENBMB4XDTE3MDUyMzAwMDAwMFoXDTIwMDUyMjIzNTk1OVowggFZMQswCQYDVQQGEwJTRTEPMA0G A1UEERMGNjgzIDMxMRIwEAYDVQQIEwlWYWVybWxhbmQxEDAOBgNVBAcTB0hhZ2ZvcnMxGDAWBgNV BAkTD05vcnJpbmdzIHZhZWcgMjEPMA0GA1UEEhMGQm94IDMwMSYwJAYDVQQKDB1LbGFyw6RsdmRh bGVucyBEYXRha29uc3VsdCBBQjEdMBsGA1UECxMUQSBLREFCIEdyb3VwIENvbXBhbnkxQzBBBgNV BAsMOklzc3VlZCB0aHJvdWdoIEtsYXLDpGx2ZGFsZW5zIERhdGFrb25zdWx0IEFCIEUtUEtJIE1h bmFnZXIxHzAdBgNVBAsTFkNvcnBvcmF0ZSBTZWN1cmUgRW1haWwxFTATBgNVBAMTDE1pbGlhbiBX b2xmZjEkMCIGCSqGSIb3DQEJARYVbWlsaWFuLndvbGZmQGtkYWIuY29tMIIBIjANBgkqhkiG9w0B AQEFAAOCAQ8AMIIBCgKCAQEAxrzfNBVvRbiAknuTBXuQnNm9sLIFLo0vbPB6kswk78A3tA++Zn5c lQUHhGlQq1cdYxagnUpqwvG3Sod15mPSOLkAPf/mabLN7p+lFbRaUP+97ZkTZtvb4BCC3osIEFI4 G393OSFWqc2qmIPE/SwSASbAA20Fcaa2M6P1lhOk/ttUh2jIurTPF0wUycIA7lBddrOgaOA8e2m6 iLTNHtlrfRbBaUX91D5ebY+UWmIjXSQ9+CtutMzBkwnF0rZKririvOkklg9VzEGNQVHrQfDF2s/U pOtmtuVSwElauGT/KALyCFuIrYC1pmaKH8S1xODJqiRaf6jH8E+KQzKjyM/ErwIDAQABo4IB1TCC AdEwHwYDVR0jBBgwFoAUgq9sjPjF/pZhfOgfPStxSF7Ei8AwHQYDVR0OBBYEFN+m99RtIuA1bSdw 6b1brOX7X3AJMA4GA1UdDwEB/wQEAwIFoDAMBgNVHRMBAf8EAjAAMB0GA1UdJQQWMBQGCCsGAQUF BwMEBggrBgEFBQcDAjBGBgNVHSAEPzA9MDsGDCsGAQQBsjEBAgEDBTArMCkGCCsGAQUFBwIBFh1o dHRwczovL3NlY3VyZS5jb21vZG8ubmV0L0NQUzBaBgNVHR8EUzBRME+gTaBLhklodHRwOi8vY3Js LmNvbW9kb2NhLmNvbS9DT01PRE9SU0FDbGllbnRBdXRoZW50aWNhdGlvbmFuZFNlY3VyZUVtYWls Q0EuY3JsMIGLBggrBgEFBQcBAQR/MH0wVQYIKwYBBQUHMAKGSWh0dHA6Ly9jcnQuY29tb2RvY2Eu Y29tL0NPTU9ET1JTQUNsaWVudEF1dGhlbnRpY2F0aW9uYW5kU2VjdXJlRW1haWxDQS5jcnQwJAYI KwYBBQUHMAGGGGh0dHA6Ly9vY3NwLmNvbW9kb2NhLmNvbTAgBgNVHREEGTAXgRVtaWxpYW4ud29s ZmZAa2RhYi5jb20wDQYJKoZIhvcNAQELBQADggEBABf47LSJADqH+ow9INv3QM1NC/qq2bjxGvsZ 68iD11VEUAFlsYfsVTgQqUirwPVTYenXtwVBELHZyywsui1JxL7HKQetLQegDDP/RyfjReVaWxhy 3OpuItsgLVbru9QVgPifnoBFPtfZcwjeJDmeSbLT8oj4Rd0KYBOIve7WKvsfNPsNwfbLwY2zILkE LjxZcVi2AwZHDyab+dzL/3YcLuJj1lSawBGn7ilpcdZydlv4aye51pD/MemLIYLcylt+ImrmjnTV y+QlAHRF3s5FE8yAr+W1MBD/1bKZCSgFt8VQoAlz3hiQh8QqZp4Zl8WuVL4+mP/mT6VDEWgq/0Bo cukxggJuMIICagIBATCBrDCBlzELMAkGA1UEBhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIgTWFuY2hl c3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgGA1UEChMRQ09NT0RPIENBIExpbWl0ZWQxPTA7BgNV BAMTNENPTU9ETyBSU0EgQ2xpZW50IEF1dGhlbnRpY2F0aW9uIGFuZCBTZWN1cmUgRW1haWwgQ0EC EAf6KCF9+1doL2oEOTPysLwwDQYJYIZIAWUDBAIBBQCggZMwGAYJKoZIhvcNAQkDMQsGCSqGSIb3 DQEHATAcBgkqhkiG9w0BCQUxDxcNMTcwNTI0MTM0MjU5WjAoBgkqhkiG9w0BCQ8xGzAZMAsGCWCG SAFlAwQBAjAKBggqhkiG9w0DBzAvBgkqhkiG9w0BCQQxIgQgCuGohetoR/2iwWIMHU2TCC3cxr2/ LhXcIf67EBnOE/gwDQYJKoZIhvcNAQEBBQAEggEAIqRWB1kd1AjBuWR926OmgFiscOmUUBwVj1xK NuObQ97d+OzSBBPEPsm/IEfhgv0OvnVL895zULPNIuoKc8rs9QA0Qup+ioLPns6bxryeOaSo2DiN EZrX3Oz35jfdBnSRb8TTbI7vWi484o0bU0aRbZPZXNznaiJ/wrnyR0YgGuyRULFFNlY0L4J2zTfW uAGuYrlw10kY0Xd5K1FuVIoPH1nafywJ6rlQ5bVolP6RojzrXqmJjf5dVkz4+34cxztylgZpTYh/ BMtKxrXJXC5K7ZRJMUjyJPPSo87RpqFbZcmWFgeKD9pKoAHS9hM+QUgBmtLYHMDF1MT5quGYUOO+ hQAAAAAAAA== --nextPart5254200.YPyYFa5Yj9--