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 314F4F32941; Tue, 11 Feb 2025 11:16:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 314F4F32941 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739261781; bh=R/POB6j5cKZakGFcrnv18+mCS7+o8Pp89zHRoXgdF8M=; 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=iCrzdFhq/gcI0UvMoE6zyN9crn6eJWUt1pPGxtNZfxg0xEhfQ3Ww8vX8bc/xU9jOw n4jtd9pYDj7/TxsOEbTDUF2cuPfwfmf2RnlD4FqfAH4JabsyzHP8joxcJQgFpRRpkl Lrd7sJacd1scLnMMsfu+lUahzgrmWosvgxWWQ28M= 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 2CB9CF32941 for ; Tue, 11 Feb 2025 11:16:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2CB9CF32941 Received: by exim-smtp-5c664d6544-rgt75 with esmtpa (envelope-from ) id 1thlRJ-000000002mC-3vyO; Tue, 11 Feb 2025 11:16:18 +0300 Content-Type: multipart/alternative; boundary="------------eRGzc2wxt4DJnQSqZgJ9WznG" Message-ID: Date: Tue, 11 Feb 2025 11:16:17 +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: <6b70bd32f156ff24015bd48c45a404eafacd468a.1738663201.git.sergeyb@tarantool.org> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD980EDB4062763125EF12BC0850CD4868D8D6D180C49CB07F600894C459B0CD1B92AA08F1F0F48B295F378A8CA21F699D6ED5645CAACD737571B6FEF39B83D50954B72E82432766FBE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE701173C01F417A2A6EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CDA089757FB31C668638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BA101095043D95C6A92BDAD9E2AC732A2C1119146ACB1859CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8989FD0BDF65E50FBF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2A336C65186350912D242C3BD2E3F4C64AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C39A1C9D3BA3303E89BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF17B107DEF921CE791DD303D21008E298D5E8D9A59859A8B6D082881546D9349175ECD9A6C639B01B78DA827A17800CE70BB89B22BF4660DC731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5BE10EA100E4034A35002B1117B3ED696F97A3BD87891EB9EE20DC3F561CE4150823CB91A9FED034534781492E4B8EEADF1165EFABA1780A6BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34BDCC71B3781C9C9559D1A220EF8DD1E186A7E7A7327E69923A41B2330887CC105993C0AB6D645CCC1D7E09C32AA3244CBBCF44B6513E65B477DD89D51EBB77420863B0E02C9927A7EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4p5EDZXZmhH/2O61IfjS/w== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F25884585C29915BCDB14525364022C9811FBA4E513A2EBBC810FC7CE1CEC4C7C2DDA68E645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/4] test: add descriptions to sysprof testcases 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. --------------eRGzc2wxt4DJnQSqZgJ9WznG Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey, thanks for review! On 10.02.2025 17:51, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > LGTM, with a bunch of nits below. > > On 04.02.25, Sergey Bronnikov wrote: >> The patch add descriptions to sysprof testcases to make TAP report > Typo: s/add/adds/ > Typo: s/TAP/the TAP/ Fixed both typos. >> more usable. >> --- >> .../profilers/misclib-sysprof-lapi.test.lua | 36 ++++++++++--------- >> 1 file changed, 20 insertions(+), 16 deletions(-) >> >> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> index b2c38c49..581fb7fd 100644 >> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> @@ -65,34 +65,35 @@ end >> >> -- Wrong profiling mode. >> local res, err, errno = misc.sysprof.start{ mode = "A" } >> -test:ok(res == nil and err:match("profiler misuse")) >> -test:ok(type(errno) == "number") >> +test:ok(res == nil and err:match("profiler misuse"), "res with no parameters") > Minor: s/res/result status/ Fixed. > Here and below. > Feel free to ignore. > > It is not the "no parameters", but rather the wrong profiler mode. Fixed. > >> +test:ok(type(errno) == "number", "errno with no parameters") > It is not the "no parameters", but rather the wrong profiler mode. > >> >> -- Already running. > > >> -- Not running. > > >> -- Bad path. >> res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH }) >> -test:ok(res == nil and err:match("No such file or directory")) >> -test:ok(type(errno) == "number") >> +test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path") > Nit: Line length is more than 80 symbols. Fixed and sent a separate patch that limits max len by 80 symbols. > >> +test:ok(type(errno) == "number", "errno with bad path") >> >> -- Bad interval. >> res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 } >> -test:ok(res == nil and err:match("profiler misuse")) >> -test:ok(type(errno) == "number") >> +test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval") > Nit: Line length is more than 80 symbols. Fixed. > >> +test:ok(type(errno) == "number", "errno with bad interval") >> >> -- DEFAULT MODE >> >> @@ -102,20 +103,23 @@ end >> >> local report = misc.sysprof.report() >> >> --- Check the profile is not empty >> -test:ok(report.samples > 0) >> +-- Check the profile is not empty. >> +test:ok(report.samples > 0, "number of samples is greater than 0") > I would also add the following: > "... than 0 for the default payload" Fixed. > >> -- There is a Lua function with FNEW bytecode in it. Hence there >> -- are only three possible sample types: >> -- * LFUNC -- Lua payload is sampled. >> -- * GC -- Lua GC machinery triggered in scope of FNEW bytecode >> -- is sampled. >> -- * INTERP -- VM is in a specific state when the sample is taken. >> -test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0) >> +test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0, >> + "total number of LFUNC, GC and INTERP samples is greater than 0") > I would also add the following: > "... than 0 for the default payload" Fixed. >> -- There is no fast functions and C function in default payload. >> -test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0) >> +test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0, >> + "total number of FFUNC and CFUNC samples is equal to 0") > I would also add the following: > "... to 0 for the default payload" Fixed. > >> -- Check all JIT-related VM states are not sampled. >> for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do >> - test:ok(report.vmstate[vmstate] == 0) >> + local msg = ("total number of VM state %s is equal to 0"):format(vmstate) > I would also add the following: > "... to 0 for the default payload" > > Minor: I would avoid the tmp variable here, since it is used only once > anyway. Fixed and moved out of loop. >> + test:ok(report.vmstate[vmstate] == 0, msg) >> end >> >> -- With very big interval. >> @@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then >> end >> >> report = misc.sysprof.report() >> -test:ok(report.samples == 0) >> +test:ok(report.samples == 0, "total number of samples is equal to 0") > I would also add the following: > "... to 0 for the too big sampling interval" Fixed. >> >> -- LEAF MODE >> check_mode("L", 100) >> -- >> 2.34.1 >> --------------eRGzc2wxt4DJnQSqZgJ9WznG Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey,

thanks for review!

On 10.02.2025 17:51, Sergey Kaplun via Tarantool-patches wrote:
Hi, Sergey!
Thanks for the patch!
LGTM, with a bunch of nits below.

On 04.02.25, Sergey Bronnikov wrote:

The patch add descriptions to sysprof testcases to make TAP report

Typo: s/add/adds/
Typo: s/TAP/the TAP/

Fixed both typos.



      

        
more usable.
---
 .../profilers/misclib-sysprof-lapi.test.lua   | 36 ++++++++++---------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index b2c38c49..581fb7fd 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -65,34 +65,35 @@ end
 
 -- Wrong profiling mode.
 local res, err, errno = misc.sysprof.start{ mode = "A" }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res with no parameters")


      

      
Minor: s/res/result status/

    

    Fixed.

    

      
Here and below.
Feel free to ignore.

It is not the "no parameters", but rather the wrong profiler mode.

    

    Fixed.

    

      



      

        
+test:ok(type(errno) == "number", "errno with no parameters")


      

      
It is not the "no parameters", but rather the wrong profiler mode.



      

        
 
 -- Already running.


      

      
<snipped>



      

        
 -- Not running.


      

      
<snipped>



      

        
 -- Bad path.
 res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH })
-test:ok(res == nil and err:match("No such file or directory"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("No such file or directory"), "res and error with bad path")


      

      
Nit: Line length is more than 80 symbols.

    

    Fixed and sent a separate patch that limits max len by 80 symbols.

    

      



      

        
+test:ok(type(errno) == "number", "errno with bad path")
 
 -- Bad interval.
 res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
-test:ok(res == nil and err:match("profiler misuse"))
-test:ok(type(errno) == "number")
+test:ok(res == nil and err:match("profiler misuse"), "res and error with bad interval")


      

      
Nit: Line length is more than 80 symbols.

    

    Fixed.

    

      



      

        
+test:ok(type(errno) == "number", "errno with bad interval")
 
 -- DEFAULT MODE
 
@@ -102,20 +103,23 @@ end
 
 local report = misc.sysprof.report()
 
--- Check the profile is not empty
-test:ok(report.samples > 0)
+-- Check the profile is not empty.
+test:ok(report.samples > 0, "number of samples is greater than 0")


      

      
I would also add the following:
"... than 0 for the default payload"

    

    Fixed.

    

      



      

        
 -- There is a Lua function with FNEW bytecode in it. Hence there
 -- are only three possible sample types:
 -- * LFUNC -- Lua payload is sampled.
 -- * GC -- Lua GC machinery triggered in scope of FNEW bytecode
 --   is sampled.
 -- * INTERP -- VM is in a specific state when the sample is taken.
-test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0)
+test:ok(report.vmstate.LFUNC + report.vmstate.GC + report.vmstate.INTERP > 0,
+        "total number of LFUNC, GC and INTERP samples is greater than 0")


      

      
I would also add the following:
"... than 0 for the default payload"


    

    Fixed.

    

      

      

        
 -- There is no fast functions and C function in default payload.
-test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0)
+test:ok(report.vmstate.FFUNC + report.vmstate.CFUNC == 0,
+        "total number of FFUNC and CFUNC samples is equal to 0")


      

      
I would also add the following:
"... to 0 for the default payload"

    

    Fixed.

    

      



      

        
 -- Check all JIT-related VM states are not sampled.
 for _, vmstate in pairs({ 'TRACE', 'RECORD', 'OPT', 'ASM', 'EXIT' }) do
-  test:ok(report.vmstate[vmstate] == 0)
+  local msg = ("total number of VM state %s is equal to 0"):format(vmstate)


      

      
I would also add the following:
"... to 0 for the default payload"

Minor: I would avoid the tmp variable here, since it is used only once
anyway.


    

    Fixed and moved out of loop.

    

      

      

        
+  test:ok(report.vmstate[vmstate] == 0, msg)
 end
 
 -- With very big interval.
@@ -124,7 +128,7 @@ if not pcall(generate_output, { mode = "D", interval = 1000 }) then
 end
 
 report = misc.sysprof.report()
-test:ok(report.samples == 0)
+test:ok(report.samples == 0, "total number of samples is equal to 0")


      

      
I would also add the following:
"... to 0 for the too big sampling interval"


    

    
Fixed.

    


    

    

      

      

        
 
 -- LEAF MODE
 check_mode("L", 100)
-- 
2.34.1



      

      

    

  
  


--------------eRGzc2wxt4DJnQSqZgJ9WznG--