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 B58B26FC87; Wed, 29 Sep 2021 22:21:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B58B26FC87 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632943290; bh=cXAPRyL9VGIa0E7RFogbVlNDBY8adq7c0t5Vo/JjANY=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=WhW5o2KvRqVYAOsSmZCD1rVtuEiWxYlSOD99PLYPDVzcAET/OFErPD57Uhe9xamaV yik1c3fdfgEnpa8LdnqM0zWP1+CDQagaYWnxeLazVCdhELz9YxLWeBnlmTcseqqlGN +GCTbPJkZimu+zjQ7SLESUUS7va4w2S9Hz/eiZw0= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [94.100.177.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 20CC26FC87 for ; Wed, 29 Sep 2021 22:21:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 20CC26FC87 Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1mVf8y-0004de-4x; Wed, 29 Sep 2021 22:21:28 +0300 Date: Wed, 29 Sep 2021 22:21:27 +0300 To: Igor Munkin Message-ID: <20210929192127.i53vbniej5er652q@surf.localdomain> References: <20210820070546.115293-1-m.shishatskiy@tarantool.org> <20210820070546.115293-4-m.shishatskiy@tarantool.org> <20210916153201.GC6844@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline In-Reply-To: <20210916153201.GC6844@tarantool.org> X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD96A58C36AA2E99649BF631F26B0465AFD0E15652C7D51B98D182A05F538085040CAD926AA567F47B2EE2B4B6C19766CE3CD48F715F15C94DA74C2A744B9EA69BA X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE721AF84DC1D70954DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BCF76D51F00B42068638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C2E78D652BCEB9850E2ED606E08DFD82117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EECCD848CCB6FE560C6B91AC3BD56FC247D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE35B63D382EEF4D8962D242C3BD2E3F4C6C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637AD0424077D726551EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79DB194B0D77246B867D535F66A8B3211F8 X-C1DE0DAB: 0D63561A33F958A5366972AA6C7454F98A011174F349267C51E129E46CFB53C1D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75BFC02AB3DF06BA5A410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347324AA9FA07FF01EE2C68BB6E74843820B133EDB5C239D497EB61E793EFC781BD316E4F15FF9F0951D7E09C32AA3244C45AD39C707C38209DACACC9B8B99B9EFF26BFA4C8A6946B8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojsAIehEB+JdkxOr0pE60Ztw== X-Mailru-Sender: EFA0F3A8419EF21635BFE795C6CB22C90A448BBF58211AC6EE2B4B6C19766CE3BA0848438379BFCE2376072A51849BFFE66B5C1DBFD5D09D5E022D45988A037B448E0EA96F20AB367402F9BA4338D657ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v3 3/5] memprof: dump traceno if allocate from trace 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: Mikhail Shishatskiy via Tarantool-patches Reply-To: Mikhail Shishatskiy Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Igor! Thank you for the review! On 16.09.2021 18:32, Igor Munkin wrote: >Misha, > >Thanks for the patch! Please consider my comments below. > >On 20.08.21, Mikhail Shishatskiy wrote: >> When LuaJIT executes a trace, the trace number is stored in >> the virtual machine state. So, we can treat this number as >> an allocation event source in memprof and report allocation events >> from traces as well. >> >> Previously, all the allocations from traces were marked as INTERNAL. >> >> This patch introduces the functionality described above by adding >> a new allocation source type named ASOURCE_TRACE. If at the moment >> when allocation event occurs VM state indicates that trace executed, >> trace number streamed to a binary file: >> >> | loc-trace := trace-addr trace-no >> | trace-addr := >> | trace-no := >> >> Also, the memory profiler parser is adjusted to recognize this >> source type by extending structure: field , >> representing trace number, is added. > >I understand, why you've chosen this order, but I don't like it. IMHO, >the binary format should not rely or depend on the particular parser >implementation a lot. Please, consider more comments below. Fixed in the upcoming patch series v4. > >> >> Part of tarantool/tarantool#5814 >> --- >> >> Issue: https://github.com/tarantool/tarantool/issues/5814 >> Luajit branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number >> tarantool branch: https://github.com/tarantool/tarantool/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number >> >> src/Makefile.dep.original | 2 +- >> src/lj_memprof.c | 35 +++++++++++++++++++++++++++++++++-- >> src/lj_memprof.h | 15 ++++++++++----- >> tools/memprof/parse.lua | 22 ++++++++++++++-------- >> 4 files changed, 58 insertions(+), 16 deletions(-) >> >> diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original >> index f3672413..ee6bafb2 100644 >> --- a/src/Makefile.dep.original >> +++ b/src/Makefile.dep.original >> @@ -146,7 +146,7 @@ lj_mcode.o: lj_mcode.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \ >> lj_gc.h lj_err.h lj_errmsg.h lj_jit.h lj_ir.h lj_mcode.h lj_trace.h \ >> lj_dispatch.h lj_bc.h lj_traceerr.h lj_vm.h >> lj_memprof.o: lj_memprof.c lj_arch.h lua.h luaconf.h lj_memprof.h \ >> - lj_def.h lj_wbuf.h lj_obj.h lj_frame.h lj_bc.h lj_debug.h >> + lj_def.h lj_wbuf.h lj_obj.h lj_frame.h lj_bc.h lj_debug.h lj_dispatch.h > >It looks some headers are missing (it's better use from >Makefile.original to check yourself). Fixed in the upcoming patch series v4. > >> lj_meta.o: lj_meta.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_gc.h \ >> lj_err.h lj_errmsg.h lj_buf.h lj_str.h lj_tab.h lj_meta.h lj_frame.h \ >> lj_bc.h lj_vm.h lj_strscan.h lj_strfmt.h lj_lib.h >> diff --git a/src/lj_memprof.c b/src/lj_memprof.c >> index 2c1ef3b8..fb99829d 100644 >> --- a/src/lj_memprof.c >> +++ b/src/lj_memprof.c > > > >> @@ -168,9 +197,11 @@ static const memprof_writer memprof_writers[] = { >> ** But since traces must follow the semantics of the original code, >> ** behaviour of Lua and JITted code must match 1:1 in terms of allocations, >> ** which makes using memprof with enabled JIT virtually redundant. >> - ** Hence use the stub below. >> + ** But if one wants to investigate allocations with JIT enabled, >> + ** memprof_write_trace() dumps trace number to the binary output. > >Typo: number and mcode starting address, right? Fixed in the upcoming patch series v4. > >> + ** It can be useful to compare with with jit.v or jit.dump outputs. >> */ >> - memprof_write_hvmstate /* LJ_VMST_TRACE */ >> + memprof_write_trace /* LJ_VMST_TRACE */ >> }; >> >> static void memprof_write_caller(struct memprof *mp, uint8_t aevent) >> diff --git a/src/lj_memprof.h b/src/lj_memprof.h >> index 3417475d..6a35385d 100644 >> --- a/src/lj_memprof.h >> +++ b/src/lj_memprof.h >> @@ -51,9 +51,10 @@ >> */ >> >> #define SYMTAB_LFUNC ((uint8_t)0) >> +#define SYMTAB_TRACE ((uint8_t)1) > >This looks like related to the next patch, doesn't it? Fixed in the upcoming patch series v4. > >> #define SYMTAB_FINAL ((uint8_t)0x80) >> >> -#define LJM_CURRENT_FORMAT_VERSION 0x01 >> +#define LJM_CURRENT_FORMAT_VERSION 0x02 >> >> /* >> ** Event stream format: > > > >> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua >> index 12e2758f..adc7c072 100644 >> --- a/tools/memprof/parse.lua >> +++ b/tools/memprof/parse.lua > > > >> @@ -24,8 +24,11 @@ local AEVENT_MASK = 0x3 >> local ASOURCE_INT = lshift(1, 2) >> local ASOURCE_LFUNC = lshift(2, 2) >> local ASOURCE_CFUNC = lshift(3, 2) >> +local ASOURCE_TRACE = lshift(4, 2) >> >> -local ASOURCE_MASK = lshift(0x3, 2) >> +local ASOURCE_MASK = lshift(0x7, 2) >> + >> +local EV_HEADER_MAX = ASOURCE_TRACE + AEVENT_REALLOC > >Why so complex? I believe lshift(5, 2) is more clear and covers (i.e. is >greater than) all cases of AEVENT_* and ASOURCE_*. As for me, lshift(5, 2) is less descriptive. ASOURCE_TRACE + AEVENT_REALLOC shows the layout of flags in the header [FUUSSSEE] ^^^^^ and gives an idea, why EV_HEADER_MAX is EV_HEADER_MAX :) > >> >> local M = {} >> >> @@ -59,20 +62,23 @@ local function link_to_previous(heap_chunk, e, nsize) >> end >> end >> >> -local function id_location(addr, line) >> - return string_format("f%#xl%d", addr, line), { >> +local function id_location(addr, line, traceno) >> + return string_format("f%#xl%dt%d", addr, line, traceno), { >> addr = addr, >> line = line, >> + traceno = traceno, >> } >> end >> >> local function parse_location(reader, asource) >> if asource == ASOURCE_INT then >> - return id_location(0, 0) >> + return id_location(0, 0, 0) >> elseif asource == ASOURCE_CFUNC then >> - return id_location(reader:read_uleb128(), 0) >> + return id_location(reader:read_uleb128(), 0, 0) >> elseif asource == ASOURCE_LFUNC then >> - return id_location(reader:read_uleb128(), reader:read_uleb128()) >> + return id_location(reader:read_uleb128(), reader:read_uleb128(), 0) >> + elseif asource == ASOURCE_TRACE then >> + return id_location(reader:read_uleb128(), 0, reader:read_uleb128()) > >As a result of your changes this function becomes too "cryptic". It's >better to refactor this function (maybe even in a separate commit), so >we have something like the function below at the final. Refactored in the upcoming patch series v4. > >| local function id(params) >| return string_format("f%#xl%ds%d", params.addr, params.line, params.state) >| end >| >| local function parse_location(reader, asource) >| local location = { addr = 0, line = 0, traceno = 0 } >| if asource == ASOURCE_INT then >| -- Do nothing >| elseif asource == ASOURCE_CFUNC then >| location.addr = reader:read_uleb128() >| elseif asource == ASOURCE_LFUNC then >| location.addr = reader:read_uleb128() >| location.line = reader:read_uleb128() >| elseif asource == ASOURCE_TRACE then >| location.trace = reader:read_uleb128() >| location.addr = reader:read_uleb128() >| else >| error("Unknown asource "..asource) >| end >| return id(location), location >| end > >You can also make this function public and move it to utils.lua module. > >BTW, these entries are "loaded" but not "rendered" in the final output >now, aren't they? In other words, why don't you make everything in a >single patch? My bad, I split changes in quite a strange way. New patch series makes it more "natural": simple rendering is moved to this patch. More complex "started at ..." is added in another patch. > >> end >> error("Unknown asource "..asource) > end > > > >> -- >> 2.32.0 >> > >-- >Best regards, >IM Best regards, Mikhail Shishatskiy