From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 7ECE8445320 for ; Fri, 24 Jul 2020 15:00:08 +0300 (MSK) Date: Fri, 24 Jul 2020 15:00:00 +0300 From: Sergey Kaplun Message-ID: <20200724120000.GB4086@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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Hi Igor! Thanks for the review! On 23.07.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! Please consider my comments below. > > On 21.07.20, Sergey Kaplun wrote: > > We still don't have strict prefixes, so I propose to use 'core' for this > patch. Does it look fine to you? Feel free to propose your own one. Maybe we should use 'misc' for this kind of patches? > > This patch adds counters for: > > - amount of table, cdata and udata objects, > > - GC invokations by GC states, > > - strhash misses or hits, > > - amount of freed or allocated memory, > > - amount of aborted traces and restored snapshots. > > > > C and Lua API will be added in the next patch. > > OK, let's make the commit message a bit clearer: > | core: introduce various platform metrics > | > | 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 > | > | Interfaces to obtain these metrics via both Lua and C API are > | introduced in the next patch. > > Feel free to change it on your own. Thanks! It looks fine for me. > > > > Part of tarantool/tarantool#5187 > > --- > > src/lj_cdata.c | 2 ++ > > src/lj_cdata.h | 2 ++ > > src/lj_gc.c | 4 ++++ > > src/lj_gc.h | 6 +----- > > src/lj_jit.h | 7 +++++++ > > src/lj_obj.h | 25 +++++++++++++++++++++++++ > > src/lj_snap.c | 1 + > > src/lj_state.c | 13 ++++++++++++- > > src/lj_str.c | 5 +++++ > > src/lj_tab.c | 2 ++ > > src/lj_trace.c | 5 ++++- > > src/lj_udata.c | 2 ++ > > 12 files changed, 67 insertions(+), 7 deletions(-) > > > > > > > diff --git a/src/lj_jit.h b/src/lj_jit.h > > index 7eb3d2a..960a02b 100644 > > --- a/src/lj_jit.h > > +++ b/src/lj_jit.h > > @@ -475,6 +475,13 @@ typedef struct jit_State { > > size_t szmcarea; /* Size of current mcode area. */ > > size_t szallmcarea; /* Total size of all allocated mcode areas. */ > > > > + size_t nsnaprestore; /* Overall number of snap restores for all traces > > + ** "belonging" to the given jit_State > > + ** since the last call to luaM_metrics(). */ > > + size_t ntraceabort; /* Overall number of abort traces > > + ** "belonging" to the given jit_State > > + ** since the last call to luaM_metrics(). */ > > Code indentation is really strange here, like you mixed LuaJIT and uJIT > styles. Besides, I guess the remark regarding the field reset is wrong: > * These fields are not reset in this patch. > * is introduced only in the following patch but you've > already mentioned it here. My bad! Thanks for this catch. I will fix indentation in the next patch. Naming-wise, as you can see at [1] we shouldn't use metrics since last call. So comment should gone. > > + > > TValue errinfo; /* Additional info element for trace errors. */ > > > > #if LJ_HASPROFILE > > diff --git a/src/lj_obj.h b/src/lj_obj.h > > index f368578..1fee04f 100644 > > --- a/src/lj_obj.h > > +++ b/src/lj_obj.h > > @@ -571,13 +571,28 @@ 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, > > + GCSpropagate, > > + GCSatomic, > > + GCSsweepstring, > > + GCSsweep, > > + GCSfinalize, > > + GCSlast > > Minor: GCSMAX looks better, doesn't it? Feel free to ignore. Really, GCSlast is unreachable. GCSmax for this kind of naming is good enough. GCSunreachable also looks nice. What do you think? > > +}; > > + > > typedef struct GCState { > > GCSize total; /* Memory currently allocated. */ > > + size_t freed; /* Memory freed since last luaM_metrics() call. */ > > + size_t allocated; /* Memory allocated since last luaM_metrics() call. */ > > Well, why field type differs from and fields > type? And again, remark about can be dropped (see above). GCSize for total memfory usage is defines by LJ_GC64 macro definition. | #if LJ_GC64 | typedef uint64_t GCSize; | #else | typedef uint32_t GCSize; | #endif But summarized allocation size can be many times over maximum memory size). So these field types should be independet. > > GCSize threshold; /* Memory threshold. */ > > uint8_t currentwhite; /* Current white color. */ > > uint8_t state; /* GC state. */ > > uint8_t nocdatafin; /* No cdata finalizer called. */ > > uint8_t unused2; > > + size_t state_count[GCSlast]; /* Count of GC invocations with different states > > + ** since previous call of luaM_metrics(). */ > > Again, a mess with whitespace. Furthermore, I propose to reword this > comment the following way: > | /* Count of incremental GC steps per state */ Ok, no problems! > > MSize sweepstr; /* Sweep position in string table. */ > > GCRef root; /* List of all collectable objects. */ > > MRef sweep; /* Sweep position in root list. */ > > @@ -589,6 +604,12 @@ typedef struct GCState { > > GCSize estimate; /* Estimate of memory actually in use. */ > > MSize stepmul; /* Incremental GC step granularity. */ > > MSize pause; /* Pause between successive GC cycles. */ > > + > > + size_t tabnum; /* Current amount of table objects. */ > > + size_t udatanum; /* Current amount of udata objects. */ > > Minor: These comments are a bit misleading. "Current" sounds like the > metric is reset somewhen. But you're talking about "alive" objects of > the particular type. Yes, formally (in GC terms) some of them are not > alive, but are still allocated (so called "dead") objects. So the term > "alive" is also a misleading one. I guess you can freely reword the > comments the following way: > | /* Number of allocated objects */ > I think that "allocated" is unnecessarily. We can omit it easily. I should reword the comments as | /* Number of objects. */ or | /* Amount of objects. */ and it seems enough. > > +#ifdef LJ_HASFFI > > + size_t cdatanum; /* Current amount of cdata objects. */ > > 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. My fault! Ok, I will try to implement it at this week. > > +#endif > > } GCState; > > > > /* Global state, shared by all threads of a Lua universe. */ > > @@ -602,6 +623,10 @@ typedef struct global_State { > > BloomFilter next[2]; > > } strbloom; > > #endif > > + size_t strhash_hit; /* New string has been found in the storage > > + ** since last luaM_metrics() call. */ > > + size_t strhash_miss; /* New string has been added to the storage > > + ** since last luaM_metrics() call. */ > > Well, such indentation is closer to the desired one, but I would use two > spaces prior to these tabs. What do you think? Also please, don't forget > about remark. Sorry, but ^\s+\t+\s+ stylistics looks very bad. In *.c files Mike use exactly ^\t+ style for code, so we have to use the same for comments. Thoughts? > > lua_Alloc allocf; /* Memory allocator. */ > > void *allocd; /* Memory allocator data. */ > > GCState gc; /* Garbage collector. */ > > > > > diff --git a/src/lj_state.c b/src/lj_state.c > > index 632dd07..b71ebae 100644 > > --- a/src/lj_state.c > > +++ b/src/lj_state.c > > @@ -204,6 +204,16 @@ LUA_API lua_State *lua_newstate(lua_Alloc f, void *ud) > > setgcref(g->uvhead.prev, obj2gco(&g->uvhead)); > > setgcref(g->uvhead.next, obj2gco(&g->uvhead)); > > g->strmask = ~(MSize)0; > > + > > + memset(g->gc.state_count, 0, GCSlast * sizeof(g->gc.state_count[0])); > > + g->strhash_hit = 0; > > + g->strhash_miss = 0; > > + g->gc.tabnum = 0; > > + g->gc.udatanum = 0; > > +#if LJ_HASFFI > > + g->gc.cdatanum = 0; > > +#endif > > + > > It looks like all assignments above are excess: is filled > with zeros, points to the corresponding chunk, is > allocated within , ergo also initialized with zeros. Feel free to > drop this hunk. Nice catch! Thanks! > > setnilV(registry(L)); > > setnilV(&g->nilnode.val); > > setnilV(&g->nilnode.key); > > @@ -214,7 +224,8 @@ LUA_API lua_State *lua_newstate(lua_Alloc f, void *ud) > > g->gc.state = GCSpause; > > setgcref(g->gc.root, obj2gco(L)); > > setmref(g->gc.sweep, &g->gc.root); > > - g->gc.total = sizeof(GG_State); > > + g->gc.allocated = g->gc.total = sizeof(GG_State); > > + g->gc.freed = 0; > > Ditto for g->gc.freed. Sure! > > > g->gc.pause = LUAI_GCPAUSE; > > g->gc.stepmul = LUAI_GCMUL; > > lj_dispatch_init((GG_State *)L); > > > > > -- > > 2.24.1 > > > > -- > Best regards, > IM [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018845.html -- Best regards, Sergey Kaplun