Re: [Tarantool-patches] [PATCH v2 1/2] core: introduce various platform metrics

[Sergey Ostanevich <sergos@tarantool.org>]

On 26 авг 17:48, 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).
> 
> > 
> > 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(-)
> > 
> 
> <snipped>
> 
> > 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.
> 
> > +
> >    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
> 
> <snipped>
> 
> > @@ -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.
> 
> >    MSize sweepstr;	/* Sweep position in string table. */
> >    GCRef root;		/* List of all collectable objects. */
> >    MRef sweep;		/* Sweep position in root list. */
> 
> <snipped>
> 
> > @@ -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/.
> 
> > +  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. */
> 
> <snipped>
> 
> >  
> > -- 
> > 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?

AFAU the CNEW IR appeared in src/lj_asm_x86.h so I didn't get what
preprocessor condition do you mean.
Also, the inconsistency with the allocated CDATA counter will appear
anyways, so it needs to be implemented for other targets also. Looks
like a not-so-big deal?
BTW, AlexanderTi made a successful build and a little less successful
run on an ARM emulator that took some reasonable time. So there's a 
way to test the change for ARM. Both PPC and MIPS I suppose would be
just fine to test for successful build.

> 
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019012.html
> 
> -- 
> Best regards,
> IM