Re: [Tarantool-patches] [PATCH luajit] perf: add helper for benchmark results comparison

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

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 31.08.26, Sergey Bronnikov wrote:
> Hello,
> 
> Thanks for the patch! Please see my comments.
> 
> On 8/26/26 17:34, Sergey Kaplun wrote:
> > This patch adds the helper, which is similar to the Google-benchmark
> > compare.py script [1]. Nevertheless, it has the following semantic
> > changes:
> >
> > * compare.py compares the absolute time of the benchmark, which may lead
> >    to confusion when the time is autotuned to be sure that the benchmark
> >    runs the minimum required time amount. Hence, this script compares the
> >    items_per_second metric to check the performance difference even for
> >    the same absolute times of the benchmarks.
> > * compare.py compares only 2 files, while this script allows comparing
> >    directories filled with the same benchmarks (checks intersection with
> >    warning of unmatched benchmarks), which allows to see the full
> >    statistic of the patch for the suite.
> > * This helper doesn't allow running the benchmarks to compare. It
> >    proceeds only with the given results.
> > * There is no support for U test.
> > * --alpha flag allows setting the relative difference threshold from
> >    which results are considered as changed, instead of the p-value for
> >    U test.
> > * The geomean of the benchmarks can be hidden by the flag
> >    --hide_aggregates, -i.
> > * The output may be filtered to contain only changed benchmarks by the
> >    option --changes_only, -c.
> 
> In fact, all these reasons seem far-fetched. compere.py is just a 
> standalone script;
> 
> we can bring it as is and modify it to suit your needs, as we did with 
> performance tests.

Of course we can bring the 2k Python script with the corresponding
dependencies, but for what?
This is the repository about the Lua language, and it seems hypocritical
to support the Lua language if even our only team doesn't want to use
it.

> 
> The commit doesn't explain why you need to write something from scratch, 
> specifically in Lua.

It is a small patch utility patch. I don't see a problem here.

> 
> The patch brings the script, but it's not used anywhere. Why do we need 
> it in the repository?

It is the script for local console use. It is used to compare local
benchmark runs, as I mentioned in the description. For example, during
the review of the future patches with performance improvements like for
the commit [1].

> 
> The script reinvents argparse helpers, which we already wrote in 
> perf/utils/bench.lua. Why?

Since it is used as a script from the command line, it is done to avoid
using LUA_PATH, so I prefer not to use argparse as a separate module.

> 
> In your commit, you describe what you're missing from the script compare.py,
> 
> but where did all these requirements come from? If you have any plans in 
> mind about
> 
> how and what's missing for performance testing, please share them.
> 
> Otherwise, it just looks like a personal need.

It is common sense about what you want from the comparison script.
You may share your own thoughts and suggestions.

<snipped>

[1]: https://github.com/LuaJIT/LuaJIT/commit/14d8a7a27dc8c626ab9e7c7e9e50b6df6def4f03

-- 
Best regards,
Sergey Kaplun