From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 4D66E6EC60; Mon, 29 Mar 2021 12:47:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4D66E6EC60 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617011271; bh=TaFhK4iDcw7Oe59wdQjv0I9Gy4A7Pmjv/08vecmBzsg=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=IygJZ7x4hibAr6HKIaPIfitf5Ktpl5PYoUV7ttE0Pfr7dWbcQtkhHdPGZvXf9c80D TldPwmvKLlKF3C9ZvJQk4uCe4sSqdNKVKiHng1+M8nZIKUqYxdbP4esREY4ooEaWFN 2MRTyI67JRiUh8Bdkzef+u2fnP6Ba0p5FqyH9zHY= Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 957246EC60 for ; Mon, 29 Mar 2021 12:47:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 957246EC60 Received: by smtp38.i.mail.ru with esmtpa (envelope-from ) id 1lQoUv-0001g7-Oc; Mon, 29 Mar 2021 12:47:50 +0300 Date: Mon, 29 Mar 2021 12:46:56 +0300 To: Igor Munkin Message-ID: References: <20210326164855.30242-1-skaplun@tarantool.org> <20210326203356.GJ29703@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210326203356.GJ29703@tarantool.org> X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E32947A0146560F8BA709498CFB6209D8582A182A05F538085040A689B9003CD878D21990A66C2D0998B3CE139D9947621B808671C5475DA433D8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE792C68BF9CD4C0E9EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CDE631A26C8A2C128638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C7428A34725AB662D867376136FE62C6595257062F7ED2AD2A471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658359CC434672EE6371117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73528A6D463EDFD0DBBC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5A6D28A08D16E3830136EA19E33EDFF1EA5B7771CBAAAA0B0D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D16EA493CC1FD9F81EA52B3EC31C2A4374625FABD20F17D64397232463282ED27A8DBB1CE2EF067B1D7E09C32AA3244C53643F64C855A5731BABC20649690F8FA8CE788DE6831205FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojljIiQOC84rRo3/h5ysSjxQ== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4D21B905A02CE83A4E7845C1ED2773FEC7D02C0A1B8F0975FF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Igor, Thanks for the review! On 26.03.21, Igor Munkin wrote: > Sergey, > > Thanks for the patch! LGTM, except the couple of nits below. > > On 26.03.21, Sergey Kaplun wrote: > > Profiler parser output looks like the following: > > | ALLOCATIONS > > | @true_memleak.lua:6, line 9: 3 144 0 > > | @true_memleak.lua:6, line 8: 2 41 0 > > > > Line of function definition is confusing for users and looks redundant. > > Typo: s/is confusing for/confuses/. > > > Also, these "magic numbers" may shock users. > > Minor: Are you still talking about the line of function definition or > about the events/bytes here? It's a bit unclear from the wording. Thanks, reworded the commit message as the following: =================================================================== tools: make memprof parser output user-friendly Profiler parser output looks like the following: | ALLOCATIONS | @true_memleak.lua:6, line 9: 3 144 0 | @true_memleak.lua:6, line 8: 2 41 0 Line of function definition confuses users and looks redundant. Also, these "magic numbers" following after event location may shock users. This patch removes lines of function definitions from the output. Info about the line function definition is saved inside symbol info table by field `linedefined` (it can be used later for a user's custom parser). Field `name` is renamed to `source`. Moreover, explanatory words and signs are added to numbers for more verbose output. After the patch, profiler parser output looks like the following: | ALLOCATIONS | @true_memleak.lua:9: 3 events +144 bytes -0 bytes | @true_memleak.lua:8: 2 events +41 bytes -0 bytes Resolves tarantool/tarantool#5811 Part of tarantool/tarantool#5657 =================================================================== > > > > > This patch removes lines of function definitions from the output. > > Info about the line function definition is saved inside symbol info > > table by field `defined` (it can be used later for a user's custom > > parser). Moreover, explanatory words and signs are added to numbers > > for more verbose output. > > > > After the patch, profiler parser output looks like the following: > > | ALLOCATIONS > > | @true_memleak.lua:9: 3 events +144 bytes -0 bytes > > | @true_memleak.lua:8: 2 events +41 bytes -0 bytes > > > > Resolves tarantool/tarantool#5811 > > Part of tarantool/tarantool#5657 > > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5811-user-friendly-memprof > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5811-user-friendly-memprof > > Issues: > > * https://github.com/tarantool/tarantool/issues/5811 > > * https://github.com/tarantool/tarantool/issues/5657 > > > > test/tarantool-tests/misclib-memprof-lapi.test.lua | 4 +++- > > tools/memprof/humanize.lua | 2 +- > > tools/utils/symtab.lua | 5 +++-- > > 3 files changed, 7 insertions(+), 4 deletions(-) > > Missed ChangeLog entry, feel free to change it at your pleasure: =================================================================== ##feature/luajit * Make LuaJIT memory profiler parser output more user-friendly (gh-5811). **Breaking change**: Info about the line function definition is saved inside symbol info table by field `linedefined` now. Field `name` was renamed to `source`. =================================================================== > > > > > diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua > > index 109a39db..2d5814c6 100644 > > --- a/tools/memprof/humanize.lua > > +++ b/tools/memprof/humanize.lua > > @@ -20,7 +20,7 @@ function M.render(events, symbols) > > > > for i = 1, #ids do > > local event = events[ids[i]] > > - print(string.format("%s: %d\t%d\t%d", > > + print(string.format("%s: %d events\t+%d bytes\t-%d bytes", > > Minor: Heh, "events" is still plural even if is 1. Feel free > to ignore (the current implementation might ease the postprocessing). Valgring use plural too for different kind of messages. | ==24036== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) Ignoring. > > > symtab.demangle(symbols, event.loc), > > event.num, > > event.alloc, > > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > > index 03aadbd5..6cfa1065 100644 > > --- a/tools/utils/symtab.lua > > +++ b/tools/utils/symtab.lua > > @@ -25,7 +25,8 @@ local function parse_sym_lfunc(reader, symtab) > > local sym_line = reader:read_uleb128() > > > > symtab[sym_addr] = { > > - name = string_format("%s:%d", sym_chunk, sym_line), > > + name = sym_chunk, > > + defined = sym_line, > > Minor: Since you split "name" into two components -- file name and line > the function is defined -- it looks natural to me to use the same names > as does: source + linedefined. Anyway, feel free to > ignore, since the naming you chose is also fine. Yep, "backward compatibility" is broken anyway by this commit, so I renamed these fields. Side note: I didn't rerun tests on CI, but checked it locally. =================================================================== diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua index 6aa33198..9cfa6f29 100644 --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua @@ -57,7 +57,7 @@ local function fill_ev_type(events, symbols, event_type) elseif symbols[addr] then ev_type[event.loc.line] = { name = string.format( - "%s:%d", symbols[addr].name, symbols[addr].defined + "%s:%d", symbols[addr].source, symbols[addr].linedefined ), num = event.num, } diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua index 6cfa1065..3ed1dd13 100644 --- a/tools/utils/symtab.lua +++ b/tools/utils/symtab.lua @@ -25,8 +25,8 @@ local function parse_sym_lfunc(reader, symtab) local sym_line = reader:read_uleb128() symtab[sym_addr] = { - name = sym_chunk, - defined = sym_line, + source = sym_chunk, + linedefined = sym_line, } end @@ -81,7 +81,7 @@ function M.demangle(symtab, loc) end if symtab[addr] then - return string_format("%s:%d", symtab[addr].name, loc.line) + return string_format("%s:%d", symtab[addr].source, loc.line) end return string_format("CFUNC %#x", addr) =================================================================== > > > } > > end > > > > > > > -- > > 2.31.0 > > > > -- > Best regards, > IM -- Best regards, Sergey Kaplun