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 9859D6F879; Tue, 25 Jan 2022 11:17:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9859D6F879 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1643098620; bh=Sg0G0+KO79aOY4d7lZ79e4PFnDdeQIFOYfgL3wG9LBg=; 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=l7tAN60PhZ5Ppzs5QwNq+c8lq+8Tnyl9DsUxibPnFsEwICJZ3vlwAQ3BAC9CSTPlU 2OdIE77fg1XINgnNu1+ZK9xpf8d8Q0UUecgRHSeI6VT1FIFOsSQZ0iCcZkhdmcpaYH dB+OwSpPoNFMPr7t9yuebSedLq6TaYXgu7jzwe3k= Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 2359D6F879 for ; Tue, 25 Jan 2022 11:16:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2359D6F879 Received: by smtp54.i.mail.ru with esmtpa (envelope-from ) id 1nCH0c-0000t4-83; Tue, 25 Jan 2022 11:16:58 +0300 Date: Tue, 25 Jan 2022 11:14:59 +0300 To: Mikhail Shishatskiy Message-ID: References: <20211202110329.664738-1-m.shishatskiy@tarantool.org> <20211202110329.664738-3-m.shishatskiy@tarantool.org> <20220119165650.mxddl6so6txvavkf@surf.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220119165650.mxddl6so6txvavkf@surf.localdomain> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9AA78FDF62ECAE61FF13F204EA2BDEB7EE91D6674DF21EB76182A05F5380850401AC5633F0F724CA379BA553319EA186E6B36F1141CEA60972884F8B5643F856A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73F300A97FDD4E158EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E0A24425AA421BFF8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C4CE36595A000766BD0906A88B33766F117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B8C7ADC89C2F0B2A5A471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C70E8A7D5F64413BE43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A58BFBF755EDE6C84C7EDD51BF074485B81CC79374C281019DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75B7BFB303F1C7DB4D8E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D2AC226BFD455724DCB27A4B63C920BBD0C3BBFCA8E0DBAF770C538314C03CC530883125B5DA02341D7E09C32AA3244CE01197D2FCCD03A1B8DDE2B8667F74AB6C24832127668422FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPq2vZmbSbSQEXmZ4waq3qA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB48AE0E69E53EF39CF8CB4B4230B47D4BE02DB81B281332CC1F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A84198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2 2/3] memprof: enrich symtab when meeting new prototype 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 Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Mikhail! Thanks for the patch! LGTM, except a few comments below. Also, please share Tarantool's branch for tests, I can't found it :(. On 19.01.22, Mikhail Shishatskiy wrote: > Updated patch in line with what we discussed with Sergey offline: > > New commit message: > =========================================================== > memprof: enrich symtab when meeting new prototype > > Previously, the profiler dumped all the function prototypes only > at the start of profiling. It was enough for most cases, but > sometimes we may want to investigate the memory profile of module > loading. In such a case, we expect memprof to dump new prototypes > from parsed source code. > > This patch extends memprof's streaming format with entries providing > extra information about allocation source's symbols if they were not > streamed at the start of profiling. This event is called > and precedes the allocation-related event. The format is the following: > > | event-symtab := event-header sym? > | sym := sym-lua > | sym-lua := sym-addr sym-chunk sym-line > > The `sym-addr`, `sym-chunk` and `sym-line` are the prototype's address, > its chunk name and line where it was defined. > > The profiler dumps new prototypes to the symbol table right after the > creation of a GCproto object. > > Also, the profiler parser is adjusted to recognize entries described > above and enrich the symtab on the fly while parsing events. For this > reason, the API has been changed: the function > `parse_sym_lfunc` became public to be available for the parser module. > > Resolves tarantool/tarantool#5815 > =========================================================== > > New diff: Did I understand correctly, that after this patch there is no need in epoch labels for prototypes? So the previous commit can be removed? > =========================================================== > diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original > index faa44a0b..82f34315 100644 > --- a/src/Makefile.dep.original > +++ b/src/Makefile.dep.original > @@ -59,7 +59,7 @@ lj_bc.o: lj_bc.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_bc.h \ > lj_bcread.o: lj_bcread.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_bc.h \ > lj_ctype.h lj_cdata.h lualib.h lj_lex.h lj_bcdump.h lj_state.h \ > - lj_strfmt.h > + lj_strfmt.h lj_memprof.h You must mention lj_wbuf.h here too, as far as it is included by lj_memprof.h. > lj_bcwrite.o: lj_bcwrite.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \ > lj_gc.h lj_buf.h lj_str.h lj_bc.h lj_ctype.h lj_dispatch.h lj_jit.h \ > lj_ir.h lj_strfmt.h lj_bcdump.h lj_lex.h lj_err.h lj_errmsg.h lj_vm.h > @@ -175,7 +175,7 @@ lj_opt_split.o: lj_opt_split.c lj_obj.h lua.h luaconf.h lj_def.h \ > lj_parse.o: lj_parse.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \ > lj_gc.h lj_err.h lj_errmsg.h lj_debug.h lj_buf.h lj_str.h lj_tab.h \ > lj_func.h lj_state.h lj_bc.h lj_ctype.h lj_strfmt.h lj_lex.h lj_parse.h \ > - lj_vm.h lj_vmevent.h > + lj_vm.h lj_vmevent.h lj_memprof.h Ditto. > lj_profile.o: lj_profile.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \ > lj_buf.h lj_gc.h lj_str.h lj_frame.h lj_bc.h lj_debug.h lj_dispatch.h \ > lj_jit.h lj_ir.h lj_trace.h lj_traceerr.h lj_profile.h luajit.h > diff --git a/src/lj_bcread.c b/src/lj_bcread.c > index 48c5e7c7..cb08599d 100644 > --- a/src/lj_bcread.c > +++ b/src/lj_bcread.c > @@ -22,6 +22,9 @@ > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > index 2d779983..9bb2483e 100644 > --- a/src/lj_memprof.c > +++ b/src/lj_memprof.c > @@ -66,6 +66,14 @@ static void dump_symtab_trace(struct lj_wbuf *out, const GCtrace *trace) > > #endif > > +static void dump_symtab_proto(struct lj_wbuf *out, const GCproto *pt, > + const global_State *g) Looks like global_State is unused in this function. > +{ > + lj_wbuf_addu64(out, (uintptr_t)pt); > + lj_wbuf_addstring(out, proto_chunknamestr(pt)); > + lj_wbuf_addu64(out, (uint64_t)pt->firstline); > +} > + > static void dump_symtab(struct lj_wbuf *out, const struct global_State *g) > { > const GCRef *iter = &g->gc.root; > @@ -80,9 +88,7 @@ static void dump_symtab(struct lj_wbuf *out, const struct global_State *g) > case (~LJ_TPROTO): { > const GCproto *pt = gco2pt(o); > lj_wbuf_addbyte(out, SYMTAB_LFUNC); > - lj_wbuf_addu64(out, (uintptr_t)pt); > - lj_wbuf_addstring(out, proto_chunknamestr(pt)); > - lj_wbuf_addu64(out, (uint64_t)pt->firstline); > + dump_symtab_proto(out, pt, g); > break; > } > case (~LJ_TTRACE): { > @@ -140,6 +146,7 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent, The next chunk (*) seems like unnecessary changes, why do we need them? We can just add the comment to the old chunk if you want. > ** -DLUAJIT_DISABLE_DEBUGINFO flag. > */ > const BCLine line = lj_debug_frameline(L, fn, nextframe); > + const GCproto *pt = funcproto(fn); > > if (line < 0) { > /* > @@ -151,11 +158,17 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent, > ** We report such allocations as internal in order not to confuse users. > */ > lj_wbuf_addbyte(out, aevent | ASOURCE_INT); > - } else { > - lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC); > - lj_wbuf_addu64(out, (uintptr_t)funcproto(fn)); > - lj_wbuf_addu64(out, (uint64_t)line); > + return; > } > + > + /* > + ** As a prototype is a source of an allocation, it has > + ** already been inserted into the symtab: on the start > + ** of the profiling or right after the creation. > + */ > + lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC); > + lj_wbuf_addu64(out, (uintptr_t)pt); > + lj_wbuf_addu64(out, (uint64_t)line); > } > > static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t aevent, (*) up to here. > @@ -414,6 +427,17 @@ errio: > diff --git a/src/lj_memprof.h b/src/lj_memprof.h > index 395fb429..eda8ca0c 100644 > --- a/src/lj_memprof.h > +++ b/src/lj_memprof.h > @@ -164,4 +174,10 @@ int lj_memprof_start(struct lua_State *L, const struct lj_memprof_options *opt); > */ > int lj_memprof_stop(struct lua_State *L); > > +/* > +** Enriches profiler symbol table with a new proto, if profiler Typo: s/profiler/the profiler/ > +** is running. > +*/ > +void lj_memprof_add_proto(const struct GCproto *pt); > + > #endif > diff --git a/src/lj_parse.c b/src/lj_parse.c > index a6325a76..30b0caa0 100644 > --- a/src/lj_parse.c > +++ b/src/lj_parse.c > @@ -27,6 +27,9 @@ > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > index dd973f2a..1c74b4d7 100644 > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > @@ -7,7 +7,7 @@ require("utils").skipcond( > @@ -223,6 +224,38 @@ test:test("stack-resize", function(subtest) > misc.memprof.stop() > end) > > +-- Test for extending symtab with function prototypes > +-- while profiler is running. > +test:test("symtab-enriching", function(subtest) > + subtest:plan(2) > + > + local payload_str = [[ > + local M = { > + tmp = string.rep("1", 100) -- line 2. > + } > + > + function M.payload() > + local str = string.rep("42", 100) -- line 6. > + end > + > + return M > + ]] > + > + local symbols, events = generate_parsed_output(function() > + local str_chunk = assert(loadstring(payload_str, 'str_chunk'))() > + str_chunk.payload() > + end) > + > + local alloc = fill_ev_type(events, symbols, "alloc") > + > + subtest:ok(check_alloc_report( > + alloc, { source = 'str_chunk', line = 6, linedefined = 5 }, 1) > + ) > + subtest:ok(check_alloc_report( > + alloc, { source = 'str_chunk', line = 2, linedefined = 0 }, 1) > + ) It is also good to test bcread case. Please, add the corresponding test. > +end) > + > -- Test profiler with enabled JIT. > jit.on() > > diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua > index be5844a4..38f76f00 100644 > --- a/tools/memprof/parse.lua > +++ b/tools/memprof/parse.lua > @@ -13,10 +13,11 @@ local string_format = string.format > diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua > index c758b67e..00bab03a 100644 > --- a/tools/utils/symtab.lua > +++ b/tools/utils/symtab.lua > =========================================================== > > On 02.12.2021 14:03, Mikhail Shishatskiy wrote: > > -- Best regards, Sergey Kaplun