Re: [Tarantool-patches] [PATCH 1/2] metrics: add counters for metrics interested in

[Igor Munkin <imun@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:
> > 
> <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?

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 <int32_t> type
* IIRC, integral literals are 32 bit integer types

There is also an additional check within <emit_gmroi> 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,
> 
> > 
> <snipped>
> > 
> > -- 
> > 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