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 528C1125D575; Thu, 27 Feb 2025 16:07:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 528C1125D575 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1740661638; bh=MauAEhah0Dq98AqY9q9Ssxc6i0nptVQeQ8mnEXJiJPw=; 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=uZ/Sj/bFP5NSQmY5+jw5hF0zbTpJFehjCTS+L2Jp6QpPkBdEpPw9KGIr1a//jC4Kq R9PXCpUM6HyY4Mpu8VUUTSS3UwzpsWr0+xTDEFbp/x7RqNg8MxDfChMbsrTnDcV1NX +/v2Mv8DTApA90EM/iq7mI2af4V5FiNdV0+UM0Qo= Received: from send218.i.mail.ru (send218.i.mail.ru [95.163.59.57]) (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 974A4125D575 for ; Thu, 27 Feb 2025 16:07:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 974A4125D575 Received: by exim-smtp-6558d74cf4-qj58m with esmtpa (envelope-from ) id 1tndbf-0000000030w-2wdn; Thu, 27 Feb 2025 16:07:16 +0300 Content-Type: multipart/alternative; boundary="------------2UwRLOI0wySnShAHRQpQq7NG" Message-ID: Date: Thu, 27 Feb 2025 16:07:15 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org References: <80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527.1739362814.git.sergeyb@tarantool.org> <93178b00-f3e0-4145-932f-bd273935a477@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9F0D6B4A195F952737BCED6131FB3BAEF210934F55CCE698A182A05F538085040C2F2FC51FCC3228A3DE06ABAFEAF6705F5A6F2897397623211A91CA39DD0A6E795102AAA86CA9C0B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77BF46084C0059042EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637FE9EFE935CD7C6AE8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B22850F060A12C3E9F2E070BE324C7D3C41899E873420F2B3CF6B57BC7E64490618DEB871D839B73339E8FC8737B5C224952D31B9D28593E51CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C0A29E2F051442AF778941B15DA834481F9449624AB7ADAF37BA3038C0950A5D3613377AFFFEAFD269176DF2183F8FC7C0CAB4775CB929E3BB7B076A6E789B0E97A8DF7F3B2552694AD5FFEEA1DED7F25D49FD398EE364050F9647ADFADE5905B1F41620B44FB51B7DB3661434B16C20ACC84D3B47A649675FE827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B779389CF6F126FEC75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A517D429B14D9E81935002B1117B3ED6960384030F5E23D66654BB1175C6E7DD94823CB91A9FED034534781492E4B8EEAD17AEC49845D0B908 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34A5970F8A4685C8ABAFB1C3C5CB19BC9A14428970870917E5BD86EBE76E4A722D182552090C11A5FB1D7E09C32AA3244CFE142A574CB7360577DD89D51EBB7742F5BF11CB93A8618CEA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQQG/FugD0/CKdTPH2SN9jc= X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D61401A84D78C75C0F272A6D5EE0DB6E1EC8D7E18177B8544B1570152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file. 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. --------------2UwRLOI0wySnShAHRQpQq7NG Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey, thanks for review! Changes applied and force-pushed. Sergey On 26.02.2025 14:58, Sergey Kaplun wrote: > Hi, Sergey, > Thanks for the fixes! > See my answers below. > > On 25.02.25, Sergey Bronnikov wrote: >> Hi, Sergey, >> >> thanks for review! Updates applied and force-pushed. >> >> Sergey >> >> On 13.02.2025 18:31, Sergey Kaplun via Tarantool-patches wrote: >>> Hi, Sergey! >>> Thanks for the fixes and clarification! >>> Please consider by answers below. >>> >>> On 12.02.25, Sergey Bronnikov wrote: > > >>>> +local script = utils.exec.makecmd(arg) >>>> +-- Execute a Lua script with start and stop LuaJIT profiler, >>>> +-- it is expected no samples found by profiler. The script's >>>> +-- output is suppressed, it is not interested. >>>> +local _ = script(p_filename, p_postfix) >>>> # > I don't get it. Why do we need the separate script for it? >>>> # prof_finish() is executed first time on calling `jit.p.stop()` >>>> # >>>> # and executed second time when GC finalizer is calling [1] and therefore >>>> # message "'[No samples collected]'" appears >>>> # >>>> # two times in the output file. >>>> # >>>> # 1.https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290 >>>> # >>>> # We can inline script.lua to a test body and call garbage collector manually, >>>> # but I propose to leave it as is because  executing as a separate process is >>>> # more real use case. >>> Now it is clear to me, thanks! >>> >>> This is another face of the bug that was fixed by this commit. >>> Nice catch! >>> >>> In the original issue, automated parsing shows empty file content. >>> I suppose it was done with something like the following: >>> >>> | LUA_PATH="src/?.lua;;" src/luajit -e ' >>> | local jp = require"jit.p" >>> | local filename = "/tmp/j.p" >>> | >>> | collectgarbage("stop") >>> | jp.start("i9999999", filename) >>> | jp.stop() >>> | >>> | local f = io.open(filename, "r") >>> | print(("<%s>"):format(f:read("*all"))) >>> | f.close() >>> | collectgarbage("restart") >>> | os.remove(filename) >>> | ' >>> >>> Before the patch, I get the following output: >>> >>> | <> >>> >>> After the patch, I get the following output: >>> >>> | <[No samples collected] >>> | > >> ok, replaced testcase >> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1304 > Nit: Missed dot at the end of the sentence. Fixed, thanks! (BTW currently 150 tests with links finished with dot, and 35 are not.) > > >> + >> + jit_p.start('i9999999', filename) > Please add the comment before the call that we set the interval of the > profiling to a huge enough value to be sure that there are no samples > collected. Added. > >> + jit_p.stop() >> + >> + local f = io.open(filename, 'r') >> + local p_content = io.open(filename):read('a*') >> +test:is(p_content, '[No samples collected]\n', 'profile dump has no samples') >> + f.close() > Typo: s/f./f:/ > Fixed. >> + >> + -- Teardown. >> + collectgarbage('restart') >> + os.remove(filename) >>> I guess this is the use case for guys from the NeoVim: >>> They run the profiler inside the application but have no samples >>> collected. The inspector may read the file from the different process, >>> but there is no need for it. And, yes, in the test we have to manually >>> stop and restart the GC, but only for the stability of the reproducer. >>> It is not a part of a use case itself. >>> >>> Your test case may be simplified as the following (in terms of my >>> script): >>> | LUA_PATH="src/?.lua;;" src/luajit -e ' >>> | local jp = require"jit.p" >>> | local filename = "/tmp/j.p" >>> | >>> | collectgarbage("stop") >>> | jp.start("i9999999", filename) >>> | jp.stop() >>> | >>> | -- Unload the module and clean the local. >>> | package.loaded["jit.p"] = nil >>> | jp = nil >>> | collectgarbage("collect") >>> | >>> | local f = io.open(filename, "r") >>> | print(("<%s>"):format(f:read("*all"))) >>> | f.close() >>> | collectgarbage("restart") >>> | os.remove(filename) >>> | ' >>> | <[No samples collected] >>> | [No samples collected] >>> | > >>> >>> Since, IMHO, there is no need to check the content (see >>> the rationale below), we may use this test case in the original file >>> without the helper script. >>> >>> So, let's check both cases (the original one and that found by you) in >>> the test. >> What is a rationale for this? > As I said before, this is an additional possible demonstration of the > bug. I see nothing bad to add it as well, since it simulates a little > bit different workload -- flushing the output before we finish the > process or if we unload the module. I am not insisting on it, though. How often users unload profiler's module? For me, this case looks artificial and the final goal for us is not a demonstration, but covering a code touched by the patch, it is done by proposed test. (I'm really confused that on review I learn more and more new requirements to the patches like adding all available testcases that cover a patch or that code should follow C89 or that using etc.) The bug fixed by the patch is quite minor, it will not break production and will not kill humans etc. I believe a single testcase and time that we both spend on doing backport, test and two iterations of review is more than enough for this patch. Moreover, jit.p is not used by Tarantool users, we provide our own profilers to them. >> This is a helper script for LuaJIT profiler and according to our >> backporting rules >> >> test must cover a backported patch. This rule is fulfilled by my test. > > --------------2UwRLOI0wySnShAHRQpQq7NG Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey,

thanks for review!

Changes applied and force-pushed.

Sergey

On 26.02.2025 14:58, Sergey Kaplun wrote:
Hi, Sergey,
Thanks for the fixes!
See my answers below.

On 25.02.25, Sergey Bronnikov wrote:

Hi, Sergey,

thanks for review! Updates applied and force-pushed.

Sergey

On 13.02.2025 18:31, Sergey Kaplun via Tarantool-patches wrote:

Hi, Sergey!
Thanks for the fixes and clarification!
Please consider by answers below.

On 12.02.25, Sergey Bronnikov wrote:

<snipped>



          

            
+local script = utils.exec.makecmd(arg)
+-- Execute a Lua script with start and stop LuaJIT profiler,
+-- it is expected no samples found by profiler. The script's
+-- output is suppressed, it is not interested.
+local _ = script(p_filename, p_postfix)
# > I don't get it. Why do we need the separate script for it?
# prof_finish() is executed first time on calling `jit.p.stop()`
#
# and executed second time when GC finalizer is calling [1] and therefore
# message "'[No samples collected]'" appears
#
# two times in the output file.
#
# 1.https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290
#
# We can inline script.lua to a test body and call garbage collector manually,
# but I propose to leave it as is because  executing as a separate process is
# more real use case.


          

          
Now it is clear to me, thanks!

This is another face of the bug that was fixed by this commit.
Nice catch!

In the original issue, automated parsing shows empty file content.
I suppose it was done with something like the following:

| LUA_PATH="src/?.lua;;" src/luajit -e '
| local jp = require"jit.p"
| local filename = "/tmp/j.p"
|
| collectgarbage("stop")
| jp.start("i9999999", filename)
| jp.stop()
|
| local f = io.open(filename, "r")
| print(("<%s>"):format(f:read("*all")))
| f.close()
| collectgarbage("restart")
| os.remove(filename)
| '

Before the patch, I get the following output:

| <>

After the patch, I get the following output:

| <[No samples collected]
| >


        

        
ok, replaced testcase


      

      

      

        
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1304


      

      
Nit: Missed dot at the end of the sentence.


    

    
Fixed, thanks!

    
(BTW currently 150 tests with links finished with dot, and 35 are
      not.)

    

    

      
<snipped>



      

        
+
+ jit_p.start('i9999999', filename)


      

      
Please add the comment before the call that we set the interval of the
profiling to a huge enough value to be sure that there are no samples
collected.

    

    Added.

    

      



      

        
+ jit_p.stop()
+
+ local f = io.open(filename, 'r')
+ local p_content = io.open(filename):read('a*')
+ test:is(p_content, '[No samples collected]\n', 'profile dump has no samples')
+ f.close()


      

      
Typo: s/f./f:/



    

    Fixed.

    

      

        
+
+ -- Teardown.
+ collectgarbage('restart')
+ os.remove(filename)


      

      

      

        

          
I guess this is the use case for guys from the NeoVim:
They run the profiler inside the application but have no samples
collected. The inspector may read the file from the different process,
but there is no need for it. And, yes, in the test we have to manually
stop and restart the GC, but only for the stability of the reproducer.
It is not a part of a use case itself.

Your test case may be simplified as the following (in terms of my
script):
| LUA_PATH="src/?.lua;;" src/luajit -e '
| local jp = require"jit.p"
| local filename = "/tmp/j.p"
|
| collectgarbage("stop")
| jp.start("i9999999", filename)
| jp.stop()
|
| -- Unload the module and clean the local.
| package.loaded["jit.p"] = nil
| jp = nil
| collectgarbage("collect")
|
| local f = io.open(filename, "r")
| print(("<%s>"):format(f:read("*all")))
| f.close()
| collectgarbage("restart")
| os.remove(filename)
| '
| <[No samples collected]
| [No samples collected]
| >

Since, IMHO, there is no need to check the </proc/$$/fd> content (see
the rationale below), we may use this test case in the original file
without the helper script.

So, let's check both cases (the original one and that found by you) in
the test.


        

        
What is a rationale for this?


      

      
As I said before, this is an additional possible demonstration of the
bug. I see nothing bad to add it as well, since it simulates a little
bit different workload -- flushing the output before we finish the
process or if we unload the module. I am not insisting on it, though.


    

    
How often users unload profiler's module? For me, this case looks
      artificial

    
and the final goal for us is not a demonstration, but covering a
      code touched by the patch,

    
it is done by proposed test.

    
(I'm really confused that on review I learn more and more new
      requirements to the patches

    
like adding all available testcases that cover a patch or that
      code should follow C89 or

    
that using etc.)

    
The bug fixed by the patch is quite minor, it will not break
      production and will not kill humans etc.

    
I believe a single testcase and time that we both spend on doing
      backport, test and two iterations

    
of review is more than enough for this patch. Moreover, jit.p is
      not used by Tarantool users,

    
we provide our own profilers to them.

    

    


    

    

      

      

        
This is a helper script for LuaJIT profiler and according to our 
backporting rules

test must cover a backported patch. This rule is fulfilled by my test.


      

      
<snipped>



    

  
  


--------------2UwRLOI0wySnShAHRQpQq7NG--