From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 2707116ADBAE; Fri, 26 Dec 2025 11:40:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2707116ADBAE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1766738437; bh=iyl/XzdaF6Abv1DLbJjDLln5Nh6SkLgnkVIgrwrRp3E=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=cXJTkvINh5WWsXwV/zho4QOovBBCu8h1Nn+HBlF2xy3Lynm03WboXwDkG7c7Dkpst g92E8KW0wFz356DX63RTuTYKOEQUVI0Xk/qhMDgbuieJqf2HeiygKeWFPbVGvbH+yR zwZOdNofaXIrpZJKy6KAxt8fgNnM+h0hlbulAxok= Received: from send81.i.mail.ru (send81.i.mail.ru [89.221.237.176]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7B0AA16ADBA2 for ; Fri, 26 Dec 2025 11:40:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7B0AA16ADBA2 Received: by exim-smtp-7b4fb89df9-hr6bm with esmtpa (envelope-from ) id 1vZ3ND-00000000EmE-1nsL; Fri, 26 Dec 2025 11:40:35 +0300 Date: Fri, 26 Dec 2025 11:40:33 +0300 To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Message-ID: References: <70e9131c5cfc5c44822dd73ecb010d13dbd370c4.1761301736.git.skaplun@tarantool.org> <1e32e858-f823-453c-87fc-2bad7e0f6980@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1e32e858-f823-453c-87fc-2bad7e0f6980@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD979975AF0D777FEBD136B1DE29D0A14187F3E33C5CB109B4B182A05F538085040BBFDA0D3FE8268503DE06ABAFEAF6705F19B2620386B664495925BE4019C9994C9EA2F84C7A79461 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CEF52BA550D6CAADEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB55337566657B88B02DF8C8697DFA230F7BE0C5C2002D17363E8D33EA3DC9F5C75F4FCABD389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6957A4DEDD2346B42CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB86D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE79E9721B410A3B6ED731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A57D5A4838E5CBF8C35002B1117B3ED696F9740531AB2B3C0CA9DAB4B68AE4D22F823CB91A9FED034534781492E4B8EEADA3A806F356AF31D6 X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE1918E10F71CB4DF9F96AB70F9BE574AE9C625B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D65938B12FCD088FBF95DC39464F7481FEC22A00BC38CFD7A17FB1275CA3F638DE16CF5A488EE1F15C57B8341EE9D5BE9A0A2EE738555C890692A315B7EF8C891FF2EB6C929920121E058CD93680B12512CF4C41F94D744909CECFA6C6B0C050A61A8CAF69B82BA93681CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVdtTL5f5BIXbWkQlOV7IMX8= X-Mailru-Sender: 689FA8AB762F7393DDD5FD59B456EAD24DFD453837765E9D14E1DC80C168CD16FE8E31823C383DC9E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 luajit 37/41] perf: add CMake infrastructure X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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: > > 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 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 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() > > + > > +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() > > + > > + 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 . 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 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. > > > + -- Best regards, Sergey Kaplun