From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 6F913430409 for ; Thu, 3 Sep 2020 15:52:02 +0300 (MSK) Date: Thu, 3 Sep 2020 15:51:45 +0300 From: Sergey Kaplun Message-ID: <20200903125145.GA30957@root> References: <35a19def79a9cbc46dabdfa579869af9e4e589fb.1595794764.git.skaplun@tarantool.org> <20200826144837.GA18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200826144837.GA18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/2] core: introduce various platform metrics List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, On 26.08.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! It looks OK in general, except several nits, I > left below. > > On 26.07.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 > > Typo: we usually use whitespace prior to the list bullets (as you did in > the previous version). Sure, thanks! > > > > > Interfaces to obtain these metrics via both Lua and C API are > > introduced in the next patch. > > > > 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 | 3 +++ > > src/lj_obj.h | 22 ++++++++++++++++++++++ > > src/lj_snap.c | 1 + > > src/lj_state.c | 2 +- > > src/lj_str.c | 5 +++++ > > src/lj_tab.c | 2 ++ > > src/lj_trace.c | 5 ++++- > > src/lj_udata.c | 2 ++ > > 12 files changed, 49 insertions(+), 7 deletions(-) > > > > > > > diff --git a/src/lj_jit.h b/src/lj_jit.h > > index 7eb3d2a..90c1914 100644 > > --- a/src/lj_jit.h > > +++ b/src/lj_jit.h > > @@ -475,6 +475,9 @@ 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 this jit_State. */ > > + size_t ntraceabort; /* Overall number of abort traces for this jit_State. */ > > Why did you emphasize that the counters relate to *this jit_State*? > There are no such mentions elsewhere. Well, I suppose that they can be omitted. > > > + > > 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..18df173 100644 > > --- a/src/lj_obj.h > > +++ b/src/lj_obj.h > > > > > @@ -578,6 +589,9 @@ typedef struct GCState { > > uint8_t state; /* GC state. */ > > uint8_t nocdatafin; /* No cdata finalizer called. */ > > uint8_t unused2; > > + size_t freed; /* Total amount of freed memory. */ > > + size_t allocated; /* Total amount of allocated memory. */ > > + size_t state_count[GCSmax]; /* Count of incremental GC steps per state. */ > > One more time: consider the structure alignment and reorder the > introduced fields to avoid excess padding. Sure! I will add all fields to the end of structure. > > > MSize sweepstr; /* Sweep position in string table. */ > > GCRef root; /* List of all collectable objects. */ > > MRef sweep; /* Sweep position in root list. */ > > > > > @@ -602,6 +622,8 @@ typedef struct global_State { > > BloomFilter next[2]; > > } strbloom; > > #endif > > + size_t strhash_hit; /* Strings amount founded in string hash. */ > > Typo: s/founded/found/. Thanks! > > > + 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. */ > > > > > > > -- > > 2.24.1 > > > > Furthermore, please address the comments I left regarding the patch > you've made for CNEW IR[1] and squash it with this one. > > Sergos, do we need other JIT architectures to be patched in scope of > this series or Sergey can just add the corresponding preprocessor > condition to stub the issue for now? > > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019012.html > I'll add several architectures support with the next series. > -- > Best regards, > IM -- Best regards, Sergey Kaplun