

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