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 7F3B26FCB3; Mon, 24 Feb 2025 21:41:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7F3B26FCB3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1740422461; bh=zmMJVYaKOlO2G5m3TrFlqyowIyLAd9EeP0S3lYPa8aY=; 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=xarIaE8hTuwm9leiy986J6+DqqRjFuxPkuNcVdg/Q0M9xeFKa0wFE7VuZpfFUthKF DoKyjYqCuXIoAI+OvEimrpdNQN1KefCC/DCrx3BK+ovTsQiLaHSI9hCFkzLxhqHbjQ K5pDO29OVxi/8ZJT4OZMRs9JtOWVOEgeIUGYuSG0= Received: from send196.i.mail.ru (send196.i.mail.ru [95.163.59.35]) (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 123B56FCB3 for ; Mon, 24 Feb 2025 21:41:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 123B56FCB3 Received: by exim-smtp-75f5fcb77d-v4nws with esmtpa (envelope-from ) id 1tmdNy-00000000GiL-110g; Mon, 24 Feb 2025 21:40:59 +0300 Content-Type: multipart/alternative; boundary="------------ZqAbWlD5jW0SeUIkDu9XzapK" Message-ID: <6f39b239-083c-4508-a364-1ba02b3b5ed3@tarantool.org> Date: Mon, 24 Feb 2025 21:40:57 +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: 4F1203BC0FB41BD957BCB5CA1E0F722C35BEE681BDA3F5709A462CE9289B68C9182A05F5380850402F0911F83B633BF13DE06ABAFEAF670585C989A7E6418D6A2108A7ABA525422265B29713DD787B1E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C579B1C3ABE6C709C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7CE54A8686262D0D1EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BC08E230531AC9C90025219EF93599969F24E70CE7A93369D2D2B91075A71A1AFA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE7328B01A8D746D8839FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C353FA85A707D24CADCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249A237A0FE984815BC76E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8BC343A871859BF7B73AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735D028CC0B556B22BCC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A55CBE50EE05BAFA3F5002B1117B3ED6960DFED46D77015572466072E6821086B3823CB91A9FED034534781492E4B8EEADEF0AF71940E62277BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34A63B03BCD35E0C0A131290D0D0621F1BC1DA4CD11159C88D6ED996B5ACDD221C0B60978ABA80AC7C1D7E09C32AA3244CCF2518E4D22A2E5677DD89D51EBB7742F0F81094A45372B2EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQQG/FugD0/CfY/ctvGf74o= X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F258845851FC9C5447CBCC0F0B9BF4F79351F757F2286AAD1C3D2B6F0E2E03FEF34853A5645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 8/8][v3] 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. --------------ZqAbWlD5jW0SeUIkDu9XzapK Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey, thanks for review! On 24.02.2025 14:14, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the fixes! > LGTM, with a few small nits below. > > On 20.02.25, Sergey Bronnikov wrote: >> sysprof has an optional parameter `path`, that sets a path to >> the profiling output file. By default the path is `sysprof.bin`. > Typo: s/default/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 the memprof profiling output file - `memprof.bin`. >> --- >> src/lib_misc.c | 5 ++- >> ...misclib-memprof-lapi-default-file.test.lua | 37 +++++++++++++++++++ >> 2 files changed, 41 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 d98cf3f0..92dc6e2a 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..ae8a73c9 >> --- /dev/null >> +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua >> @@ -0,0 +1,37 @@ >> +local tap = require('tap') >> +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') >> + >> +test:test('default-output-file', function(subtest) >> + >> +subtest:plan(1) >> + >> + local default_output_file = 'memprof.bin' >> + os.remove(default_output_file) >> + >> + local _, _ = misc.memprof.start() > Minor: Do we want to check that the profiler starts successfully? > I suppose we should, since this is the expected behaviour for this > feature. In case the profiler is not started (old behaviour) we would > get an error from the branch below: profiler not running. This isn't a > verbose definition of what goes wrong. I don't get why we should check that profiler is started in a test for default output file. > I suppose we may use `goto` here like the following: > > | local res, err = misc.memprof.start() > | -- Should start successfully. > | if not res then > | goto err_handling > | end > | > | res, err = misc.memprof.stop() > | > | ::err_handling:: > | -- Want to cleanup carefully if something went wrong. > | if not res then > >> + >> + local res, err = misc.memprof.stop() >> + >> + -- Want to cleanup carefully if something went wrong. >> + if not res then >> + os.remove(default_output_file) >> + error(err) >> + end >> + >> + local profile_buf = tools.read_file(default_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(default_output_file) >> +end) >> + >> +test:done(true) >> -- >> 2.43.0 >> --------------ZqAbWlD5jW0SeUIkDu9XzapK Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey,

thanks for review!

On 24.02.2025 14:14, Sergey Kaplun via Tarantool-patches wrote:
Hi, Sergey!
Thanks for the fixes!
LGTM, with a few small nits below.

On 20.02.25, Sergey Bronnikov wrote:

sysprof has an optional parameter `path`, that sets a path to
the profiling output file. By default the path is `sysprof.bin`.

Typo: s/default/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 the memprof profiling output file - `memprof.bin`.
---
 src/lib_misc.c                                |  5 ++-
 ...misclib-memprof-lapi-default-file.test.lua | 37 +++++++++++++++++++
 2 files changed, 41 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 d98cf3f0..92dc6e2a 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..ae8a73c9
--- /dev/null
+++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-default-file.test.lua
@@ -0,0 +1,37 @@
+local tap = require('tap')
+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')
+
+test:test('default-output-file', function(subtest)
+
+  subtest:plan(1)
+
+  local default_output_file = 'memprof.bin'
+  os.remove(default_output_file)
+
+  local _, _ = misc.memprof.start()

Minor: Do we want to check that the profiler starts successfully?
I suppose we should, since this is the expected behaviour for this
feature. In case the profiler is not started (old behaviour) we would
get an error from the branch below: profiler not running. This isn't a
verbose definition of what goes wrong.

I don't get why we should check that profiler is started in a test for default output file.


I suppose we may use `goto` here like the following:

|   local res, err = misc.memprof.start()
|   -- Should start successfully.
|   if not res then
|     goto err_handling
|   end
|
|   res, err = misc.memprof.stop()
|
| ::err_handling::
|   -- Want to cleanup carefully if something went wrong.
|   if not res then


+
+  local res, err = misc.memprof.stop()
+
+  -- Want to cleanup carefully if something went wrong.
+  if not res then
+    os.remove(default_output_file)
+    error(err)
+  end
+
+  local profile_buf = tools.read_file(default_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(default_output_file)
+end)
+
+test:done(true)
-- 
2.43.0



    

  
  


--------------ZqAbWlD5jW0SeUIkDu9XzapK--