From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 AFD20430408 for ; Wed, 12 Aug 2020 12:49:56 +0300 (MSK) Date: Wed, 12 Aug 2020 12:39:32 +0300 From: Igor Munkin Message-ID: <20200812093932.GZ18920@tarantool.org> References: <20200721113451.25817-1-skaplun@tarantool.org> <20200721113451.25817-2-skaplun@tarantool.org> <20200723201219.GQ18920@tarantool.org> <20200727022546.GA11579@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200727022546.GA11579@root> Subject: Re: [Tarantool-patches] [PATCH 1/2] metrics: add counters for metrics interested in List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Sergey, On 27.07.20, Sergey Kaplun wrote: > Igor, > > On 23.07.20, Igor Munkin wrote: > > Sergey, > > > > Thanks for the patch! Please consider my comments below. > > > > On 21.07.20, Sergey Kaplun wrote: > > > > > > > This counter is not incremented if cdata is allocated on trace. Consider > > the following test: > > | $ LUA_PATH="$HOME/.luarocks/share/lua/5.1/?.lua;;" ./luajit -joff < > | heredoc> local t = { } > > | heredoc> for i = 1, 1000 do > > | heredoc> table.insert(t, require('ffi').new('uint64_t', i)) > > | heredoc> end > > | heredoc> t = nil > > | heredoc> collectgarbage('collect') > > | heredoc> print(require('inspect')(misc.getmetrics())) > > | heredoc> EOF > > | { > > | global = { > > | cdatanum = 0, > > | gc_total = 74722, > > | jit_mcode_size = 0, > > | jit_trace_num = 0, > > | strnum = 534, > > | tabnum = 77, > > | udatanum = 5 > > | }, > > | incremental = { > > | gc_allocated = 173092, > > | gc_freed = 98370, > > | gc_steps_atomic = 1, > > | gc_steps_finalize = 0, > > | gc_steps_pause = 3, > > | gc_steps_propagate = 553, > > | gc_steps_sweep = 64, > > | gc_steps_sweepstring = 1024, > > | jit_snap_restore = 0, > > | jit_trace_abort = 0, > > | strhash_hit = 4021, > > | strhash_miss = 538 > > | } > > | } > > | $ LUA_PATH="$HOME/.luarocks/share/lua/5.1/?.lua;;" ./luajit -jon < > | heredoc> local t = { } > > | heredoc> for i = 1, 1000 do > > | heredoc> table.insert(t, require('ffi').new('uint64_t', i)) > > | heredoc> end > > | heredoc> t = nil > > | heredoc> collectgarbage('collect') > > | heredoc> print(require('inspect')(misc.getmetrics())) > > | heredoc> EOF > > | { > > | global = { > > | cdatanum = -941, > > | gc_total = 76561, > > | jit_mcode_size = 65536, > > | jit_trace_num = 3, > > | strnum = 534, > > | tabnum = 77, > > | udatanum = 5 > > | }, > > | incremental = { > > | gc_allocated = 145299, > > | gc_freed = 68738, > > | gc_steps_atomic = 3, > > | gc_steps_finalize = 0, > > | gc_steps_pause = 2, > > | gc_steps_propagate = 505, > > | gc_steps_sweep = 64, > > | gc_steps_sweepstring = 1024, > > | jit_snap_restore = 6, > > | jit_trace_abort = 0, > > | strhash_hit = 3085, > > | strhash_miss = 538 > > | } > > | } > > > > As you can see global.cdatanum is negative when JIT is on. You can take > > a look on the compiled trace by yourself. The root cause is CNEW IR > > semantics: for VLA/VLS cdata types call is emitted and > > the counters are fine, but for other cdata types JIT compiler emits just > > a call that so this object is not counted. Thereby JIT > > has to emit the corresponding instructions while assembling CNEW IR. > > I will elaborate on this later. > > Can we use an approach like this? Kinda. > > diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h > index 3e189b1..a32a8c4 100644 > --- a/src/lj_asm_x86.h > +++ b/src/lj_asm_x86.h > @@ -1835,6 +1835,12 @@ static void asm_cnew(ASMState *as, IRIns *ir) > return; > } > > + /* Global state live longer than given trace. Global state is a Lua universe, so this line is too obvious. Typo: s/live/lives/. > + ** Increment cdatanum counter by address directly. > + */ > + emit_gmroi(as, XG_ARITHi(XOg_ADD), RID_NONE, > + ptr2addr(&J2G(as->J)->gc.cdatanum), (int32_t)1); I see no reason for explicit cast here: * the parameter is type * IIRC, integral literals are 32 bit integer types There is also an additional check within and imm8 addition is emitted for your case. Besides, other emitters are still left unpatched. > + > /* Combine initialization of marked, gct and ctypeid. */ > emit_movtomro(as, RID_ECX, RID_RET, offsetof(GCcdata, marked)); > emit_gri(as, XG_ARITHi(XOg_OR), RID_ECX, > > > > > > > > -- > > Best regards, > > IM > > I've fixed your other comments in [1]. Great, thanks! > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018866.html > > -- > Best regards, > Sergey Kaplun -- Best regards, IM