Re: [Tarantool-patches] [PATCH v1 luajit 37/41] perf: add CMake infrastructure

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

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

On 18.11.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! See my comments.
> 
> Sergey
> 
> On 10/24/25 14:00, Sergey Kaplun wrote:

<snipped>

> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index c0da4362..73f46835 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -464,6 +464,17 @@ if(LUAJIT_USE_TEST)
> >   endif()
> >   add_subdirectory(test)
> >   
> > +# --- Benchmarks source tree ---------------------------------------------------
> > +
> > +# The option to enable performance tests for the LuaJIT.
> > +# Disabled by default, since commonly it is used only by LuaJIT
> > +# developers and run in the CI with the specially set-up machine.
> > +option(LUAJIT_ENABLE_PERF "Generate <perf> target" OFF)
> > +
> > +if(LUAJIT_ENABLE_PERF)
> 
> option name confuses a bit due to `perf` utility.
> 
> I would rename to something like "LUAJIT_ENABLE_PERF_TESTS".

OTOH, it matches with the directory name which is the same as in
Tarantool.

LUAJIT_USE_PERFTOOLS is used for the perftools support in JIT engine.

> 
> Feel free to ignore.

Ignoring.

> 
> > +  add_subdirectory(perf)
> > +endif()
> > +
> >   # --- Misc rules ---------------------------------------------------------------
> >   
> >   # XXX: Implement <uninstall> target using the following recipe:
> > diff --git a/perf/CMakeLists.txt b/perf/CMakeLists.txt
> > new file mode 100644
> > index 00000000..cc3c312f
> > --- /dev/null
> > +++ b/perf/CMakeLists.txt
> > @@ -0,0 +1,99 @@
> > +# Running various bench suites against LuaJIT.
> > +
> > +include(MakeLuaPath)
> > +
> > +if(CMAKE_BUILD_TYPE STREQUAL "Debug")
> > +  message(WARNING "LuaJIT and perf tests are built in the Debug mode."
> 
> s/./. /
> 
> missed whitespace after dot

Fixed, thanks!
===================================================================
diff --git a/perf/CMakeLists.txt b/perf/CMakeLists.txt
index c315597f..5a5cf777 100644
--- a/perf/CMakeLists.txt
+++ b/perf/CMakeLists.txt
@@ -3,7 +3,7 @@
 include(MakeLuaPath)
 
 if(CMAKE_BUILD_TYPE STREQUAL "Debug")
-  message(WARNING "LuaJIT and perf tests are built in the Debug mode."
+  message(WARNING "LuaJIT and perf tests are built in the Debug mode. "
                   "Timings may be affected.")
 endif()
 
===================================================================

> 
> > +                  "Timings may be affected.")
> > +endif()
> > +

<snipped>

> > +macro(AddBenchTarget perf_suite)
> > +  file(MAKE_DIRECTORY "${PERF_OUTPUT_DIR}/${perf_suite}/")
> > +  message(STATUS "Add perf suite ${perf_suite}")
> > +  add_custom_target(${perf_suite})
> > +  add_custom_target(${perf_suite}-console
> > +    COMMAND ${CMAKE_CTEST_COMMAND}
> > +      -L ${perf_suite}
> > +      --parallel 1
> > +      --verbose
> > +      --output-on-failure
> > +      --no-tests=error
> may be --schedule-random, --timeout XXX (default timeout is 10000000)?

I don't want to add the `--schedule-random`. It is better to have the
deterministic execution order.
There is no need for the strict timeout anyway. We can't predict the
behaviour of the benchmark. The default timeout on CI runners is OK.
The user may determine the timeout locally via the `CTEST_TEST_TIMEOUT`
environment variable.

> > +  )
> > +  add_dependencies(${perf_suite}-console luajit-main)
> > +endmacro()
> > +

<snipped>

> > +  set(BENCH_FLAGS
> > +    "--benchmark_out_format=json --benchmark_out=${bench_out_file}"
> > +  )
> > +  set(bench_command_flags ${bench_command} ${BENCH_FLAGS})
> > +  separate_arguments(bench_command_separated UNIX_COMMAND ${bench_command})
> > +  add_custom_command(
> > +    COMMAND ${CMAKE_COMMAND} -E env
> > +      LUA_PATH="${LUA_PATH}"
> > +      LUA_CPATH="${LUA_CPATH}"
> > +        ${bench_command_separated}
> > +          --benchmark_out_format=json
> > +          --benchmark_out="${bench_out_file}"
> previous two lines can be replaced with ${BENCH_FLAGS}, right?

No, this brokes the CMake generation, IINM.

> > +    OUTPUT ${bench_out_file}
> > +    DEPENDS luajit-main
> > +    COMMENT
> > +      "Running benchmark ${bench_title} saving results in ${bench_out_file}."
> > +  )
> > +  add_custom_target(${bench_name} DEPENDS ${bench_out_file})
> > +  add_dependencies(${perf_suite} ${bench_name})
> > +
> > +  # Report in the console.
> > +  add_test(NAME ${bench_title}
> > +    COMMAND sh -c "${bench_command}"
> > +  )
> > +  set_tests_properties(${bench_title} PROPERTIES
> > +    ENVIRONMENT "LUA_PATH=${LUA_PATH}"
> > +    LABELS ${perf_suite}
> > +    DEPENDS luajit-main
> > +  )
> > +  unset(input_file)
> > +endmacro()
> > +
> > +add_subdirectory(LuaJIT-benches)
> > +
> > +add_custom_target(${PROJECT_NAME}-perf
> > +  DEPENDS LuaJIT-benches
> missed a COMMENT field

Why do we need it?
There is no such field in the tests target, for the example.

> > +)
> > +
> > +add_custom_target(${PROJECT_NAME}-perf-console
> > +  DEPENDS LuaJIT-benches-console
> missed a COMMENT field

Why do we need it?
The output is so huge anyway that this field will not be visible.

> > +)
> > diff --git a/perf/LuaJIT-benches/CMakeLists.txt b/perf/LuaJIT-benches/CMakeLists.txt
> > new file mode 100644
> > index 00000000..d9909f36
> > --- /dev/null
> > +++ b/perf/LuaJIT-benches/CMakeLists.txt
> > @@ -0,0 +1,52 @@
> > +set(PERF_SUITE_NAME LuaJIT-benches)
> > +set(LUA_BENCH_SUFFIX .lua)
> it is not a bench-specific suffix. May be LUA_SUFFIX?

This is just for unified naming like it is done in our tests.

> > +
> > +AddBenchTarget(${PERF_SUITE_NAME})
> > +
> > +# Input for the k-nucleotide and revcomp benchmarks.
> > +set(FASTA_NAME ${CMAKE_CURRENT_BINARY_DIR}/FASTA_5000000)
> > +add_custom_target(FASTA_5000000
> > +  COMMAND ${LUAJIT_BINARY}
> > +    ${CMAKE_CURRENT_SOURCE_DIR}/libs/fasta.lua 5000000 > ${FASTA_NAME}
> 
> FASTA_5000000 is a plain text file. I propose to add extension .txt for 
> its full name and
> 
> probably postfix "_autogenerated". Like we do this for SUMCOL_5000 and 
> SUMCOL_1.
> 

The FASTA_\d* is the name used in the PARAMS file and
<TEST_md5sum.txt>. I prefer not to change it to avoid confusion.

> > +  OUTPUT ${FASTA_NAME}
> > +  DEPENDS luajit-main
> > +  COMMENT "Generate ${FASTA_NAME}."
> > +)
> > +
> > +make_lua_path(LUA_PATH
> > +  PATHS
> > +    ${LUA_PATH_BENCH_BASE}
> > +    ${CMAKE_CURRENT_SOURCE_DIR}/libs/?.lua
> > +)
> > +
> > +# Input for the <sum-file.lua> benchmark.
> > +set(SUM_NAME ${CMAKE_CURRENT_BINARY_DIR}/SUMCOL_5000.txt)
> > +# Remove possibly existing file.
> > +file(REMOVE ${SUM_NAME})
> 
> Why do we need generate file after every cmake configuration?
> 
> I propose to skip generation if file already exist or regenerate if 
> SHA256 is not the same.

I'm not sure that this is good idea, since possible the file may changed
accidentally or replaced by the mistake. The generation takes to small
amount of time to be crucial.

> 
> > +

<snipped>

-- 
Best regards,
Sergey Kaplun