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 89806F415F1; Tue, 18 Feb 2025 17:20:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 89806F415F1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739888455; bh=LXV1mzZFWpcCFZhet0i/CjZmzgRV4UATLA3wWDDspiE=; 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=G72lMY+txYyJH3dCwTC1Bs01meiaJ5saTtR1xpyMYEK1XtYcyc97Ay52QEM07+7TD 6lShggqUyr6xVyK6xzZ6RB72d0KTJbjJ70DG78jObemw74waTwyF3xQYRUBpfQTcKf 2WBbSnpcxaQLGGGaRGz4Nb6HLc/QoZLWHxRB+594= Received: from send277.i.mail.ru (send277.i.mail.ru [95.163.59.116]) (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 A65956D60B0 for ; Tue, 18 Feb 2025 17:20:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A65956D60B0 Received: by exim-smtp-844687bc8-xwbhx with esmtpa (envelope-from ) id 1tkOSz-00000000Ltt-2nai; Tue, 18 Feb 2025 17:20:54 +0300 Content-Type: multipart/alternative; boundary="------------GI1t4PgW5huvWYWa5NNJkspO" Message-ID: <5458f602-5612-447c-a1a8-7b89097d75cb@tarantool.org> Date: Tue, 18 Feb 2025 17:20:52 +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: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD916C41472748AFA04848245097E48316D09B3304720E2CFD400894C459B0CD1B94DEA687170C565F0460A1D3C0470A96DD122BDBF1E713799F7AB22C17522FD12E26C7FC155FDCB66 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CC84CC3AD347B910EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378F586D843116CFB2EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BC08E230531AC9C906275EF0E96E98F4EB69B0FA9CDB855B9CFB6E909CA6E07ECA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C34C82C86BFC697D19117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF9A1C9D3BA3303E89BA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B6BFC5BFC0DB013F7476E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C1156E5889A6D309143847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A559FE3454E3E48A675002B1117B3ED696DF1C9650B640B12DB2920F75BA9A967F823CB91A9FED034534781492E4B8EEADC3194D76C41E9723BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D344A431191C56981FE1CC798AF0D8C96A4E18C3FFBD4F17C927B0D9C7D6ECD7C7700388E247AA63CEE1D7E09C32AA3244CFA820FFEDAB8901877DD89D51EBB774216071D955D666956EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVWiyXSWEEqdrEGgE8L6jwRI= X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F258845827C74D762D23992B04DE63E6493D46050D94FAD1D274C8C6BEC46C5FD3C206C9645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 7/7] memprof: set default path to profiling 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. --------------GI1t4PgW5huvWYWa5NNJkspO Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey, thanks for review! On 18.02.2025 14:55, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > LGTM, with a minor nits below. > > On 13.02.25, Sergey Bronnikov wrote: >> sysprof has an optional parameter `path`, that set a path to > Typo: s/set/sets/ Fixed. > >> profiling output file, by default the path is `sysprof.bin`. > Typo: s/profiling/the profiling/ > Typo: s/file, by default/file. By default,/ Fixed. >> `misc.memprof.start()` requires to set a path to profiling output > Typo: s/to set/setting/ > Typo: s/profiling/the profiling/ Fixed. > >> file. The patch fixes this inconsistency by introducing a default >> path to memprof profiling output file - `memprof.bin`. > Typo: s/memprof/the memprof/ Fixed. > >> --- >> src/lib_misc.c | 5 ++- >> ...misclib-memprof-lapi-default-file.test.lua | 41 +++++++++++++++++++ >> 2 files changed, 45 insertions(+), 1 deletion(-) >> create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua >> >> diff --git a/src/lib_misc.c b/src/lib_misc.c >> index 7666d85f..4f5d72fc 100644 >> --- a/src/lib_misc.c >> +++ b/src/lib_misc.c > > >> diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua >> new file mode 100644 >> index 00000000..b6233d4a >> --- /dev/null >> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua >> @@ -0,0 +1,41 @@ >> +local tap = require("tap") > Minor: Let's use single quotes here and below. > Side note: This code style part wasn't fixed when the first tests for > memprof was written. Fixed. >> +local test = tap.test("misc-memprof-lapi-default-file"):skipcond({ >> + ["Memprof is implemented for x86_64 only"] = jit.arch ~= "x86" and >> + jit.arch ~= "x64", >> + ["Memprof is disabled"] = os.getenv('LUAJIT_DISABLE_MEMPROF'), >> +}) >> + >> +test:plan(1) >> + >> +local tools = require "utils.tools" > Minor: Please use brackets for function calls. Fixed. > >> + >> +test:test("default-output-file", function(subtest) >> + >> +subtest:plan(1) >> + >> + local def_output_file = 'memprof.bin' > Minor: s/def/default/ Fixed. > >> + os.remove(def_output_file) >> + >> + local res, err = misc.memprof.start() >> + -- Should start successfully. >> + assert(res, err) > This assert should be removed since we want to remove the file in case > of an error (for example, we can't write the memprof prologue). Fixed. > >> + >> + res, err = misc.memprof.stop() >> + -- Should stop successfully. >> + assert(res, err) > This assert should be removed since we want to remove the file in case > of an error. Fixed. > >> + >> + -- Want to cleanup carefully if something went wrong. >> + if not res then >> + os.remove(def_output_file) >> + error(err) >> + end >> + >> + local profile_buf = tools.read_file(def_output_file) >> +subtest:ok(profile_buf ~= nil and #profile_buf ~= 0, >> + 'default output file is not empty') >> + >> + -- We don't need it any more. >> + os.remove(def_output_file) >> +end) >> + >> +test:done(true) >> -- >> 2.34.1 >> --------------GI1t4PgW5huvWYWa5NNJkspO Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey,

thanks for review!

On 18.02.2025 14:55, Sergey Kaplun via Tarantool-patches wrote:
Hi, Sergey!
Thanks for the patch!
LGTM, with a minor nits below.

On 13.02.25, Sergey Bronnikov wrote:

sysprof has an optional parameter `path`, that set a path to

Typo: s/set/sets/
Fixed.


profiling output file, by default the path is `sysprof.bin`.

Typo: s/profiling/the profiling/
Typo: s/file, by default/file. By default,/

Fixed.



      

        
`misc.memprof.start()` requires to set a path to profiling output


      

      
Typo: s/to set/setting/
Typo: s/profiling/the profiling/

    

    Fixed.

    

      



      

        
file. The patch fixes this inconsistency by introducing a default
path to memprof profiling output file - `memprof.bin`.


      

      
Typo: s/memprof/the memprof/

    

    Fixed.

    

      



      

        
---
 src/lib_misc.c                                |  5 ++-
 ...misclib-memprof-lapi-default-file.test.lua | 41 +++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 7666d85f..4f5d72fc 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c


      

      
<snipped>



      

        
diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
new file mode 100644
index 00000000..b6233d4a
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -0,0 +1,41 @@
+local tap = require("tap")


      

      
Minor: Let's use single quotes here and below.
Side note: This code style part wasn't fixed when the first tests for
memprof was written.


    

    Fixed.

    

      

      

        
+local test = tap.test("misc-memprof-lapi-default-file"):skipcond({
+  ["Memprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
+                                               jit.arch ~= "x64",
+  ["Memprof is disabled"] = os.getenv('LUAJIT_DISABLE_MEMPROF'),
+})
+
+test:plan(1)
+
+local tools = require "utils.tools"


      

      
Minor: Please use brackets for function calls.

    

    Fixed.

    

      



      

        
+
+test:test("default-output-file", function(subtest)
+
+  subtest:plan(1)
+
+  local def_output_file = 'memprof.bin'


      

      
Minor: s/def/default/

    

    Fixed.

    

      



      

        
+  os.remove(def_output_file)
+
+  local res, err = misc.memprof.start()
+  -- Should start successfully.
+  assert(res, err)


      

      
This assert should be removed since we want to remove the file in case
of an error (for example, we can't write the memprof prologue).

    

    Fixed.

    

      



      

        
+
+  res, err = misc.memprof.stop()
+  -- Should stop successfully.
+  assert(res, err)


      

      
This assert should be removed since we want to remove the file in case
of an error.

    

    


    

    
Fixed.

    

    

      



      

        
+
+  -- Want to cleanup carefully if something went wrong.
+  if not res then
+    os.remove(def_output_file)
+    error(err)
+  end
+
+  local profile_buf = tools.read_file(def_output_file)
+  subtest:ok(profile_buf ~= nil and #profile_buf ~= 0,
+             'default output file is not empty')
+
+  -- We don't need it any more.
+  os.remove(def_output_file)
+end)
+
+test:done(true)
-- 
2.34.1



      

      

    

  
  


--------------GI1t4PgW5huvWYWa5NNJkspO--