Re: [Tarantool-patches] [PATCH luajit] tools: make memprof parser output user-friendly

[Sergey Ostanevich via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- 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 --]