Re: [Tarantool-patches] [PATCH luajit v3 4/4] sysprof: improve parser's memory footprint

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Max,

Thanks for the patch! Everything is OK in general, but please consider
my comments below.

On 31.07.23, Maxim Kokryashkin wrote:
> This patch reduces sysprof's parser memory footprint,
> by avoiding reading all callchains before collapsing them.
> Instead of it, parser merges stacks immediately after
> reading them and stores counts in a lua table.
> 
> The `collapse.lua` module is purged as a result of the
> patch, but it is left as a stub to keep the integrational
> testing intact. This stub should be removed in the next
> series.
> 
> Resolves tarantool/tarantool#8700
> ---
>  tools/CMakeLists.txt       |   4 ++
>  tools/sysprof.lua          |  21 +------
>  tools/sysprof/collapse.lua | 123 +-----------------------------------
>  tools/sysprof/parse.lua    | 126 ++++++++++++++++++++++++++-----------
>  4 files changed, 101 insertions(+), 173 deletions(-)
> 
> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index dd7ec6bd..1ae559ee 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt
> @@ -112,6 +112,8 @@ else()
>    add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
>      luajit-parse-sysprof
>      sysprof/parse.lua
> +    # FIXME: This line is not deleted only for the sake of integrational
> +    # testing. It should be deleted in the next series.

Minor: I'd rather left TODO instead of FIXME, but this is not a big
deal, so feel free to ignore.

>      sysprof/collapse.lua
>      sysprof.lua
>      utils/bufread.lua
> @@ -121,6 +123,8 @@ else()
>  
>    install(FILES
>        ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/parse.lua
> +      # FIXME: This line is not deleted only for the sake of integrational
> +      # testing. It should be deleted in the next series.

Ditto.

>        ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/collapse.lua
>      DESTINATION ${LUAJIT_DATAROOTDIR}/sysprof
>      PERMISSIONS

<snipped>

> diff --git a/tools/sysprof/collapse.lua b/tools/sysprof/collapse.lua
> index ac5269ea..9e815e0d 100755
> --- a/tools/sysprof/collapse.lua
> +++ b/tools/sysprof/collapse.lua
> @@ -1,120 +1,3 @@

<snipped>

> +-- FIXME: This line is not deleted only for the sake of
> +-- integrational testing. It should be deleted in the
> +-- next series.

Honestly, I would literally "purge" collapse.lua the following way:
replace all of its contents with the only assert call to check that
nobody will use it. However, if it breaks Tarantool, I agree to left it
intact until the file is removed completely from the source tree.

> diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua
> index 5b52f104..19add4f3 100755
> --- a/tools/sysprof/parse.lua
> +++ b/tools/sysprof/parse.lua

<snipped>

> @@ -143,18 +153,63 @@ local function parse_symtab(reader, symbols, vmstate)

<snipped>

>  
> +local function insert_lua_callchain(chain, lua)
> +  local ins_cnt = 0
> +  local name_lua
> +  for _, fr in ipairs(lua.callchain) do
> +    ins_cnt = ins_cnt + 1
> +    if fr.type == FRAME.CFUNC then
> +      -- C function encountered, the next chunk
> +      -- of frames is located on the C stack.
> +      break
> +    end
> +    name_lua = fr.name
> +
> +    if fr.type == FRAME.LFUNC
> +    and lua.trace.traceno ~= nil
> +    and lua.trace.addr == fr.addr
> +    and lua.trace.line == fr.line
> +    then
> +            name_lua = lua.trace.name
> +    end

Something bad with indentation. I guess there should be something
similar to this:
| if fr.type == FRAME.LFUNC
|   and lua.trace.traceno ~= nil
|   and lua.trace.addr == fr.addr
|   and lua.trace.line == fr.line
| then
|   name_lua = lua.trace.name
| end

> +
> +    table.insert(chain, name_lua)
> +  end
> +  table.remove(lua.callchain, ins_cnt)
> +end

<snipped>

> @@ -171,8 +226,9 @@ local function parse_event(reader, events, symbols)
>    event.lua.vmstate = vmstate
>  
>    event_parsers[vmstate](reader, event, symbols)
> -
> -  table.insert(events, event)
> +  local callchain = merge(event)
> +  local cur_cnt = events[callchain]
> +  events[callchain] = (cur_cnt or 0) + 1

Minor: The following line looks better (IMHO), but feel free to ignore.
| events[callchain] = (events[callchain] or 0) + 1

>    return true
>  end
>  
> -- 
> 2.41.0
> 

-- 
Best regards,
IM