From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 1766D469719 for ; Wed, 7 Oct 2020 17:21:42 +0300 (MSK) Date: Wed, 7 Oct 2020 17:11:06 +0300 From: Igor Munkin Message-ID: <20201007141106.GP18920@tarantool.org> References: <2280bc3a2e32356455c3aebae711bafe2c4332f5.1601878708.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2280bc3a2e32356455c3aebae711bafe2c4332f5.1601878708.git.skaplun@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 1/2] core: introduce various platform metrics 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, Thanks for the patch! Please consider my comments below. On 05.10.20, Sergey Kaplun wrote: > This patch introduces the following counters: > - overall amount of allocated tables, cdata and udata objects > - number of incremental GC steps grouped by GC state > - number of string hashes hits and misses > - amount of allocated and freed memory > - number of trace aborts and restored snapshots Looks like you forgot a number of alive traces. > > Interfaces to obtain these metrics via both Lua and C API are > introduced in the next patch. > > Part of tarantool/tarantool#5187 > --- > src/lj_asm.c | 2 ++ > src/lj_asm_arm.h | 7 +++++++ > src/lj_asm_arm64.h | 7 +++++++ > src/lj_asm_mips.h | 4 ++++ > src/lj_asm_ppc.h | 3 +++ > src/lj_asm_x86.h | 4 ++++ > src/lj_cdata.c | 2 ++ > src/lj_cdata.h | 2 ++ > src/lj_gc.c | 4 ++++ > src/lj_gc.h | 6 +----- > src/lj_jit.h | 3 +++ > src/lj_obj.h | 28 +++++++++++++++++++++++++--- > src/lj_snap.c | 1 + > src/lj_state.c | 2 +- > src/lj_str.c | 5 +++++ > src/lj_tab.c | 2 ++ > src/lj_trace.c | 6 +++++- > src/lj_udata.c | 2 ++ > 18 files changed, 80 insertions(+), 10 deletions(-) > > diff --git a/src/lj_asm.c b/src/lj_asm.c > index c2cf5a9..8fb3ccd 100644 > --- a/src/lj_asm.c > +++ b/src/lj_asm.c > @@ -2273,6 +2273,7 @@ void lj_asm_trace(jit_State *J, GCtrace *T) > as->J = J; > as->T = T; > J->curfinal = lj_trace_alloc(J->L, T); /* This copies the IR, too. */ > + J->tracenum++; Why didn't you simply put this increment right to as you did for decrement and ? > as->flags = J->flags; > as->loopref = J->loopref; > as->realign = NULL; > @@ -2379,6 +2380,7 @@ void lj_asm_trace(jit_State *J, GCtrace *T) > lj_trace_free(J2G(J), J->curfinal); > J->curfinal = NULL; /* In case lj_trace_alloc() OOMs. */ > J->curfinal = lj_trace_alloc(J->L, T); > + J->tracenum++; Ditto. > as->realign = NULL; > } > > diff --git a/src/lj_asm_mips.h b/src/lj_asm_mips.h > index affe7d8..f4b4b5d 100644 > --- a/src/lj_asm_mips.h > +++ b/src/lj_asm_mips.h > @@ -1473,11 +1473,15 @@ static void asm_cnew(ASMState *as, IRIns *ir) > return; > } > > + /* Code incrementing cdatanum is sparse to avoid mips data hazards. */ Side note: LOL, here you are :) > + emit_setgl(as, RID_RET+2, gc.cdatanum); Well, I glanced a MIPS register-usage convention and AFAICS $4 register (RID_RET + 2) is a general-purpose (i.e. doesn't store 0 or preserved by kernel) caller-safe one. Ergo it should be allocated it in a proper way from scratch set, shouldn't it? > /* Initialize gct and ctypeid. lj_mem_newgco() already sets marked. */ > emit_tsi(as, MIPSI_SB, RID_RET+1, RID_RET, offsetof(GCcdata, gct)); > emit_tsi(as, MIPSI_SH, RID_TMP, RID_RET, offsetof(GCcdata, ctypeid)); > + emit_tsi(as, MIPSI_AADDIU, RID_RET+2, RID_RET+2, 1); > emit_ti(as, MIPSI_LI, RID_RET+1, ~LJ_TCDATA); > emit_ti(as, MIPSI_LI, RID_TMP, id); /* Lower 16 bit used. Sign-ext ok. */ > + emit_getgl(as, RID_RET+2, gc.cdatanum); > args[0] = ASMREF_L; /* lua_State *L */ > args[1] = ASMREF_TMP1; /* MSize size */ > asm_gencall(as, ci, args); > diff --git a/src/lj_obj.h b/src/lj_obj.h > index f368578..600b68d 100644 > --- a/src/lj_obj.h > +++ b/src/lj_obj.h > @@ -571,6 +571,17 @@ typedef enum { > #define basemt_obj(g, o) ((g)->gcroot[GCROOT_BASEMT+itypemap(o)]) > #define mmname_str(g, mm) (strref((g)->gcroot[GCROOT_MMNAME+(mm)])) > > +/* Garbage collector states. Order matters. */ > +enum { > + GCSpause, /* Start a GC cycle and mark the root set.*/ > + GCSpropagate, /* One gray object is processed. */ > + GCSatomic, /* Atomic transition from mark to sweep phase. */ > + GCSsweepstring, /* Sweep one chain of strings. */ > + GCSsweep, /* Sweep few objects from root. */ > + GCSfinalize, /* Finalize one userdata or cdata object. */ > + GCSmax Adjust the comments considering the code nearby. > +}; > + > typedef struct GCState { > GCSize total; /* Memory currently allocated. */ > GCSize threshold; /* Memory threshold. */ > @@ -602,22 +622,24 @@ typedef struct global_State { > BloomFilter next[2]; > } strbloom; > #endif > + size_t strhash_hit; /* Strings amount found in string hash. */ > + size_t strhash_miss; /* Strings amount allocated and put into string hash. */ > lua_Alloc allocf; /* Memory allocator. */ > void *allocd; /* Memory allocator data. */ > GCState gc; /* Garbage collector. */ > - volatile int32_t vmstate; /* VM state or current JIT code trace number. */ > SBuf tmpbuf; /* Temporary string buffer. */ > GCstr strempty; /* Empty string. */ > uint8_t stremptyz; /* Zero terminator of empty string. */ > uint8_t hookmask; /* Hook mask. */ > uint8_t dispatchmode; /* Dispatch mode. */ > uint8_t vmevmask; /* VM event mask. */ > + int32_t hookcount; /* Instruction hook countdown. */ > GCRef mainthref; /* Link to main thread. */ > TValue registrytv; /* Anchor for registry. */ > - TValue tmptv, tmptv2; /* Temporary TValues. */ > + TValue tmptv2, tmptv; /* Temporary TValues. */ > Node nilnode; /* Fallback 1-element hash part (nil key and value). */ > GCupval uvhead; /* Head of double-linked list of all open upvalues. */ > - int32_t hookcount; /* Instruction hook countdown. */ > + volatile int32_t vmstate; /* VM state or current JIT code trace number. */ There is no a single word in the commit message regarding this unclear change. Please drop a sentence about the rationale for this reordering. > int32_t hookcstart; /* Start count for instruction hook counter. */ > lua_Hook hookf; /* Hook function. */ > lua_CFunction wrapf; /* Wrapper for C function calls. */ > -- > 2.28.0 > -- Best regards, IM