From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: "Sergey Kaplun" <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v1 luajit 5/5] test: rewrite misclib-sysprof-capi test in C Date: Mon, 20 Mar 2023 19:24:28 +0300 [thread overview] Message-ID: <1679329468.9486210@f748.i.mail.ru> (raw) In-Reply-To: <251e7cacc074df4ebae5efcddc12665a2f8ffaa4.1678895861.git.skaplun@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 --]
next prev parent reply other threads:[~2023-03-20 16:24 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-03-15 16:11 [Tarantool-patches] [PATCH v1 luajit 0/5] reworking C tests Sergey Kaplun via Tarantool-patches 2023-03-15 16:11 ` [Tarantool-patches] [PATCH v1 luajit 1/5] test: fix setting of {DY}LD_LIBRARY_PATH variables Sergey Kaplun via Tarantool-patches 2023-03-20 13:54 ` Maxim Kokryashkin via Tarantool-patches 2023-03-15 16:11 ` [Tarantool-patches] [PATCH v1 luajit 2/5] test: introduce module for C tests Sergey Kaplun via Tarantool-patches 2023-03-20 15:17 ` Maxim Kokryashkin via Tarantool-patches 2023-03-15 16:11 ` [Tarantool-patches] [PATCH v1 luajit 3/5] test: introduce utils.h helper " Sergey Kaplun via Tarantool-patches 2023-03-20 15:21 ` Maxim Kokryashkin via Tarantool-patches 2023-03-15 16:11 ` [Tarantool-patches] [PATCH v1 luajit 4/5] test: rewrite misclib-getmetrics-capi test in C Sergey Kaplun via Tarantool-patches 2023-03-22 0:07 ` Maxim Kokryashkin via Tarantool-patches 2023-03-15 16:11 ` [Tarantool-patches] [PATCH v1 luajit 5/5] test: rewrite misclib-sysprof-capi " Sergey Kaplun via Tarantool-patches 2023-03-20 16:24 ` Maxim Kokryashkin via Tarantool-patches [this message] 2023-03-20 13:50 ` [Tarantool-patches] [PATCH v1 luajit 0/5] reworking C tests Maxim Kokryashkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1679329468.9486210@f748.i.mail.ru \ --to=tarantool-patches@dev.tarantool.org \ --cc=m.kokryashkin@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v1 luajit 5/5] test: rewrite misclib-sysprof-capi test in C' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox