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 E40EF1256B1D; Wed, 26 Feb 2025 14:58:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E40EF1256B1D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1740571133; bh=sM1TYqZSHNMZP2U7IU6Yvdp/cftxNSZBoAop9q9qwd8=; 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=B/rN0NkNevK16+cIsbwSg/SThC9g495+4jAuORQW9yER0xCiM/sW9/6xwh4nmnqS6 m6w1+JgHrljCOfUwuXRGPUttymYjn0//x/aPsdXgcwR/WppBeo2Awy82CqldEEvscZ Z5T0//m6/dWpIg/bkzb5q+AT3Vffdk4BKFbQe7As= Received: from send242.i.mail.ru (send242.i.mail.ru [95.163.59.81]) (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 B2BEF464E97 for ; Wed, 26 Feb 2025 14:58:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B2BEF464E97 Received: by exim-smtp-75f5fcb77d-r2sdt with esmtpa (envelope-from ) id 1tnG3t-000000005NL-2QbZ; Wed, 26 Feb 2025 14:58:50 +0300 Date: Wed, 26 Feb 2025 14:58:08 +0300 To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Message-ID: References: <80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527.1739362814.git.sergeyb@tarantool.org> <93178b00-f3e0-4145-932f-bd273935a477@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <93178b00-f3e0-4145-932f-bd273935a477@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD902203E0DD57300ECF8D247464B1F7AE92F0A3D46B8B757FE182A05F5380850407E4F38A5B492DCE43DE06ABAFEAF6705403F4603BA9D3A1E4D0204AB399A1940C1F8F9D846F8CF10 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70312E9A300D47E3BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006373DF18198E8D8384F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D82BA58665DC0A788F4871EF9201A5B06DC5D01C8E6259D9A0CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8062BEEFFB5F8EA3EF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CDA532D2019E286A7AD7EC71F1DB884274AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3BC70C8FBAE0A4268BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7588D3C263EAE74EA731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5177301655989B21E5002B1117B3ED696A1292FAD5D1523185D145BB8EF0DE66B823CB91A9FED034534781492E4B8EEAD14747542773C033FBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34A5112A9AECFE11B1267D11195139A618EB706ED5C5A5971F660C18F19AA9CF1CC7E0783CA48744D71D7E09C32AA3244CB688D62099CCCA0B77DD89D51EBB7742DFA5C2BB408D446FEA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQQG/FugD0/CIFV+TRgLqis= X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B595BA73C378090D3E63DE06ABAFEAF6705403F4603BA9D3A1EB7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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. > + > + 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. > + 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:/ > + > + -- 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. > > 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. -- Best regards, Sergey Kaplun