Re: [Tarantool-patches] [PATCH v1 luajit 5/5] test: rewrite misclib-sysprof-capi test in C

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4742 bytes --]


Hi!
Thanks for the patch!
Please consider my comments below.
 
  
>Среда, 15 марта 2023, 19:14 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>This patch rewrites the aforementioned test with usage libtest recently
Typo: s/with usage libtest/with the usage of libtest/
>introduced. The approach is similar to the previous patch. `c_payload()`
>function to profile now is set up globally on script loading. The
Typo: s/now is set up/is now set up/
>payload for enabled and disabled JIT is the same. As far as for sysprof
>testing is necessary to replace backtrace with libunwind first, the
>payload tests are marked with `todo()`.
>
>Nevertheless, glibc `assert()` still necessary to check correctness of
>profile writer, so it is still used there.
Typo: s/to check correctness of profile/to check the correctness of the profile/
>
>Relates to tarantool/tarantool#781
>Part of tarantool/tarantool#7900
>---
> .../misclib-sysprof-capi-script.lua | 35 ++
> .../misclib-sysprof-capi.test.c | 317 ++++++++++++++++++
> test/tarantool-tests/CMakeLists.txt | 1 -
> .../misclib-sysprof-capi.test.lua | 54 ---
> .../misclib-sysprof-capi/CMakeLists.txt | 1 -
> .../misclib-sysprof-capi/testsysprof.c | 260 --------------
> 6 files changed, 352 insertions(+), 316 deletions(-)
> create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi-script.lua
> create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi.test.c
> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi.test.lua
> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt
> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
>
>diff --git a/test/tarantool-c-tests/misclib-sysprof-capi-script.lua b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua
>new file mode 100644
>index 00000000..dd8387db
>--- /dev/null
>+++ b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua
>@@ -0,0 +1,35 @@
>+local M = {}
>+
>+-- luacheck: no global
>+assert(c_payload, 'c_payload global function should be set via script loader')
>+
>+local function lua_payload(n)
>+ if n <= 1 then
>+ return n
>+ end
>+ return lua_payload(n - 1) + lua_payload(n - 2)
>+end
>+
>+local function payload()
>+ local n_iterations = 500000
>+
>+ local co = coroutine.create(function()
>+ for i = 1, n_iterations do
>+ if i % 2 == 0 then
>+ c_payload(10)
>+ else
>+ lua_payload(10)
>+ end
>+ coroutine.yield()
>+ end
>+ end)
>+
>+ for _ = 1, n_iterations do
>+ coroutine.resume(co)
>+ end
>+end
>+
>+M.profile_func_jiton = payload
>+M.profile_func_jitoff = payload
>+
>+return M
>diff --git a/test/tarantool-c-tests/misclib-sysprof-capi.test.c b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
>new file mode 100644
>index 00000000..4c54877e
>--- /dev/null
>+++ b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
>@@ -0,0 +1,317 @@
>+#include "lauxlib.h"
>+#include "lmisclib.h"
>+#include "lua.h"
>+#include "luajit.h"
>+
>+#include "test.h"
>+#include "utils.h"
>+
>+#include <errno.h>
>+#include <fcntl.h>
>+#include <stdlib.h>
>+#include <unistd.h>
>+
>+/* XXX: Still need normal assert inside writer functions. */
>+#undef NDEBUG
>+#include <assert.h>
>+
>+/* Need for skipcond for OS and ARCH. */
>+#include "lj_arch.h"
Side note: I don’t like the approach with private headers, but
I couldn’t find any better way to check that. Maybe it is a good
idea to implement a public C API function to get the information
about OS and ARCH, since it is a really common to check them? 
 
<snipped>
>
>diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>index b4ce39d3..b1c7207f 100644
>--- a/test/tarantool-tests/CMakeLists.txt
>+++ b/test/tarantool-tests/CMakeLists.txt
<snipped>
> 
> # The part of the memory profiler toolchain is located in tools
> # directory, jit, profiler, and bytecode toolchains are located
>diff --git a/test/tarantool-tests/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua
>deleted file mode 100644
>index 4395bce3..00000000
>--- a/test/tarantool-tests/misclib-sysprof-capi.test.lua
>+++ /dev/null
<snipped>
>diff --git a/test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt b/test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt
>deleted file mode 100644
>index d9fb1a1a..00000000
>--- a/test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt
>+++ /dev/null
>@@ -1 +0,0 @@
<snipped>
>diff --git a/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c b/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
>deleted file mode 100644
>index d7a3e355..00000000
>--- a/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
>+++ /dev/null
>@@ -1,260 +0,0 @@
<snipped>
---
Best regards,
Maxim Kokryashkin
 

[-- Attachment #2: Type: text/html, Size: 6748 bytes --]