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 2838F12D81EB; Wed, 5 Mar 2025 17:13:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2838F12D81EB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1741184034; bh=ssgf5mTqu7uGVdmdqDTBZa7MUuhrddwNAmzjM7pCYoQ=; 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=hSoEdf7CEFekw09/61cOZjnhi8Sbiy8Naba0TWo4qY8Fi1kLvZia0QRbnFXfrKhwY 2KIsiFzzHfPH4mhGVMxmKg4rcxmHhGE2oYjRf6+GYyQ/rHSw1bopV9wzQ0Ytg8fB/+ tbePVIQBVT/oqUQTs/WXlMW8hdQ2e8t3TGWJHa8c= Received: from send217.i.mail.ru (send217.i.mail.ru [95.163.59.56]) (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 6CAC14F00FB for ; Wed, 5 Mar 2025 17:13:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6CAC14F00FB Received: by exim-smtp-8cb569c79-glwmd with esmtpa (envelope-from ) id 1tppVP-000000007nx-0s6c; Wed, 05 Mar 2025 17:13:52 +0300 Content-Type: multipart/alternative; boundary="------------tEHMR7WsDNko0b0F4ORCvy65" Message-ID: <96d32e1d-eed2-4c64-b6fd-7743ab03051a@tarantool.org> Date: Wed, 5 Mar 2025 17:13:50 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US 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> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9C8AED3E6A44DB6AB71859AA6756D3A98774FB5197ED18BC0182A05F5380850403B270B5CAA309DCF3DE06ABAFEAF670558B92BE0F050DABBCDD0CD6A3005F4D6E2E35834FA4BCB46 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE731D82F3F177D3BCDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637FE9EFE935CD7C6AE8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B237E3F3FB091BC90D2E070BE324C7D3C423250E4818CCE445F6B57BC7E64490618DEB871D839B73339E8FC8737B5C224952D31B9D28593E51CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C0A29E2F051442AF778941B15DA834481F9449624AB7ADAF37BA3038C0950A5D3613377AFFFEAFD269176DF2183F8FC7C0E3E3FB6EC827F0A07B076A6E789B0E97A8DF7F3B2552694AD5FFEEA1DED7F25D49FD398EE364050F9647ADFADE5905B107FB45A5F6E725C8B3661434B16C20ACC84D3B47A649675FE827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B1B1EE5E8F15BD68475ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5F129970901B8EDD15002B1117B3ED696A5073018A7DA8BECC89B063BDC7FAC35823CB91A9FED034534781492E4B8EEADEEA082C9A12FE455BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3473688ED311681BF0B47B25A9D1F1D6ADBEACD8384D2DA896CD6A113B216761BA41985B05FD44D2DA1D7E09C32AA3244CA7BF06C18BD0923977DD89D51EBB77426E601AFA15AE22A7EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQiWK+2I7Y2s3iZr3MyNGj4= X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D614004A29623C542A66D91417EB218679B82A453653988B8915A0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 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. --------------tEHMR7WsDNko0b0F4ORCvy65 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey! The second testcase has been added and changes force-pushed to the branch. Sergey On 04.03.2025 18:40, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes, LGTM. > > On 27.02.25, Sergey Bronnikov wrote: >> Hi, Sergey, >> >> thanks for review! >> >> Changes applied and force-pushed. >> >> Sergey >> >> On 26.02.2025 14:58, Sergey Kaplun wrote: > > >>>> 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 > The following module [1] is used to reload all Lua modules (including > `jit.p`). It was used for any Lua package update or code modification. > > [1]:https://github.com/moonlibs/package-reload > > Also, it is not only about unloading the module but also about finishing > the process too (as you first discovered). This looks like a real use > case. This variant of the test is just simpler than creating a child > process and checking its result files. > >> 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 > It is not a requirement, just a suggestion. See the last line of my > comment. I'm OK with reproducing the issue only since it is not the > vital part of the code. > >> 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. > So, just ignore it then. > >> >>>> 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. >>> >>> --------------tEHMR7WsDNko0b0F4ORCvy65 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey!

The second testcase has been added and

changes force-pushed to the branch.

Sergey

On 04.03.2025 18:40, Sergey Kaplun wrote:
Hi, Sergey!
Thanks for the fixes, LGTM.

On 27.02.25, Sergey Bronnikov wrote:

Hi, Sergey,

thanks for review!

Changes applied and force-pushed.

Sergey

On 26.02.2025 14:58, Sergey Kaplun wrote:

<snipped>


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

The following module [1] is used to reload all Lua modules (including
`jit.p`). It was used for any Lua package update or code modification.

[1]: https://github.com/moonlibs/package-reload

Also, it is not only about unloading the module but also about finishing
the process too (as you first discovered). This looks like a real use
case. This variant of the test is just simpler than creating a child
process and checking its result files.


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

It is not a requirement, just a suggestion. See the last line of my
comment. I'm OK with reproducing the issue only since it is not the
vital part of the code.


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.

So, just ignore it then.




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>



    

  
  


--------------tEHMR7WsDNko0b0F4ORCvy65--