Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
@ 2021-03-26 16:48 Sergey Kaplun via Tarantool-patches
  2021-03-26 20:33 ` Igor Munkin via Tarantool-patches
  2021-03-29 21:58 ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 9+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-03-26 16:48 UTC (permalink / raw)
  To: Sergey Ostanevich, Igor Munkin; +Cc: tarantool-patches

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.
Also, these "magic numbers" 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 `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(-)

diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index 1c36c8ab..6aa33198 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -56,7 +56,9 @@ local function fill_ev_type(events, symbols, event_type)
     }
     elseif symbols[addr] then
       ev_type[event.loc.line] = {
-        name = symbols[addr].name,
+        name = string.format(
+          "%s:%d", symbols[addr].name, symbols[addr].defined
+        ),
         num = event.num,
       }
     end
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",
       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,
   }
 end
 
@@ -80,7 +81,7 @@ function M.demangle(symtab, loc)
   end
 
   if symtab[addr] then
-    return string_format("%s, line %d", symtab[addr].name, loc.line)
+    return string_format("%s:%d", symtab[addr].name, loc.line)
   end
 
   return string_format("CFUNC %#x", addr)
-- 
2.31.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-26 16:48 [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly Sergey Kaplun via Tarantool-patches
@ 2021-03-26 20:33 ` Igor Munkin via Tarantool-patches
  2021-03-29  9:46   ` Sergey Kaplun via Tarantool-patches
  2021-03-29 21:58 ` Igor Munkin via Tarantool-patches
  1 sibling, 1 reply; 9+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-03-26 20:33 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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.

> 
> 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(-)
> 

<snipped>

> 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 <event.num> is 1. Feel free
to ignore (the current implementation might ease the postprocessing).

>        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 <debug.getinfo> does: source + linedefined. Anyway, feel free to
ignore, since the naming you chose is also fine.

>    }
>  end
>  

<snipped>

> -- 
> 2.31.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-26 20:33 ` Igor Munkin via Tarantool-patches
@ 2021-03-29  9:46   ` Sergey Kaplun via Tarantool-patches
  2021-03-29 11:16     ` Igor Munkin via Tarantool-patches
  0 siblings, 1 reply; 9+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-03-29  9:46 UTC (permalink / raw)
  To: Igor Munkin; +Cc: 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`.
===================================================================

> 
> <snipped>
> 
> > 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 <event.num> 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 <debug.getinfo> 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
> >  
> 
> <snipped>
> 
> > -- 
> > 2.31.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-29  9:46   ` Sergey Kaplun via Tarantool-patches
@ 2021-03-29 11:16     ` Igor Munkin via Tarantool-patches
  2021-03-29 13:00       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 9+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-03-29 11:16 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

Thanks for the fixes! I'll push it to the trunk as soon as Sergos gives
his LGTM.

On 29.03.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 

<snipped>

> 
> Missed ChangeLog entry, feel free to change it at your pleasure:

Great, thanks! I propose the following wording:

| ##feature/luajit
|
| * Make LuaJIT memory profiler parser output more user-friendly (gh-5811).
|   Now the source line definition where the event occurs is much clearer:
|   only source file name and allocation-related line are presented,
|   function definition line number is omitted. Moreover, event-related
|   statistics are indicated with units.
|   **Breaking change**: Line info of the line function definition is
|   saved in symbol info table by field `linedefined` now and field `name`
|   is renamed to `source` with the respect to Lua Debug API.

> 
> ===================================================================
> ##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`.
> ===================================================================
> 
> > 
> > <snipped>
> > 
> > > 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 <event.num> is 1. Feel free
> > to ignore (the current implementation might ease the postprocessing).
> 
> Valgring use plural too for different kind of messages.

Lol, never notice this.

> | ==24036== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
> 
> Ignoring.
> 

<snipped>

> > > -- 
> > > 2.31.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-29 11:16     ` Igor Munkin via Tarantool-patches
@ 2021-03-29 13:00       ` Sergey Kaplun via Tarantool-patches
  2021-03-29 15:41         ` Sergey Ostanevich via Tarantool-patches
  2021-03-29 19:11         ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 9+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-03-29 13:00 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Igor,

On 29.03.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the fixes! I'll push it to the trunk as soon as Sergos gives
> his LGTM.
> 
> On 29.03.21, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the review!
> > 
> 
> <snipped>
> 
> > 
> > Missed ChangeLog entry, feel free to change it at your pleasure:
> 
> Great, thanks! I propose the following wording:

Thanks! I like it, exept some typos:

> 
> | ##feature/luajit
> |
> | * Make LuaJIT memory profiler parser output more user-friendly (gh-5811).
> |   Now the source line definition where the event occurs is much clearer:
> |   only source file name and allocation-related line are presented,

Typo? s/are presented/is presented/

> |   function definition line number is omitted. Moreover, event-related

Typo: /function definition line number/the function definition line number/.

> |   statistics are indicated with units.
> |   **Breaking change**: Line info of the line function definition is
> |   saved in symbol info table by field `linedefined` now and field `name`
> |   is renamed to `source` with the respect to Lua Debug API.

Typo: s/to Lua Debug/to the Lua Debug/

> 

<snipped>

> > > > -- 
> > > > 2.31.0
> > > > 
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-29 13:00       ` Sergey Kaplun via Tarantool-patches
@ 2021-03-29 15:41         ` Sergey Ostanevich via Tarantool-patches
  2021-03-29 15:56           ` Sergey Kaplun via Tarantool-patches
  2021-03-29 19:11         ` Igor Munkin via Tarantool-patches
  1 sibling, 1 reply; 9+ messages in thread
From: Sergey Ostanevich via Tarantool-patches @ 2021-03-29 15:41 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]

Hi!

Thank you for the patch and review!

Let me propose some technical fix:

long/path/to/function.lua: 3 events
very/long/path/to/function.lua:	3 events

I think dump as above is not-so-readable for humans. A single-symbol fixup gives 

long/path/to/function.lua:	3 events
very/long/path/to/function.lua:	3 events

+    print(string.format("%s:\t%d events\t+%d bytes\t-%d bytes",

The better solution would be to keep the longest name and append/prepend all of them
with spaces, so that the following will not happen:

long/path/to/funct.lua:	3 events
very/long/path/to/funct.lua:	3 events


All of these are too much ‘vkusovschina’, so you have my LGTM.

Sergos



> On 29 Mar 2021, at 16:00, Sergey Kaplun <skaplun@tarantool.org <mailto:skaplun@tarantool.org>> wrote:
> 
> Igor,
> 
> On 29.03.21, Igor Munkin wrote:
>> Sergey,
>> 
>> Thanks for the fixes! I'll push it to the trunk as soon as Sergos gives
>> his LGTM.
>> 
>> On 29.03.21, Sergey Kaplun wrote:
>>> Igor,
>>> 
>>> Thanks for the review!
>>> 
>> 
>> <snipped>
>> 
>>> 
>>> Missed ChangeLog entry, feel free to change it at your pleasure:
>> 
>> Great, thanks! I propose the following wording:
> 
> Thanks! I like it, exept some typos:
> 
>> 
>> | ##feature/luajit
>> |
>> | * Make LuaJIT memory profiler parser output more user-friendly (gh-5811).
>> |   Now the source line definition where the event occurs is much clearer:
>> |   only source file name and allocation-related line are presented,
> 
> Typo? s/are presented/is presented/
> 
>> |   function definition line number is omitted. Moreover, event-related
> 
> Typo: /function definition line number/the function definition line number/.
> 
>> |   statistics are indicated with units.
>> |   **Breaking change**: Line info of the line function definition is
>> |   saved in symbol info table by field `linedefined` now and field `name`
>> |   is renamed to `source` with the respect to Lua Debug API.
> 
> Typo: s/to Lua Debug/to the Lua Debug/
> 
>> 
> 
> <snipped>
> 
>>>>> -- 
>>>>> 2.31.0
>>>>> 
>>>> 
>>>> -- 
>>>> Best regards,
>>>> IM
>>> 
>>> -- 
>>> Best regards,
>>> Sergey Kaplun
>> 
>> -- 
>> Best regards,
>> IM
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 19497 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-29 15:41         ` Sergey Ostanevich via Tarantool-patches
@ 2021-03-29 15:56           ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 9+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-03-29 15:56 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

Hi, Sergos!

Thanks for the review!

On 29.03.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thank you for the patch and review!
> 
> Let me propose some technical fix:
> 
> long/path/to/function.lua: 3 events
> very/long/path/to/function.lua:	3 events
> 
> I think dump as above is not-so-readable for humans. A single-symbol fixup gives 
> 
> long/path/to/function.lua:	3 events
> very/long/path/to/function.lua:	3 events
> 
> +    print(string.format("%s:\t%d events\t+%d bytes\t-%d bytes",
> 
> The better solution would be to keep the longest name and append/prepend all of them
> with spaces, so that the following will not happen:

Yep, I agree. We should count the length of each section:
* filename
* number of events
* allocated bytes
* freed bytes

I suppose, we should create a new one "good first issue" to someone to
onboard in Lua and LuaJIT in particular. Thoughts?

> 
> long/path/to/funct.lua:	3 events
> very/long/path/to/funct.lua:	3 events
> 
> 
> All of these are too much ‘vkusovschina’, so you have my LGTM.
> 
> Sergos
> 
> 
> 
> > On 29 Mar 2021, at 16:00, Sergey Kaplun <skaplun@tarantool.org <mailto:skaplun@tarantool.org>> wrote:
> > 
> > Igor,
> > 
> > On 29.03.21, Igor Munkin wrote:
> >> Sergey,
> >> 
> >> Thanks for the fixes! I'll push it to the trunk as soon as Sergos gives
> >> his LGTM.
> >> 
> >> On 29.03.21, Sergey Kaplun wrote:
> >>> Igor,
> >>> 
> >>> Thanks for the review!
> >>> 
> >> 
> >> <snipped>
> >> 
> >>> 
> >>> Missed ChangeLog entry, feel free to change it at your pleasure:
> >> 
> >> Great, thanks! I propose the following wording:
> > 
> > Thanks! I like it, exept some typos:
> > 
> >> 
> >> | ##feature/luajit
> >> |
> >> | * Make LuaJIT memory profiler parser output more user-friendly (gh-5811).
> >> |   Now the source line definition where the event occurs is much clearer:
> >> |   only source file name and allocation-related line are presented,
> > 
> > Typo? s/are presented/is presented/
> > 
> >> |   function definition line number is omitted. Moreover, event-related
> > 
> > Typo: /function definition line number/the function definition line number/.
> > 
> >> |   statistics are indicated with units.
> >> |   **Breaking change**: Line info of the line function definition is
> >> |   saved in symbol info table by field `linedefined` now and field `name`
> >> |   is renamed to `source` with the respect to Lua Debug API.
> > 
> > Typo: s/to Lua Debug/to the Lua Debug/
> > 
> >> 
> > 
> > <snipped>
> > 
> >>>>> -- 
> >>>>> 2.31.0
> >>>>> 
> >>>> 
> >>>> -- 
> >>>> Best regards,
> >>>> IM
> >>> 
> >>> -- 
> >>> Best regards,
> >>> Sergey Kaplun
> >> 
> >> -- 
> >> Best regards,
> >> IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-29 13:00       ` Sergey Kaplun via Tarantool-patches
  2021-03-29 15:41         ` Sergey Ostanevich via Tarantool-patches
@ 2021-03-29 19:11         ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 9+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-03-29 19:11 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

On 29.03.21, Sergey Kaplun wrote:
> Igor,
> 

<snipped>

> 
> Thanks! I like it, exept some typos:
> 
> > 
> > | ##feature/luajit
> > |
> > | * Make LuaJIT memory profiler parser output more user-friendly (gh-5811).
> > |   Now the source line definition where the event occurs is much clearer:
> > |   only source file name and allocation-related line are presented,
> 
> Typo? s/are presented/is presented/

No, since both *are* presented.

> 
> > |   function definition line number is omitted. Moreover, event-related
> 
> Typo: /function definition line number/the function definition line number/.

Fixed.

> 
> > |   statistics are indicated with units.
> > |   **Breaking change**: Line info of the line function definition is
> > |   saved in symbol info table by field `linedefined` now and field `name`
> > |   is renamed to `source` with the respect to Lua Debug API.
> 
> Typo: s/to Lua Debug/to the Lua Debug/

Fixed.

> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly
  2021-03-26 16:48 [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly Sergey Kaplun via Tarantool-patches
  2021-03-26 20:33 ` Igor Munkin via Tarantool-patches
@ 2021-03-29 21:58 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 9+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-03-29 21:58 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

I've checked the series into tarantool branch in tarantool/luajit and
bumped a new version in master.

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.
> Also, these "magic numbers" 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 `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(-)
> 

<snipped>

> -- 
> 2.31.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-29 21:58 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-26 16:48 [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly Sergey Kaplun via Tarantool-patches
2021-03-26 20:33 ` Igor Munkin via Tarantool-patches
2021-03-29  9:46   ` Sergey Kaplun via Tarantool-patches
2021-03-29 11:16     ` Igor Munkin via Tarantool-patches
2021-03-29 13:00       ` Sergey Kaplun via Tarantool-patches
2021-03-29 15:41         ` Sergey Ostanevich via Tarantool-patches
2021-03-29 15:56           ` Sergey Kaplun via Tarantool-patches
2021-03-29 19:11         ` Igor Munkin via Tarantool-patches
2021-03-29 21:58 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox