Re: [Tarantool-patches] [PATCH v2 luajit 03/41] perf: introduce bench module

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

Hi, Sergey!
Thanks for the comments!
Please see my answers below.

On 15.01.26, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! LGTM with minor comments.
> 
> Sergey
> 
> On 12/26/25 12:17, Sergey Kaplun wrote:

<snipped>

> > ---
> >   perf/utils/bench.lua | 511 +++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 511 insertions(+)
> >   create mode 100644 perf/utils/bench.lua
> >
> > diff --git a/perf/utils/bench.lua b/perf/utils/bench.lua
> > new file mode 100644
> > index 00000000..09a5c41a
> > --- /dev/null
> > +++ b/perf/utils/bench.lua

<snipped>

> > +
> > +-- Process the options and update the benchmark context.
> > +local function argparse(arg, name)
> > +  local ctx = {name = name}
> 
> You format test output and column with test name has a fixed length,
> 
> I would check a test name for max len:
> 
> diff --git a/perf/utils/bench.lua b/perf/utils/bench.lua
> index 09a5c41a..f3557347 100644
> --- a/perf/utils/bench.lua
> +++ b/perf/utils/bench.lua
> @@ -190,8 +190,11 @@ local function parse_opt(ctx, arg, a, n)
>     end
>   end
> 
> +local MAX_TEST_NAME_LEN = 37
> +
>   -- Process the options and update the benchmark context.
>   local function argparse(arg, name)
> +  assert(#name > MAX_TEST_NAME_LEN)

It is not the required length, but more like an alignment
restriction.

| $ luajit -e 'print(("%-3s %d"):format("aaaaa", 1))'
| aaaaa 1
| $ luajit -e 'print(("%-3s %d"):format("aa", 1))'
| aa  1

>     local ctx = {name = name}
>     local n = 1
>     while n <= #arg do
> @@ -212,8 +215,8 @@ end
>   local function format_console_header()
>     -- Use a similar format to the Google Benchmark, except for the
>     -- fixed benchmark name length.
> -  local header = format('%-37s %12s %15s %13s %-28s\n',
> -    'Benchmark', 'Time', 'CPU', 'Iterations', 'UserCounters...'
> +  local header = format('%-%ds %12s %15s %13s %-28s\n',
> +    MAX_TEST_NAME_LEN, 'Benchmark', 'Time', 'CPU', 'Iterations', 
> 'UserCounters...'
>     )
>     local border = rep('-', #header - 1) .. '\n'
>     return border .. header .. border
> @@ -226,7 +229,7 @@ local COLORS = {
>   }
> 
>   local function format_name(ctx, name)
> -  name = format('%-37s ', name)
> +  name = format('%-%ds ', MAX_TEST_NAME_LEN, name)
>     if ctx.color then
>        name = format(COLORS.GREEN, name)
>     end
> 
> Feel free to ignore.

This requires to format the format string, so it looks not very
readable: `format(format("%%%-%ds "), name)`
Ignoring.

> 

<snipped>

> > +
> > +local COLORS = {
> > +  GREEN = '\027[32m%s\027[m',
> > +  YELLOW = '\027[33m%s\027[m',
> > +  CYAN = '\027[36m%s\027[m',
> > +}
> > +
> 
> Minor: we can sort it alphabetically:
> 
> 
>   local COLORS = {
> +  CYAN = '\027[36m%s\027[m',
>     GREEN = '\027[32m%s\027[m',
>     YELLOW = '\027[33m%s\027[m',
> -  CYAN = '\027[36m%s\027[m',
>   }

I prefer to sort in the code numbers order (as it is usually done in any
listings).

> 
> 

<snipped>

> > +  -- Reset the GC to the defaults.
> > +  collectgarbage('setstepmul', 200)
> > +  collectgarbage('setpause', 200)
> We don't change these parameters in bench.lua, why should we reset to 
> defaults?

In the case if user changes them during the benchmark.

> > +
> > +  -- Collect all garbage at the end. Twice to be sure that all
> > +  -- finalizers are run.
> > +  collectgarbage()
> > +  collectgarbage()
> > +end
> > +

<snipped>

> > +      -- Iterations are determined dinamycally, adjusting to fit
> typo: s/dinamycally/dynamycally/

Fixed, thanks!

===================================================================
diff --git a/perf/utils/bench.lua b/perf/utils/bench.lua
index 09a5c41a..0f1b2898 100644
--- a/perf/utils/bench.lua
+++ b/perf/utils/bench.lua
@@ -452,7 +452,7 @@ local function run_benches(ctx)
       delta_real = clock.realtime() - start_real
       delta_cpu  = clock.process_cputime() - start_cpu
     else
-      -- Iterations are determined dinamycally, adjusting to fit
+      -- Iterations are determined dynamycally, adjusting to fit
       -- the minimum time to run for the benchmark.
       local min_time = bench.min_time or ctx.min_time
       local next_iterations = 1
===================================================================

Branch is force-pushed.

> > +      -- the minimum time to run for the benchmark.
> > +      local min_time = bench.min_time or ctx.min_time
> > +      local next_iterations = 1

<snipped>

-- 
Best regards,
Sergey Kaplun