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 CEC18F3A602; Tue, 11 Feb 2025 17:54:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CEC18F3A602 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1739285662; bh=PTeUrpui6DmOmEhWrsw5we2qPqUm2dnsQEc8xtgOLW8=; 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=uJWIad9gIVcCeXhtLOjJqrowBjVIcUAnVBFL5+nZz3HiyzP3EovyXdUgoYoueZOAw VGZd2KVrmAqt+SEPCd3UE4TDYuGMp6Ncwv2PIidBMeQKSboVfxwfcVnu3B+jBFiDoT kDCxEgCp6/eWFlhPAgRYiGsbHMsmczudC12q8/LA= Received: from send149.i.mail.ru (send149.i.mail.ru [89.221.237.244]) (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 3E87DF3A602 for ; Tue, 11 Feb 2025 17:54:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3E87DF3A602 Received: by exim-smtp-79fd7578cb-8g7jp with esmtpa (envelope-from ) id 1threW-00000000L7t-0hIX; Tue, 11 Feb 2025 17:54:20 +0300 Date: Tue, 11 Feb 2025 17:53:34 +0300 To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Message-ID: References: <9c71b2f18bf2d3748a189591fce710a61c9a7f86.1739199089.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9c71b2f18bf2d3748a189591fce710a61c9a7f86.1739199089.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9DAB2B8462BE9AFBA08D286CE4974E495E1090D11B755A5CD182A05F5380850404C228DA9ACA6FE2704BABB7D49FBD7A43DE06ABAFEAF670595DD9459CC5402DEB13D1356C932AC5DFFE08B25EE243894 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F544D30F1A6FA191EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006377028CFB5BD7FD30C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D81BCB880BD433138FE9CD717CAEC858A2798186BEB652F2CCCC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8989FD0BDF65E50FBF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8BAEB924C2B054B06E75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5E459ACB7C334E09E5002B1117B3ED696649BA8754D0C486E886DC9BC01168B20823CB91A9FED034534781492E4B8EEAD003C2D46C52F18F2BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3442AF01057B6BF9974228F68CF6E7E0CB148DAC781B9BBFB6D1437E53A6AF603EACA7C6C9685178D31D7E09C32AA3244C77143B995B756C4277DD89D51EBB774286F38F338859B53CEA455F16B58544A2E30DDF7C44BCB90DA5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4p5EDZXZmhGkTPBpMRzjuA== X-DA7885C5: 565E5235D9B56F1BF255D290C0D534F9D2F3C618D94CF438B0C7BE8A82817021C7544D322E81353D5B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F739381B31377CF4CA2195D20695A87C74D7043C882CA45AB35336124D2AE6C5927CFE49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! Please consider my comments below. On 10.02.25, Sergey Bronnikov wrote: > From: Mike Pall > > Reported by Guilherme Batalheiro. > > (cherry picked from commit fca66335d131669cf017420af6963a7565babb58) > > Before the patch, a function `prof_finish` wrote a string Nit: s/`prof_finish`/`prof_finish()`/ Feel free to ignore. > `No samples collected` to profiler output file and then exits. Typo: s/profiler/a profiler/ Typo: s/exits/exited/ > Due to early exit, output file handle stay opened. This patch Typo: s/output/the output/ Typo: s/opened/open/ > fixes condition and file handle is closed even a number of samples Typo: s/condition/the condition/ Typo: s/file/the file/ Typo: s/even a number/even if the number/ > is equal to 0. > > Sergey Bronnikov: > * added the description and the test for the problem > > Part of tarantool/tarantool#11055 > --- > Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-close-file-profiler > > Related issues: > - https://github.com/luajIT/luajIT/issues/1304 > - https://github.com/tarantool/tarantool/issues/11055 > > src/jit/p.lua | 4 +-- > ...close-profile-dump-with-0-samples.test.lua | 30 +++++++++++++++++++ > .../script.lua | 14 +++++++++ > test/tarantool-tests/utils/tools.lua | 4 +++ > 4 files changed, 49 insertions(+), 3 deletions(-) > create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua > create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua > > diff --git a/src/jit/p.lua b/src/jit/p.lua > index 4569d69e..89b49584 100644 > --- a/src/jit/p.lua > +++ b/src/jit/p.lua > diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua > new file mode 100644 > index 00000000..b50b5fce > --- /dev/null > +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua > @@ -0,0 +1,30 @@ > +local tap = require('tap') > +local test = tap.test('lj-1304-close-profile-dump-with-0-samples'):skipcond({ > + ['Test requires /proc filesystem'] = jit.os == 'OSX', I suppose we may get off this skipcond, see the last comment. > +}) > +local utils = require('utils') > + > +test:plan(1) > + > +-- Test file to demonstrate LuaJIT incorrect behaviour with missed > +-- close a file handle for profile output file. Typo? s/close/closing/ Typo: s/profile/the profile/ > +-- See also: https://github.com/luajIT/luajIT/issues/1304 > + > +local p_filename = '/tmp/profile' It's better to use for the profile name generation the following function: | local profilename = require("utils").tools.profilename > + > +-- runs %testname%/script.lua by > +-- with the given environment, launch options and CLI arguments. > +local script = utils.exec.makecmd(arg) I don't get it. Why do we need the separate script for it? > +-- 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) > + > +local p_content = io.open(p_filename):read('a*') > +test:is(utils.tools.trim(p_content), '[No samples collected]', > + 'profile dump has no samples') Minor: I suppose simple `test:like()` will be enough. In that case there is no need to use the newly introduced `trim()` function here. > + > +-- Teardown. > +os.remove(p_filename) > + > +test:done(true) > diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua > new file mode 100644 > index 00000000..77335a08 > --- /dev/null > +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua > @@ -0,0 +1,14 @@ > +local p_options = 'ri1' It is better to use a much bigger interval (`i99999` for example) to be sure that there will be no samples collected. Also, the `r` option isn't required. > +local p_filename = assert(arg[1], 'filename argument is missing') > + > +require('jit.p').start(p_options, p_filename) > + > +-- No code to generate profiling samples. > + > +-- Stop profiler to execute `jit/p.lua:prof_fmt()`. With zero > +-- samples it triggers early return without closing the file. > +require('jit.p').stop() I suppose we may require this module only once. > + > +-- Make sure LuaJIT profiler is close a file handle. > +local ls_output = io.popen('ls -l /proc/$$/fd'):read('a*') Minor: I am not sure that we really need this check. The descriptor will be closed when it will be garbage collected, IINM, so there is no leak, actually. I suggest dropping it and checking only the content of the file -- this is the main issue regarding `jit.p` solved by that commit. Also, in this case we can run this test on OSX. > +assert(ls_output:find(p_filename) == nil, 'file is open') > diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua > index 33fcae78..9cb65daf 100644 > --- a/test/tarantool-tests/utils/tools.lua > +++ b/test/tarantool-tests/utils/tools.lua > @@ -21,4 +21,8 @@ function M.read_file(path) > return content > end > > +function M.trim(str) > + return (str:gsub('^%s*(.-)%s*$', '%1')) > +end > + > return M > -- > 2.34.1 > -- Best regards, Sergey Kaplun