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 BA60EF33B82; Tue, 11 Feb 2025 12:49:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BA60EF33B82 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739267357; bh=oawk8CTIE4F8qypmzWWHkZSk8B8FqrW+T/kuanio3v8=; 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=aRD5kaBRQzcIFHpBK4t3iahHWj0z/zrQ+hSeNsWf5A9OdLrgRxPTO4WWf30M6rNtC 4pNteZM+gN3i6V69HQvh8M1CKEJk/cWYcYSpGwrUI3cfHCQvNlTA0CSb0yM5R+9AuK VGZVD+hnDLL3im9xjwHcCV7h1KZ6uKfDrRLYmusc= Received: from send265.i.mail.ru (send265.i.mail.ru [95.163.59.104]) (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 7CDB5F33B81 for ; Tue, 11 Feb 2025 12:49:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7CDB5F33B81 Received: by exim-smtp-5c664d6544-pr6fl with esmtpa (envelope-from ) id 1thmtE-00000000Kdx-1huH; Tue, 11 Feb 2025 12:49:12 +0300 Content-Type: multipart/alternative; boundary="------------IfYPqz9T0W3VzzQqblNcIA78" Message-ID: Date: Tue, 11 Feb 2025 12:49:11 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org References: In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9D183B3A831A946F4FCDCA72B79F911A9EFB3220365B80E1C182A05F538085040A08B9FB00F6087A93DE06ABAFEAF67057AE0AA2F5F21EEA59D7D922F92EEE9494B90BBB9A9E916C9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE795530B80AF2ADB7BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637FB2D77E6174520AE8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BC81E538B0FFDC4325A67867EC8D24518BCE504EC4041A9FCC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9B5C8C57E37DE458BD9DD9810294C998ED8FC6C240DEA76428AA50765F7900637ECCE6E2F36009FC3D81D268191BDAD3DBD4B6F7A4D31EC0BE2F48590F00D11D6D81D268191BDAD3D78DA827A17800CE73A3D5CDF239AF116EC76A7562686271ED91E3A1F190DE8FD2E808ACE2090B5E14AD6D5ED66289B5259CC434672EE63711DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C321259270BBF67A2035872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A53345C72954E4C94E5002B1117B3ED696270758C2A48F04E3C638DF663A625AFA823CB91A9FED034534781492E4B8EEAD69BF13FED57427F1BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34C264B329661203DA7D2554EBA189853D1731AD92261BFB065D56A8117B64958F3A7664EB1833B8861D7E09C32AA3244CEA107E293107C2D277DD89D51EBB7742E7918E5B0791DCC8EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4p5EDZXZmhGkAlAPZP1yDQ== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F2588458222BC938DA2BE93C741D06F8E7BD5C494D19DFC030DBD979BE6CC51FD2D044A3645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 4/4] sysprof: fix a message with stop without run 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------IfYPqz9T0W3VzzQqblNcIA78 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey! thanks for valuable comments! Sergey On 10.02.2025 17:52, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. > > On 04.02.25, Sergey Bronnikov wrote: >> The function `misc.sysprof.stop()` reports that profiler is >> already running: >> >> | $ ./build/src/luajit -e 'print(misc.sysprof.stop())' > Minor: You may omit a ./build here, but this is a matter of taste, so > feel free to ignore. Removed 'build'. > >> | nil profiler is running already 22 >> >> The patch fixes that. > Unfortunatelly not: > > | $ git --no-pager log --oneline -n1 --no-decorate && ./src/luajit -e 'print(misc.sysprof.stop())' > | b49c5fac sysprof: fix a message with stop without run > | nil profiler is running already 22 > > See my comments below. > > Please add the follow-up to the tarantool/tarantool#781. Added. > >> --- >> src/lj_sysprof.c | 2 +- >> test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++-- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c >> index 88c7a41b..b76f503c 100644 >> --- a/src/lj_sysprof.c >> +++ b/src/lj_sysprof.c >> @@ -493,7 +493,7 @@ int lj_sysprof_stop(lua_State *L) >> global_State *g = sp->g; >> struct lj_wbuf *out = &sp->out; >> >> - if (SPS_IDLE == sp->state) >> + if (SPS_PROFILE != sp->state) > This check is correct. It checks that the profiler has no error or is > not running (i.e., that it is stopped). With the new check, a case with > an error during profiling will be masked. > > It looks like we should add this case in the `misc.sysprof.stop()` (it > uses `sysprof_error()` which handles start cases). Fixed. > >> return PROFILE_ERRRUN; >> else if (G(L) != g) >> return PROFILE_ERRUSE; >> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> index 68a4b72f..91f9ca5c 100644 >> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> @@ -10,7 +10,7 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({ >> ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"), >> }) >> >> -test:plan(33) >> +test:plan(34) >> >> jit.off() >> -- XXX: Run JIT tuning functions in a safe frame to avoid errors >> @@ -98,7 +98,8 @@ assert(res, err) >> >> -- Not running. >> res, err, errno = misc.sysprof.stop() >> -test:ok(res == nil and err, "res and error with not running") >> +test:is(res, nil, "res with not running") >> +test:ok(err:match("profiler is running already"), "error with not running") > I suppose there is bad copy-pasting here. It should be: 'profiler is not > running'. This is why the test passes. Fixed. > >> test:ok(type(errno) == "number", "errno with not running") >> >> -- Bad path. >> -- >> 2.34.1 >> --------------IfYPqz9T0W3VzzQqblNcIA78 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey!

thanks for valuable comments!

Sergey

On 10.02.2025 17:52, Sergey Kaplun via Tarantool-patches wrote:
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 04.02.25, Sergey Bronnikov wrote:

The function `misc.sysprof.stop()` reports that profiler is
already running:

| $ ./build/src/luajit -e 'print(misc.sysprof.stop())'

Minor: You may omit a ./build here, but this is a matter of taste, so
feel free to ignore.
Removed 'build'.


| nil     profiler is running already     22

The patch fixes that.

Unfortunatelly not:

| $ git --no-pager log --oneline -n1 --no-decorate && ./src/luajit -e 'print(misc.sysprof.stop())'
| b49c5fac sysprof: fix a message with stop without run
| nil     profiler is running already     22

See my comments below.

Please add the follow-up to the tarantool/tarantool#781.
Added.


---
 src/lj_sysprof.c                                             | 2 +-
 test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 88c7a41b..b76f503c 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -493,7 +493,7 @@ int lj_sysprof_stop(lua_State *L)
   global_State *g = sp->g;
   struct lj_wbuf *out = &sp->out;
 
-  if (SPS_IDLE == sp->state)
+  if (SPS_PROFILE != sp->state)

This check is correct. It checks that the profiler has no error or is
not running (i.e., that it is stopped). With the new check, a case with
an error during profiling will be masked.

It looks like we should add this case in the `misc.sysprof.stop()` (it
uses `sysprof_error()` which handles start cases).
Fixed.


     return PROFILE_ERRRUN;
   else if (G(L) != g)
     return PROFILE_ERRUSE;
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index 68a4b72f..91f9ca5c 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -10,7 +10,7 @@ local test = tap.test("misc-sysprof-lapi"):skipcond({
   ["Disabled due to #10803"] = os.getenv("LUAJIT_TEST_USE_VALGRIND"),
 })
 
-test:plan(33)
+test:plan(34)
 
 jit.off()
 -- XXX: Run JIT tuning functions in a safe frame to avoid errors
@@ -98,7 +98,8 @@ assert(res, err)
 
 -- Not running.
 res, err, errno = misc.sysprof.stop()
-test:ok(res == nil and err, "res and error with not running")
+test:is(res, nil, "res with not running")
+test:ok(err:match("profiler is running already"), "error with not running")

I suppose there is bad copy-pasting here. It should be: 'profiler is not
running'. This is why the test passes.
Fixed.


 test:ok(type(errno) == "number", "errno with not running")
 
 -- Bad path.
-- 
2.34.1



    

  
  


--------------IfYPqz9T0W3VzzQqblNcIA78--