From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <skaplun@tarantool.org>
Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105])
 (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 B31D8445320
 for <tarantool-patches@dev.tarantool.org>;
 Mon, 27 Jul 2020 05:25:53 +0300 (MSK)
Date: Mon, 27 Jul 2020 05:25:46 +0300
From: Sergey Kaplun <skaplun@tarantool.org>
Message-ID: <20200727022546.GA11579@root>
References: <20200721113451.25817-1-skaplun@tarantool.org>
 <20200721113451.25817-2-skaplun@tarantool.org>
 <20200723201219.GQ18920@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200723201219.GQ18920@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 1/2] metrics: add counters for
	metrics interested in
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

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:
> 
<snipped>
> 
> 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 <<EOF
> | 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 <<EOF
> | 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 <lj_cdata_newv> call is emitted and
> the counters are fine, but for other cdata types JIT compiler emits just
> a <lj_mem_newgco> 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?

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.
+  ** Increment cdatanum counter by address directly.
+  */
+  emit_gmroi(as, XG_ARITHi(XOg_ADD), RID_NONE,
+        ptr2addr(&J2G(as->J)->gc.cdatanum), (int32_t)1);
+
   /* 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,

> 
<snipped>
> 
> -- 
> Best regards,
> IM

I've fixed your other comments in [1].

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018866.html

-- 
Best regards,
Sergey Kaplun