[Tarantool-patches] [PATCH v2 0/2] Implement LuaJIT platform metrics

[Tarantool-patches] [PATCH v2 0/2] Implement LuaJIT platform metrics

[Sergey Kaplun]

The series consists of 2 patches. The first one adds corresponding
counters to LuaJIT internal structures. The second provides C and Lua
API using this counters to collect metrics.

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5187-luajit-metrics
Issue: https://github.com/tarantool/tarantool/issues/5187

Changes in v2:
- Fixed naming and comments
- Fixed padding in struct GCState
- Dropped unnecessary initialisations inside lua_newstate()
- Avoided flushing any of metrics after each call of luaM_metrics()

Sergey Kaplun (2):
  core: introduce various platform metrics
  metrics: add C and Lua API

 Makefile           |  2 +-
 src/Makefile       |  5 ++--
 src/Makefile.dep   |  3 ++
 src/lib_init.c     |  2 ++
 src/lib_misc.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 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_misc_capi.c | 59 ++++++++++++++++++++++++++++++++++++
 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 ++
 src/lmisclib.h     | 61 +++++++++++++++++++++++++++++++++++++
 src/luaconf.h      |  1 +
 20 files changed, 254 insertions(+), 10 deletions(-)
 create mode 100644 src/lib_misc.c
 create mode 100644 src/lj_misc_capi.c
 create mode 100644 src/lmisclib.h

-- 
2.24.1

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

[Sergey Kaplun]

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.

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_cdata.c b/src/lj_cdata.c
index 68e16d7..0dd8553 100644
--- a/src/lj_cdata.c
+++ b/src/lj_cdata.c
@@ -46,6 +46,7 @@ GCcdata *lj_cdata_newv(lua_State *L, CTypeID id, CTSize sz, CTSize align)
   cd->marked |= 0x80;
   cd->gct = ~LJ_TCDATA;
   cd->ctypeid = id;
+  g->gc.cdatanum++;
   return cd;
 }
 
@@ -82,6 +83,7 @@ void LJ_FASTCALL lj_cdata_free(global_State *g, GCcdata *cd)
   } else {
     lj_mem_free(g, memcdatav(cd), sizecdatav(cd));
   }
+  g->gc.cdatanum--;
 }
 
 void lj_cdata_setfin(lua_State *L, GCcdata *cd, GCobj *obj, uint32_t it)
diff --git a/src/lj_cdata.h b/src/lj_cdata.h
index 5bb0f5d..66b023b 100644
--- a/src/lj_cdata.h
+++ b/src/lj_cdata.h
@@ -45,6 +45,7 @@ static LJ_AINLINE GCcdata *lj_cdata_new(CTState *cts, CTypeID id, CTSize sz)
   cd = (GCcdata *)lj_mem_newgco(cts->L, sizeof(GCcdata) + sz);
   cd->gct = ~LJ_TCDATA;
   cd->ctypeid = ctype_check(cts, id);
+  G(cts->L)->gc.cdatanum++;
   return cd;
 }
 
@@ -54,6 +55,7 @@ static LJ_AINLINE GCcdata *lj_cdata_new_(lua_State *L, CTypeID id, CTSize sz)
   GCcdata *cd = (GCcdata *)lj_mem_newgco(L, sizeof(GCcdata) + sz);
   cd->gct = ~LJ_TCDATA;
   cd->ctypeid = id;
+  G(L)->gc.cdatanum++;
   return cd;
 }
 
diff --git a/src/lj_gc.c b/src/lj_gc.c
index 1c8e6ce..44c8aa1 100644
--- a/src/lj_gc.c
+++ b/src/lj_gc.c
@@ -634,6 +634,7 @@ static void atomic(global_State *g, lua_State *L)
 static size_t gc_onestep(lua_State *L)
 {
   global_State *g = G(L);
+  g->gc.state_count[g->gc.state]++;
   switch (g->gc.state) {
   case GCSpause:
     gc_mark_start(g);  /* Start a new GC cycle by marking all GC roots. */
@@ -857,6 +858,8 @@ void *lj_mem_realloc(lua_State *L, void *p, GCSize osz, GCSize nsz)
   lua_assert((nsz == 0) == (p == NULL));
   lua_assert(checkptrGC(p));
   g->gc.total = (g->gc.total - osz) + nsz;
+  g->gc.allocated += nsz;
+  g->gc.freed += osz;
   return p;
 }
 
@@ -869,6 +872,7 @@ void * LJ_FASTCALL lj_mem_newgco(lua_State *L, GCSize size)
     lj_err_mem(L);
   lua_assert(checkptrGC(o));
   g->gc.total += size;
+  g->gc.allocated += size;
   setgcrefr(o->gch.nextgc, g->gc.root);
   setgcref(g->gc.root, o);
   newwhite(g, o);
diff --git a/src/lj_gc.h b/src/lj_gc.h
index 669bbe9..2051220 100644
--- a/src/lj_gc.h
+++ b/src/lj_gc.h
@@ -8,11 +8,6 @@
 
 #include "lj_obj.h"
 
-/* Garbage collector states. Order matters. */
-enum {
-  GCSpause, GCSpropagate, GCSatomic, GCSsweepstring, GCSsweep, GCSfinalize
-};
-
 /* Bitmasks for marked field of GCobj. */
 #define LJ_GC_WHITE0	0x01
 #define LJ_GC_WHITE1	0x02
@@ -117,6 +112,7 @@ LJ_FUNC void *lj_mem_grow(lua_State *L, void *p,
 static LJ_AINLINE void lj_mem_free(global_State *g, void *p, size_t osize)
 {
   g->gc.total -= (GCSize)osize;
+  g->gc.freed += osize;
   g->allocf(g->allocd, p, osize, 0);
 }
 
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. */
+
   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
@@ -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,
+  GCSpropagate,
+  GCSatomic,
+  GCSsweepstring,
+  GCSsweep,
+  GCSfinalize,
+  GCSmax
+};
+
 typedef struct GCState {
   GCSize total;		/* Memory currently allocated. */
   GCSize threshold;	/* Memory threshold. */
@@ -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. */
   MSize sweepstr;	/* Sweep position in string table. */
   GCRef root;		/* List of all collectable objects. */
   MRef sweep;		/* Sweep position in root list. */
@@ -589,6 +603,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;	/* Amount of allocated table objects. */
+  size_t udatanum;	/* Amount of allocated udata objects. */
+#ifdef LJ_HASFFI
+  size_t cdatanum;	/* Amount of allocated cdata objects. */
+#endif
 } GCState;
 
 /* Global state, shared by all threads of a Lua universe. */
@@ -602,6 +622,8 @@ typedef struct global_State {
     BloomFilter next[2];
   } strbloom;
 #endif
+  size_t strhash_hit;	/* Strings amount founded 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. */
diff --git a/src/lj_snap.c b/src/lj_snap.c
index 7554caf..4cf27e7 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -904,6 +904,7 @@ const BCIns *lj_snap_restore(jit_State *J, void *exptr)
     L->top = frame + snap->nslots;
     break;
   }
+  J->nsnaprestore++;
   return pc;
 }
 
diff --git a/src/lj_state.c b/src/lj_state.c
index 632dd07..1d9c628 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c
@@ -214,7 +214,7 @@ 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.pause = LUAI_GCPAUSE;
   g->gc.stepmul = LUAI_GCMUL;
   lj_dispatch_init((GG_State *)L);
diff --git a/src/lj_str.c b/src/lj_str.c
index 24e90ca..8ff955e 100644
--- a/src/lj_str.c
+++ b/src/lj_str.c
@@ -222,6 +222,7 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
                       str_fastcmp(str, strdata(sx), len) == 0) {
 	/* Resurrect if dead. Can only happen with fixstring() (keywords). */
 	if (isdead(g, o)) flipwhite(o);
+	g->strhash_hit++;
 	return sx;  /* Return existing string. */
       }
       o = gcnext(o);
@@ -234,6 +235,7 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
                       memcmp(str, strdata(sx), len) == 0) {
 	/* Resurrect if dead. Can only happen with fixstring() (keywords). */
 	if (isdead(g, o)) flipwhite(o);
+	g->strhash_hit++;
 	return sx;  /* Return existing string. */
       }
       o = gcnext(o);
@@ -266,6 +268,7 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
 	    if (sx->hash == fh && sx->len == len && str_fastcmp(str, strdata(sx), len) == 0) {
 	      /* Resurrect if dead. Can only happen with fixstring() (keywords). */
 	      if (isdead(g, o)) flipwhite(o);
+	      g->strhash_hit++;
 	      return sx;  /* Return existing string. */
 	    }
 	    o = gcnext(o);
@@ -276,6 +279,7 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
 	    if (sx->hash == fh && sx->len == len && memcmp(str, strdata(sx), len) == 0) {
 	      /* Resurrect if dead. Can only happen with fixstring() (keywords). */
 	      if (isdead(g, o)) flipwhite(o);
+	      g->strhash_hit++;
 	      return sx;  /* Return existing string. */
 	    }
 	    o = gcnext(o);
@@ -293,6 +297,7 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
     }
   }
 #endif
+  g->strhash_miss++;
   /* Nope, create a new string. */
   s = lj_mem_newt(L, sizeof(GCstr)+len+1, GCstr);
   newwhite(g, s);
diff --git a/src/lj_tab.c b/src/lj_tab.c
index ff216f3..c5f358e 100644
--- a/src/lj_tab.c
+++ b/src/lj_tab.c
@@ -141,6 +141,7 @@ static GCtab *newtab(lua_State *L, uint32_t asize, uint32_t hbits)
   }
   if (hbits)
     newhpart(L, t, hbits);
+  G(L)->gc.tabnum++;
   return t;
 }
 
@@ -240,6 +241,7 @@ void LJ_FASTCALL lj_tab_free(global_State *g, GCtab *t)
     lj_mem_free(g, t, sizetabcolo((uint32_t)t->colo & 0x7f));
   else
     lj_mem_freet(g, t);
+  g->gc.tabnum--;
 }
 
 /* -- Table resizing ------------------------------------------------------ */
diff --git a/src/lj_trace.c b/src/lj_trace.c
index d85b47f..b17bde3 100644
--- a/src/lj_trace.c
+++ b/src/lj_trace.c
@@ -538,8 +538,10 @@ static int trace_downrec(jit_State *J)
   /* Restart recording at the return instruction. */
   lua_assert(J->pt != NULL);
   lua_assert(bc_isret(bc_op(*J->pc)));
-  if (bc_op(*J->pc) == BC_RETM)
+  if (bc_op(*J->pc) == BC_RETM) {
+    J->ntraceabort++;
     return 0;  /* NYI: down-recursion with RETM. */
+  }
   J->parent = 0;
   J->exitno = 0;
   J->state = LJ_TRACE_RECORD;
@@ -616,6 +618,7 @@ static int trace_abort(jit_State *J)
     return trace_downrec(J);
   else if (e == LJ_TRERR_MCODEAL)
     lj_trace_flushall(L);
+  J->ntraceabort++;
   return 0;
 }
 
diff --git a/src/lj_udata.c b/src/lj_udata.c
index bd0321b..70c722a 100644
--- a/src/lj_udata.c
+++ b/src/lj_udata.c
@@ -24,11 +24,13 @@ GCudata *lj_udata_new(lua_State *L, MSize sz, GCtab *env)
   /* Chain to userdata list (after main thread). */
   setgcrefr(ud->nextgc, mainthread(g)->nextgc);
   setgcref(mainthread(g)->nextgc, obj2gco(ud));
+  g->gc.udatanum++;
   return ud;
 }
 
 void LJ_FASTCALL lj_udata_free(global_State *g, GCudata *ud)
 {
+  g->gc.udatanum--;
   lj_mem_free(g, ud, sizeudata(ud));
 }
 
-- 
2.24.1

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

[Igor Munkin]

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?


[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019012.html

-- 
Best regards,
IM

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

[Sergey Ostanevich]

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

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

[Igor Munkin]

Sergos,

On 26.08.20, Sergey Ostanevich wrote:
> On 26 авг 17:48, Igor Munkin wrote:

<snipped>

> > 
> > 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.

Well, I clarify my point a bit: Sergey can implement cdata counter only
for x86 platform both in compiler and interpreter. Other platforms will
report nothing (i.e. zeros) for this metric and the issue can be fixed
on demand.

> 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?

Yes, but I believe testing will take much more time than implementation.

> 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.

OK then, so I guess Sergey can make the patch for other arches as well.

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

-- 
Best regards,
IM

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

[Sergey Kaplun]

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(-)
> > 
> 
> <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.

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
> 
> <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.

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. */
> 
> <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/.

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. */
> 
> <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?
> 
> 
> [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

[Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API

[Sergey Kaplun]

This patch adds C and Lua API for luajit metrics. Metrics include GC
statistic, JIT information, strhash hit/miss counters and amount of
different objects. C API provides with aditional header <lmisclib.h>
that contains structure `luam_metrics` and `luaM_metrics` function
definition. Lua userspace expanded with builtin library (named `misc`)
with corresponding method `getmetrics`.

Part of tarantool/tarantool#5187
---
 Makefile           |  2 +-
 src/Makefile       |  5 ++--
 src/Makefile.dep   |  3 ++
 src/lib_init.c     |  2 ++
 src/lib_misc.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 src/lj_misc_capi.c | 59 ++++++++++++++++++++++++++++++++++++
 src/lmisclib.h     | 61 +++++++++++++++++++++++++++++++++++++
 src/luaconf.h      |  1 +
 8 files changed, 205 insertions(+), 3 deletions(-)
 create mode 100644 src/lib_misc.c
 create mode 100644 src/lj_misc_capi.c
 create mode 100644 src/lmisclib.h

diff --git a/Makefile b/Makefile
index 0f93308..4a56917 100644
--- a/Makefile
+++ b/Makefile
@@ -84,7 +84,7 @@ FILE_A= libluajit.a
 FILE_SO= libluajit.so
 FILE_MAN= luajit.1
 FILE_PC= luajit.pc
-FILES_INC= lua.h lualib.h lauxlib.h luaconf.h lua.hpp luajit.h
+FILES_INC= lua.h lualib.h lauxlib.h luaconf.h lua.hpp luajit.h lmisclib.h
 FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
 	      dis_x86.lua dis_x64.lua dis_arm.lua dis_arm64.lua \
 	      dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \
diff --git a/src/Makefile b/src/Makefile
index 827d4a4..fac69bc 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -480,13 +480,14 @@ LJVM_BOUT= $(LJVM_S)
 LJVM_MODE= elfasm
 
 LJLIB_O= lib_base.o lib_math.o lib_bit.o lib_string.o lib_table.o \
-	 lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o
+	 lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o \
+	 lib_misc.o
 LJLIB_C= $(LJLIB_O:.o=.c)
 
 LJCORE_O= lj_gc.o lj_err.o lj_char.o lj_bc.o lj_obj.o lj_buf.o \
 	  lj_str.o lj_tab.o lj_func.o lj_udata.o lj_meta.o lj_debug.o \
 	  lj_state.o lj_dispatch.o lj_vmevent.o lj_vmmath.o lj_strscan.o \
-	  lj_strfmt.o lj_strfmt_num.o lj_api.o lj_profile.o \
+	  lj_strfmt.o lj_strfmt_num.o lj_api.o lj_misc_capi.o lj_profile.o \
 	  lj_lex.o lj_parse.o lj_bcread.o lj_bcwrite.o lj_load.o \
 	  lj_ir.o lj_opt_mem.o lj_opt_fold.o lj_opt_narrow.o \
 	  lj_opt_dce.o lj_opt_loop.o lj_opt_split.o lj_opt_sink.o \
diff --git a/src/Makefile.dep b/src/Makefile.dep
index 2b1cb5e..1c3d8bd 100644
--- a/src/Makefile.dep
+++ b/src/Makefile.dep
@@ -41,6 +41,9 @@ lib_string.o: lib_string.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
 lib_table.o: lib_table.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
  lj_def.h lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h \
  lj_tab.h lj_ff.h lj_ffdef.h lj_lib.h lj_libdef.h
+lib_misc.o: lib_misc.c lua.h lmisclib.h luaconf.h lj_obj.h lj_tab.h lj_str.h \
+ lj_arch.h lj_lib.h lj_vm.h lj_libdef.h
+lj_misc_capi.o: lj_misc_capi.c lua.h lmisclib.h luaconf.h lj_obj.h lj_arch.h
 lj_alloc.o: lj_alloc.c lj_def.h lua.h luaconf.h lj_arch.h lj_alloc.h
 lj_api.o: lj_api.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_gc.h \
  lj_err.h lj_errmsg.h lj_debug.h lj_str.h lj_tab.h lj_func.h lj_udata.h \
diff --git a/src/lib_init.c b/src/lib_init.c
index 2ed370e..664aa7d 100644
--- a/src/lib_init.c
+++ b/src/lib_init.c
@@ -12,6 +12,7 @@
 #include "lua.h"
 #include "lauxlib.h"
 #include "lualib.h"
+#include "lmisclib.h"
 
 #include "lj_arch.h"
 
@@ -26,6 +27,7 @@ static const luaL_Reg lj_lib_load[] = {
   { LUA_DBLIBNAME,	luaopen_debug },
   { LUA_BITLIBNAME,	luaopen_bit },
   { LUA_JITLIBNAME,	luaopen_jit },
+  { LUAM_MISCLIBNAME,	luaopen_misc },
   { NULL,		NULL }
 };
 
diff --git a/src/lib_misc.c b/src/lib_misc.c
new file mode 100644
index 0000000..ef20921
--- /dev/null
+++ b/src/lib_misc.c
@@ -0,0 +1,75 @@
+/*
+ * Lua interface to tarantool-specific extensions to the public Lua/C API.
+ *
+ * Major portions taken verbatim or adapted from the LuaVela interpreter.
+ * Copyright (C) 2015-2019 IPONWEB Ltd.
+ */
+
+#define lib_misc_c
+#define LUA_LIB
+
+#include "lua.h"
+#include "lmisclib.h"
+
+#include "lj_obj.h"
+#include "lj_str.h"
+#include "lj_tab.h"
+#include "lj_ff.h"
+#include "lj_lib.h"
+
+/* ------------------------------------------------------------------------ */
+
+static LJ_AINLINE void setnumfield(struct lua_State *L, GCtab *t,
+				   const char *name, int64_t val)
+{
+	GCstr *key = lj_str_new(L, name, strlen(name));
+	setnumV(lj_tab_setstr(L, t, key), (double)val);
+}
+
+#define LJLIB_MODULE_misc
+
+LJLIB_CF(misc_getmetrics)
+{
+	lua_createtable(L, 0, 19);
+	GCtab *m = tabV(L->top - 1);
+
+	struct luam_Metrics metrics;
+	luaM_metrics(L, &metrics);
+
+	setnumfield(L, m, "strnum", metrics.strnum);
+	setnumfield(L, m, "tabnum", metrics.tabnum);
+	setnumfield(L, m, "udatanum", metrics.udatanum);
+	setnumfield(L, m, "cdatanum", metrics.cdatanum);
+
+	setnumfield(L, m, "gc_total", metrics.gc_total);
+
+	setnumfield(L, m, "jit_mcode_size", metrics.jit_mcode_size);
+	setnumfield(L, m, "jit_trace_num", metrics.jit_trace_num);
+
+	setnumfield(L, m, "gc_freed", metrics.gc_freed);
+	setnumfield(L, m, "gc_allocated", metrics.gc_allocated);
+
+	setnumfield(L, m, "gc_steps_pause", metrics.gc_steps_pause);
+	setnumfield(L, m, "gc_steps_propagate", metrics.gc_steps_propagate);
+	setnumfield(L, m, "gc_steps_atomic", metrics.gc_steps_atomic);
+	setnumfield(L, m, "gc_steps_sweepstring", metrics.gc_steps_sweepstring);
+	setnumfield(L, m, "gc_steps_sweep", metrics.gc_steps_sweep);
+	setnumfield(L, m, "gc_steps_finalize", metrics.gc_steps_finalize);
+
+	setnumfield(L, m, "jit_snap_restore", metrics.jit_snap_restore);
+	setnumfield(L, m, "jit_trace_abort", metrics.jit_trace_abort);
+
+	setnumfield(L, m, "strhash_hit", metrics.strhash_hit);
+	setnumfield(L, m, "strhash_miss", metrics.strhash_miss);
+	return 1;
+}
+
+/* ------------------------------------------------------------------------ */
+
+#include "lj_libdef.h"
+
+LUALIB_API int luaopen_misc(struct lua_State *L)
+{
+	LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc);
+	return 1;
+}
diff --git a/src/lj_misc_capi.c b/src/lj_misc_capi.c
new file mode 100644
index 0000000..5ae98b9
--- /dev/null
+++ b/src/lj_misc_capi.c
@@ -0,0 +1,59 @@
+/*
+ * Tarantool-specific extensions to the public Lua/C API.
+ *
+ * Major portions taken verbatim or adapted from the LuaVela.
+ * Copyright (C) 2015-2019 IPONWEB Ltd.
+ */
+
+#include "lua.h"
+#include "lmisclib.h"
+
+#include "lj_obj.h"
+#include "lj_gc.h"
+#include "lj_dispatch.h"
+
+#if LJ_HASJIT
+#include "lj_jit.h"
+#endif
+
+LUAM_API struct luam_Metrics *
+luaM_metrics(lua_State *L, struct luam_Metrics *dst)
+{
+	memset(dst, 0, sizeof(*dst));
+	global_State *g = G(L);
+	GCState *gc = &g->gc;
+#if LJ_HASJIT
+	jit_State *J = G2J(g);
+#endif
+
+	dst->strhash_hit = g->strhash_hit;
+	dst->strhash_miss = g->strhash_miss;
+
+	dst->strnum = g->strnum;
+	dst->tabnum = gc->tabnum;
+	dst->udatanum = gc->udatanum;
+#if LJ_HASFFI
+	dst->cdatanum = gc->cdatanum;
+#endif
+
+	dst->gc_total = gc->total;
+	dst->gc_freed = gc->freed;
+	dst->gc_allocated = gc->allocated;
+
+	dst->gc_steps_pause = gc->state_count[GCSpause];
+	dst->gc_steps_propagate = gc->state_count[GCSpropagate];
+	dst->gc_steps_atomic = gc->state_count[GCSatomic];
+	dst->gc_steps_sweepstring = gc->state_count[GCSsweepstring];
+	dst->gc_steps_sweep = gc->state_count[GCSsweep];
+	dst->gc_steps_finalize = gc->state_count[GCSfinalize];
+
+#if LJ_HASJIT
+	dst->jit_snap_restore = J->nsnaprestore;
+	dst->jit_trace_abort = J->ntraceabort;
+
+	dst->jit_mcode_size = J->szallmcarea;
+	dst->jit_trace_num = J->freetrace;
+#endif
+
+	return dst;
+}
diff --git a/src/lmisclib.h b/src/lmisclib.h
new file mode 100644
index 0000000..87423c1
--- /dev/null
+++ b/src/lmisclib.h
@@ -0,0 +1,61 @@
+/*
+ * Major portions taken verbatim or adapted from the LuaVela.
+ * Copyright (C) 2015-2019 IPONWEB Ltd.
+ */
+
+#ifndef _LMISCLIB_H_INCLUDED
+#define _LMISCLIB_H_INCLUDED
+
+#include "lua.h"
+
+/* API for obtaining various metrics from the platform. */
+
+struct luam_Metrics {
+	/*
+	 * Strings amount founded in string hash
+	 * instead of allocation of new one.
+	 */
+	size_t strhash_hit;
+	/* Strings amount allocated and put into string hash. */
+	size_t strhash_miss;
+
+	size_t strnum;   /* Amount of allocated string objects. */
+	size_t tabnum;   /* Amount of allocated table objects. */
+	size_t udatanum; /* Amount of allocated udata objects. */
+	size_t cdatanum; /* Amount of allocated cdata objects. */
+
+	/* Memory currently allocated. */
+	size_t gc_total;
+	/* Total amount of freed memory. */
+	size_t gc_freed;
+	/* Total amount of allocated memory. */
+	size_t gc_allocated;
+
+	/* Count of incremental GC steps per state. */
+	size_t gc_steps_pause;
+	size_t gc_steps_propagate;
+	size_t gc_steps_atomic;
+	size_t gc_steps_sweepstring;
+	size_t gc_steps_sweep;
+	size_t gc_steps_finalize;
+
+	/*
+	 * Overall number of snap restores (and number of stopped
+	 * trace executions) for given jit_State.
+	 */
+	size_t jit_snap_restore;
+	/* Overall number of abort traces for given jit_State. */
+	size_t jit_trace_abort;
+	/* Total size of all allocated machine code areas. */
+	size_t jit_mcode_size;
+	/* Amount of JIT traces. */
+	unsigned int jit_trace_num;
+};
+
+LUAM_API struct luam_Metrics *luaM_metrics(lua_State *L,
+					   struct luam_Metrics *dst);
+
+#define LUAM_MISCLIBNAME "misc"
+LUALIB_API int luaopen_misc(lua_State *L);
+
+#endif /* _LMISCLIB_H_INCLUDED */
diff --git a/src/luaconf.h b/src/luaconf.h
index 60cb928..cf01e36 100644
--- a/src/luaconf.h
+++ b/src/luaconf.h
@@ -144,6 +144,7 @@
 #endif
 
 #define LUALIB_API	LUA_API
+#define LUAM_API	LUA_API
 
 /* Support for internal assertions. */
 #if defined(LUA_USE_ASSERT) || defined(LUA_USE_APICHECK)
-- 
2.24.1

Re: [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API

[Igor Munkin]

Sergey,

Thanks for the patch! Please consider my comments below.

On 26.07.20, Sergey Kaplun wrote:
> This patch adds C and Lua API for luajit metrics. Metrics include GC
> statistic, JIT information, strhash hit/miss counters and amount of
> different objects. C API provides with aditional header <lmisclib.h>
> that contains structure `luam_metrics` and `luaM_metrics` function
> definition. Lua userspace expanded with builtin library (named `misc`)
> with corresponding method `getmetrics`.

Well, let's make the commit message a bit clearer:
| This patch introduces both C and Lua API for LuaJIT platform
| metrics implemented in scope of the previous patch. New <lmisclib.h>
| header provides <luaM_metrics> C interface that fills the given
| <luam_metrics> structure with the platform metrics. Additionally
| <misc> module is loaded to Lua space and provides <getmetrics>
| method that yields the corresponding metrics via table.

Feel free to change it on your own.

> 
> Part of tarantool/tarantool#5187
> ---

This comment relates to all created files: I see no reason to violate
LuaJIT code style, so please adjust the sources considering the current
practices.

>  Makefile           |  2 +-
>  src/Makefile       |  5 ++--
>  src/Makefile.dep   |  3 ++
>  src/lib_init.c     |  2 ++
>  src/lib_misc.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/lj_misc_capi.c | 59 ++++++++++++++++++++++++++++++++++++
>  src/lmisclib.h     | 61 +++++++++++++++++++++++++++++++++++++
>  src/luaconf.h      |  1 +
>  8 files changed, 205 insertions(+), 3 deletions(-)
>  create mode 100644 src/lib_misc.c
>  create mode 100644 src/lj_misc_capi.c
>  create mode 100644 src/lmisclib.h
> 

<snipped>

> diff --git a/src/Makefile b/src/Makefile
> index 827d4a4..fac69bc 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -480,13 +480,14 @@ LJVM_BOUT= $(LJVM_S)
>  LJVM_MODE= elfasm
>  
>  LJLIB_O= lib_base.o lib_math.o lib_bit.o lib_string.o lib_table.o \
> -	 lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o
> +	 lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o \
> +	 lib_misc.o
>  LJLIB_C= $(LJLIB_O:.o=.c)
>  
>  LJCORE_O= lj_gc.o lj_err.o lj_char.o lj_bc.o lj_obj.o lj_buf.o \
>  	  lj_str.o lj_tab.o lj_func.o lj_udata.o lj_meta.o lj_debug.o \
>  	  lj_state.o lj_dispatch.o lj_vmevent.o lj_vmmath.o lj_strscan.o \
> -	  lj_strfmt.o lj_strfmt_num.o lj_api.o lj_profile.o \
> +	  lj_strfmt.o lj_strfmt_num.o lj_api.o lj_misc_capi.o lj_profile.o \

Why did you add <lj_misc_capi.o> in the middle of LJCORE_O?

>  	  lj_lex.o lj_parse.o lj_bcread.o lj_bcwrite.o lj_load.o \
>  	  lj_ir.o lj_opt_mem.o lj_opt_fold.o lj_opt_narrow.o \
>  	  lj_opt_dce.o lj_opt_loop.o lj_opt_split.o lj_opt_sink.o \
> diff --git a/src/Makefile.dep b/src/Makefile.dep
> index 2b1cb5e..1c3d8bd 100644
> --- a/src/Makefile.dep
> +++ b/src/Makefile.dep

lmisclib.h is missing in lib_init.o deplist.

> @@ -41,6 +41,9 @@ lib_string.o: lib_string.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
>  lib_table.o: lib_table.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
>   lj_def.h lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h \
>   lj_tab.h lj_ff.h lj_ffdef.h lj_lib.h lj_libdef.h

Object file list is sorted, so why do you violate this order?

> +lib_misc.o: lib_misc.c lua.h lmisclib.h luaconf.h lj_obj.h lj_tab.h lj_str.h \
> + lj_arch.h lj_lib.h lj_vm.h lj_libdef.h

Minor: luaconf.h is required for lua.h, not for lmisclib.h, so the order
should be the same as in other lib_*.o targets. This comment also
relates to lj_arch.h.

lj_def.h is missing and lj_vm.h is excess.

> +lj_misc_capi.o: lj_misc_capi.c lua.h lmisclib.h luaconf.h lj_obj.h lj_arch.h

lj_def.h, lj_jit.h and lj_dispatch.h are missing.

>  lj_alloc.o: lj_alloc.c lj_def.h lua.h luaconf.h lj_arch.h lj_alloc.h
>  lj_api.o: lj_api.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_gc.h \
>   lj_err.h lj_errmsg.h lj_debug.h lj_str.h lj_tab.h lj_func.h lj_udata.h \

<snipped>

> diff --git a/src/lib_misc.c b/src/lib_misc.c
> new file mode 100644
> index 0000000..ef20921
> --- /dev/null
> +++ b/src/lib_misc.c
> @@ -0,0 +1,75 @@
> +/*
> + * Lua interface to tarantool-specific extensions to the public Lua/C API.

It relates neither to Lua/C API nor to Tarantool. This translation unit
simply provides miscellaneous builtin functions extending Lua Reference
manual.

> + *
> + * Major portions taken verbatim or adapted from the LuaVela interpreter.
> + * Copyright (C) 2015-2019 IPONWEB Ltd.
> + */
> +
> +#define lib_misc_c
> +#define LUA_LIB
> +
> +#include "lua.h"
> +#include "lmisclib.h"
> +
> +#include "lj_obj.h"
> +#include "lj_str.h"
> +#include "lj_tab.h"
> +#include "lj_ff.h"

This include seems to be excess.

> +#include "lj_lib.h"
> +
> +/* ------------------------------------------------------------------------ */
> +
> +static LJ_AINLINE void setnumfield(struct lua_State *L, GCtab *t,
> +				   const char *name, int64_t val)
> +{
> +	GCstr *key = lj_str_new(L, name, strlen(name));

<lj_str_newz> looks more convenient here. Consider the following:
| setnumV(lj_tab_setstr(L, t, lj_str_newz(L, name), (double)val));

> +	setnumV(lj_tab_setstr(L, t, key), (double)val);

Well, I've already mentioned my concerns while discussing offline the
first series. Since I see no corresponding changes, I leave them in this
reply.

User will face precision loss if any counter exceeds 1 << 53. Consider
the following example:
| $ luajit -e 'print(2^53 == 2^53 + 1)'
| true

I guess some counters (e.g. gc_freed and gc_allocated) should be cdata
64-bit numbers instead of the Lua (i.e. doubles) ones. Thoughts?

> +}
> +
> +#define LJLIB_MODULE_misc
> +
> +LJLIB_CF(misc_getmetrics)
> +{
> +	lua_createtable(L, 0, 19);
> +	GCtab *m = tabV(L->top - 1);
> +
> +	struct luam_Metrics metrics;

Please, declare local variables in the beginning of the block.

> +	luaM_metrics(L, &metrics);
> +
> +	setnumfield(L, m, "strnum", metrics.strnum);
> +	setnumfield(L, m, "tabnum", metrics.tabnum);
> +	setnumfield(L, m, "udatanum", metrics.udatanum);
> +	setnumfield(L, m, "cdatanum", metrics.cdatanum);
> +
> +	setnumfield(L, m, "gc_total", metrics.gc_total);
> +
> +	setnumfield(L, m, "jit_mcode_size", metrics.jit_mcode_size);
> +	setnumfield(L, m, "jit_trace_num", metrics.jit_trace_num);

This ordering looks odd. It differs from <luam_Metrics> structure fields
ordering. It also differs from the order <metrics> fields are filled
within <luaM_metrics>. It's simply a mess, please adjust the ordering to
a single way (the one from structure definition is fine).

Furthermore, I don't get the rule you split blocks with whitespace...

> +
> +	setnumfield(L, m, "gc_freed", metrics.gc_freed);
> +	setnumfield(L, m, "gc_allocated", metrics.gc_allocated);
> +
> +	setnumfield(L, m, "gc_steps_pause", metrics.gc_steps_pause);
> +	setnumfield(L, m, "gc_steps_propagate", metrics.gc_steps_propagate);
> +	setnumfield(L, m, "gc_steps_atomic", metrics.gc_steps_atomic);
> +	setnumfield(L, m, "gc_steps_sweepstring", metrics.gc_steps_sweepstring);
> +	setnumfield(L, m, "gc_steps_sweep", metrics.gc_steps_sweep);
> +	setnumfield(L, m, "gc_steps_finalize", metrics.gc_steps_finalize);
> +
> +	setnumfield(L, m, "jit_snap_restore", metrics.jit_snap_restore);
> +	setnumfield(L, m, "jit_trace_abort", metrics.jit_trace_abort);
> +
> +	setnumfield(L, m, "strhash_hit", metrics.strhash_hit);
> +	setnumfield(L, m, "strhash_miss", metrics.strhash_miss);
> +	return 1;
> +}
> +
> +/* ------------------------------------------------------------------------ */
> +
> +#include "lj_libdef.h"
> +
> +LUALIB_API int luaopen_misc(struct lua_State *L)
> +{
> +	LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc);
> +	return 1;
> +}
> diff --git a/src/lj_misc_capi.c b/src/lj_misc_capi.c

Minor: <lj_mapi.c> fits better to the current LJ naming, doesn't it?

> new file mode 100644
> index 0000000..5ae98b9
> --- /dev/null
> +++ b/src/lj_misc_capi.c
> @@ -0,0 +1,59 @@
> +/*
> + * Tarantool-specific extensions to the public Lua/C API.

Again, these interfaces doesn't relate to Tarantool.

> + *
> + * Major portions taken verbatim or adapted from the LuaVela.
> + * Copyright (C) 2015-2019 IPONWEB Ltd.
> + */
> +
> +#include "lua.h"
> +#include "lmisclib.h"
> +
> +#include "lj_obj.h"
> +#include "lj_gc.h"

This include seems to be excess.

> +#include "lj_dispatch.h"
> +
> +#if LJ_HASJIT
> +#include "lj_jit.h"
> +#endif
> +
> +LUAM_API struct luam_Metrics *

OK, I guess <void> is enough here.

> +luaM_metrics(lua_State *L, struct luam_Metrics *dst)

Since there is no "src", just "metrics" are fine.

> +{

I see not a single word or check for the case when dst is NULL. If you
forbid NULL argument with the function contract, please add the
corresponding assert. Otherwise the behaviour should be adjusted to it.

> +	memset(dst, 0, sizeof(*dst));

I was misguided with this <memset>, it looked excess. Later, I realized
that the fields in the resulting structure are present despite the build
flags, but their initialization respects the way LuaJIT is built. It's
definitely worth to mention this fact with the comment nearby.

> +	global_State *g = G(L);
> +	GCState *gc = &g->gc;
> +#if LJ_HASJIT
> +	jit_State *J = G2J(g);
> +#endif
> +
> +	dst->strhash_hit = g->strhash_hit;
> +	dst->strhash_miss = g->strhash_miss;
> +
> +	dst->strnum = g->strnum;
> +	dst->tabnum = gc->tabnum;
> +	dst->udatanum = gc->udatanum;
> +#if LJ_HASFFI
> +	dst->cdatanum = gc->cdatanum;
> +#endif
> +
> +	dst->gc_total = gc->total;
> +	dst->gc_freed = gc->freed;
> +	dst->gc_allocated = gc->allocated;
> +
> +	dst->gc_steps_pause = gc->state_count[GCSpause];
> +	dst->gc_steps_propagate = gc->state_count[GCSpropagate];
> +	dst->gc_steps_atomic = gc->state_count[GCSatomic];
> +	dst->gc_steps_sweepstring = gc->state_count[GCSsweepstring];
> +	dst->gc_steps_sweep = gc->state_count[GCSsweep];
> +	dst->gc_steps_finalize = gc->state_count[GCSfinalize];
> +
> +#if LJ_HASJIT
> +	dst->jit_snap_restore = J->nsnaprestore;
> +	dst->jit_trace_abort = J->ntraceabort;
> +

Minor: Still don't get the rule you split blocks with whitespace...

> +	dst->jit_mcode_size = J->szallmcarea;
> +	dst->jit_trace_num = J->freetrace;
> +#endif
> +
> +	return dst;
> +}
> diff --git a/src/lmisclib.h b/src/lmisclib.h
> new file mode 100644
> index 0000000..87423c1
> --- /dev/null
> +++ b/src/lmisclib.h
> @@ -0,0 +1,61 @@
> +/*

Meh, you've written at least one sentence in the new LJ private sources,
but there is not a word here (in the public header, to be distributed).
I guess, this is the most important new file to be described.

> + * Major portions taken verbatim or adapted from the LuaVela.
> + * Copyright (C) 2015-2019 IPONWEB Ltd.
> + */
> +
> +#ifndef _LMISCLIB_H_INCLUDED
> +#define _LMISCLIB_H_INCLUDED
> +
> +#include "lua.h"
> +
> +/* API for obtaining various metrics from the platform. */
> +
> +struct luam_Metrics {
> +	/*
> +	 * Strings amount founded in string hash

Typo: s/founded/found/.

> +	 * instead of allocation of new one.
> +	 */
> +	size_t strhash_hit;
> +	/* Strings amount allocated and put into string hash. */
> +	size_t strhash_miss;
> +
> +	size_t strnum;   /* Amount of allocated string objects. */
> +	size_t tabnum;   /* Amount of allocated table objects. */
> +	size_t udatanum; /* Amount of allocated udata objects. */
> +	size_t cdatanum; /* Amount of allocated cdata objects. */

Why do you mix inline comments with those going prior to the field?
Please choose a single way to write comments here.

> +
> +	/* Memory currently allocated. */
> +	size_t gc_total;
> +	/* Total amount of freed memory. */
> +	size_t gc_freed;
> +	/* Total amount of allocated memory. */
> +	size_t gc_allocated;
> +
> +	/* Count of incremental GC steps per state. */
> +	size_t gc_steps_pause;
> +	size_t gc_steps_propagate;
> +	size_t gc_steps_atomic;
> +	size_t gc_steps_sweepstring;
> +	size_t gc_steps_sweep;
> +	size_t gc_steps_finalize;
> +
> +	/*
> +	 * Overall number of snap restores (and number of stopped

Strictly saying this is not true, since execution can leave the trace
without restoring the snapshot (via <lj_vm_exit_interp>).

> +	 * trace executions) for given jit_State.

A mess again: you mention the fact snap restores are counted for the
"given jit_State", *but* the overall traces amount is not? This remark
looks irrelevant and a bit misguiding. I believe it can be dropped.

> +	 */
> +	size_t jit_snap_restore;
> +	/* Overall number of abort traces for given jit_State. */
> +	size_t jit_trace_abort;
> +	/* Total size of all allocated machine code areas. */
> +	size_t jit_mcode_size;
> +	/* Amount of JIT traces. */
> +	unsigned int jit_trace_num;
> +};
> +
> +LUAM_API struct luam_Metrics *luaM_metrics(lua_State *L,
> +					   struct luam_Metrics *dst);
> +
> +#define LUAM_MISCLIBNAME "misc"
> +LUALIB_API int luaopen_misc(lua_State *L);
> +
> +#endif /* _LMISCLIB_H_INCLUDED */
> diff --git a/src/luaconf.h b/src/luaconf.h
> index 60cb928..cf01e36 100644
> --- a/src/luaconf.h
> +++ b/src/luaconf.h
> @@ -144,6 +144,7 @@
>  #endif
>  
>  #define LUALIB_API	LUA_API
> +#define LUAM_API	LUA_API

Minor: LUAMISC_API name looks more suitable to me (it visually fits
LUALIB_API). LUAM_API would be fine if LUALIB_API was named as LUAL_API.

>  
>  /* Support for internal assertions. */
>  #if defined(LUA_USE_ASSERT) || defined(LUA_USE_APICHECK)
> -- 
> 2.24.1
> 

And last but not least: what about tests?

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API

[Sergey Kaplun]

Igor,

Thanks for the review!

On 27.08.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider my comments below.
> 
> On 26.07.20, Sergey Kaplun wrote:
> > This patch adds C and Lua API for luajit metrics. Metrics include GC
> > statistic, JIT information, strhash hit/miss counters and amount of
> > different objects. C API provides with aditional header <lmisclib.h>
> > that contains structure `luam_metrics` and `luaM_metrics` function
> > definition. Lua userspace expanded with builtin library (named `misc`)
> > with corresponding method `getmetrics`.
> 
> Well, let's make the commit message a bit clearer:
> | This patch introduces both C and Lua API for LuaJIT platform
> | metrics implemented in scope of the previous patch. New <lmisclib.h>
> | header provides <luaM_metrics> C interface that fills the given
> | <luam_metrics> structure with the platform metrics. Additionally
> | <misc> module is loaded to Lua space and provides <getmetrics>
> | method that yields the corresponding metrics via table.
> 
> Feel free to change it on your own.
> 

It sounds really good now, thanks!

> > 
> > Part of tarantool/tarantool#5187
> > ---
> 
> This comment relates to all created files: I see no reason to violate
> LuaJIT code style, so please adjust the sources considering the current
> practices.

Should I use 2 space indent?

> 
> >  Makefile           |  2 +-
> >  src/Makefile       |  5 ++--
> >  src/Makefile.dep   |  3 ++
> >  src/lib_init.c     |  2 ++
> >  src/lib_misc.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> >  src/lj_misc_capi.c | 59 ++++++++++++++++++++++++++++++++++++
> >  src/lmisclib.h     | 61 +++++++++++++++++++++++++++++++++++++
> >  src/luaconf.h      |  1 +
> >  8 files changed, 205 insertions(+), 3 deletions(-)
> >  create mode 100644 src/lib_misc.c
> >  create mode 100644 src/lj_misc_capi.c
> >  create mode 100644 src/lmisclib.h
> > 
> 
> <snipped>
> 
> > diff --git a/src/Makefile b/src/Makefile
> > index 827d4a4..fac69bc 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -480,13 +480,14 @@ LJVM_BOUT= $(LJVM_S)
> >  LJVM_MODE= elfasm
> >  
> >  LJLIB_O= lib_base.o lib_math.o lib_bit.o lib_string.o lib_table.o \
> > -	 lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o
> > +	 lib_io.o lib_os.o lib_package.o lib_debug.o lib_jit.o lib_ffi.o \
> > +	 lib_misc.o
> >  LJLIB_C= $(LJLIB_O:.o=.c)
> >  
> >  LJCORE_O= lj_gc.o lj_err.o lj_char.o lj_bc.o lj_obj.o lj_buf.o \
> >  	  lj_str.o lj_tab.o lj_func.o lj_udata.o lj_meta.o lj_debug.o \
> >  	  lj_state.o lj_dispatch.o lj_vmevent.o lj_vmmath.o lj_strscan.o \
> > -	  lj_strfmt.o lj_strfmt_num.o lj_api.o lj_profile.o \
> > +	  lj_strfmt.o lj_strfmt_num.o lj_api.o lj_misc_capi.o lj_profile.o \
> 
> Why did you add <lj_misc_capi.o> in the middle of LJCORE_O?

It's next to <lj_api.o>. JIT-related modules, strfrmt modules,
BC-related modules are also located next to each other.

> 
> >  	  lj_lex.o lj_parse.o lj_bcread.o lj_bcwrite.o lj_load.o \
> >  	  lj_ir.o lj_opt_mem.o lj_opt_fold.o lj_opt_narrow.o \
> >  	  lj_opt_dce.o lj_opt_loop.o lj_opt_split.o lj_opt_sink.o \
> > diff --git a/src/Makefile.dep b/src/Makefile.dep
> > index 2b1cb5e..1c3d8bd 100644
> > --- a/src/Makefile.dep
> > +++ b/src/Makefile.dep
> 
> lmisclib.h is missing in lib_init.o deplist.
> 
> > @@ -41,6 +41,9 @@ lib_string.o: lib_string.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
> >  lib_table.o: lib_table.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \
> >   lj_def.h lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h \
> >   lj_tab.h lj_ff.h lj_ffdef.h lj_lib.h lj_libdef.h
> 
> Object file list is sorted, so why do you violate this order?
> 
> > +lib_misc.o: lib_misc.c lua.h lmisclib.h luaconf.h lj_obj.h lj_tab.h lj_str.h \
> > + lj_arch.h lj_lib.h lj_vm.h lj_libdef.h
> 
> Minor: luaconf.h is required for lua.h, not for lmisclib.h, so the order
> should be the same as in other lib_*.o targets. This comment also
> relates to lj_arch.h.
> 
> lj_def.h is missing and lj_vm.h is excess.
> 
> > +lj_misc_capi.o: lj_misc_capi.c lua.h lmisclib.h luaconf.h lj_obj.h lj_arch.h
> 
> lj_def.h, lj_jit.h and lj_dispatch.h are missing.
> 
> >  lj_alloc.o: lj_alloc.c lj_def.h lua.h luaconf.h lj_arch.h lj_alloc.h
> >  lj_api.o: lj_api.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_gc.h \
> >   lj_err.h lj_errmsg.h lj_debug.h lj_str.h lj_tab.h lj_func.h lj_udata.h \

Oh... I'll clean up all mess at deplist.

> 
> <snipped>
> 
> > diff --git a/src/lib_misc.c b/src/lib_misc.c
> > new file mode 100644
> > index 0000000..ef20921
> > --- /dev/null
> > +++ b/src/lib_misc.c
> > @@ -0,0 +1,75 @@
> > +/*
> > + * Lua interface to tarantool-specific extensions to the public Lua/C API.
> 
> It relates neither to Lua/C API nor to Tarantool. This translation unit
> simply provides miscellaneous builtin functions extending Lua Reference
> manual.

OK, I will reword description this way.

> 
> > + *
> > + * Major portions taken verbatim or adapted from the LuaVela interpreter.
> > + * Copyright (C) 2015-2019 IPONWEB Ltd.
> > + */
> > +
> > +#define lib_misc_c
> > +#define LUA_LIB
> > +
> > +#include "lua.h"
> > +#include "lmisclib.h"
> > +
> > +#include "lj_obj.h"
> > +#include "lj_str.h"
> > +#include "lj_tab.h"
> > +#include "lj_ff.h"
> 
> This include seems to be excess.

It is. Thanks!

> 
> > +#include "lj_lib.h"
> > +
> > +/* ------------------------------------------------------------------------ */
> > +
> > +static LJ_AINLINE void setnumfield(struct lua_State *L, GCtab *t,
> > +				   const char *name, int64_t val)
> > +{
> > +	GCstr *key = lj_str_new(L, name, strlen(name));
> 
> <lj_str_newz> looks more convenient here. Consider the following:
> | setnumV(lj_tab_setstr(L, t, lj_str_newz(L, name), (double)val));
> 
> > +	setnumV(lj_tab_setstr(L, t, key), (double)val);
> 

Yes, it is better.

> Well, I've already mentioned my concerns while discussing offline the
> first series. Since I see no corresponding changes, I leave them in this
> reply.
> 
> User will face precision loss if any counter exceeds 1 << 53. Consider
> the following example:
> | $ luajit -e 'print(2^53 == 2^53 + 1)'
> | true
> 
> I guess some counters (e.g. gc_freed and gc_allocated) should be cdata
> 64-bit numbers instead of the Lua (i.e. doubles) ones. Thoughts?

It's interesting point. In the one way this changes really omit
precision loss of data. But also when we build LuaJIT with
<-DLUAJIT_DISABLE_FFI> we should return number type instead of cdata
again. It leads to inconsistent behaviour of one function depends on
build type.

In the other way we can loss maximum ~2^10, IINM:
| $ luajit -e 'print(2^63 == 2^63 + 1024)'
| true
| $ luajit -e 'print(2^63 == 2^63 + 1025)'
| false

It means that users can't distinguish between values with a 1Kb
difference.  But for so fast growing up <gc_freed> and <gc_allocated>
(2^63 bytes) value it shouldn't become a very big problem to them. It's
really rare situation when you need get your metrics so frequent, that
you fail to allocate/free 1Kb of objects in due time.

P.S. We can s/size_t/uint64_t/ to guarantee maximum fields value.

> 
> > +}
> > +
> > +#define LJLIB_MODULE_misc
> > +
> > +LJLIB_CF(misc_getmetrics)
> > +{
> > +	lua_createtable(L, 0, 19);
> > +	GCtab *m = tabV(L->top - 1);
> > +
> > +	struct luam_Metrics metrics;
> 
> Please, declare local variables in the beginning of the block.

Sure, considering to LuaJIT code style.

> 
> > +	luaM_metrics(L, &metrics);
> > +
> > +	setnumfield(L, m, "strnum", metrics.strnum);
> > +	setnumfield(L, m, "tabnum", metrics.tabnum);
> > +	setnumfield(L, m, "udatanum", metrics.udatanum);
> > +	setnumfield(L, m, "cdatanum", metrics.cdatanum);
> > +
> > +	setnumfield(L, m, "gc_total", metrics.gc_total);
> > +
> > +	setnumfield(L, m, "jit_mcode_size", metrics.jit_mcode_size);
> > +	setnumfield(L, m, "jit_trace_num", metrics.jit_trace_num);
> 
> This ordering looks odd. It differs from <luam_Metrics> structure fields
> ordering. It also differs from the order <metrics> fields are filled
> within <luaM_metrics>. It's simply a mess, please adjust the ordering to
> a single way (the one from structure definition is fine).
> 
> Furthermore, I don't get the rule you split blocks with whitespace...

OK, I'll reorder this fields according to the structure definition and
remove excess empty lines.

> 
> > +
> > +	setnumfield(L, m, "gc_freed", metrics.gc_freed);
> > +	setnumfield(L, m, "gc_allocated", metrics.gc_allocated);
> > +
> > +	setnumfield(L, m, "gc_steps_pause", metrics.gc_steps_pause);
> > +	setnumfield(L, m, "gc_steps_propagate", metrics.gc_steps_propagate);
> > +	setnumfield(L, m, "gc_steps_atomic", metrics.gc_steps_atomic);
> > +	setnumfield(L, m, "gc_steps_sweepstring", metrics.gc_steps_sweepstring);
> > +	setnumfield(L, m, "gc_steps_sweep", metrics.gc_steps_sweep);
> > +	setnumfield(L, m, "gc_steps_finalize", metrics.gc_steps_finalize);
> > +
> > +	setnumfield(L, m, "jit_snap_restore", metrics.jit_snap_restore);
> > +	setnumfield(L, m, "jit_trace_abort", metrics.jit_trace_abort);
> > +
> > +	setnumfield(L, m, "strhash_hit", metrics.strhash_hit);
> > +	setnumfield(L, m, "strhash_miss", metrics.strhash_miss);
> > +	return 1;
> > +}
> > +
> > +/* ------------------------------------------------------------------------ */
> > +
> > +#include "lj_libdef.h"
> > +
> > +LUALIB_API int luaopen_misc(struct lua_State *L)
> > +{
> > +	LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc);
> > +	return 1;
> > +}
> > diff --git a/src/lj_misc_capi.c b/src/lj_misc_capi.c
> 
> Minor: <lj_mapi.c> fits better to the current LJ naming, doesn't it?

Sure, especially since we still haven't come up with a better library
name than misc :)

> 
> > new file mode 100644
> > index 0000000..5ae98b9
> > --- /dev/null
> > +++ b/src/lj_misc_capi.c
> > @@ -0,0 +1,59 @@
> > +/*
> > + * Tarantool-specific extensions to the public Lua/C API.
> 
> Again, these interfaces doesn't relate to Tarantool.

Sure!

> 
> > + *
> > + * Major portions taken verbatim or adapted from the LuaVela.
> > + * Copyright (C) 2015-2019 IPONWEB Ltd.
> > + */
> > +
> > +#include "lua.h"
> > +#include "lmisclib.h"
> > +
> > +#include "lj_obj.h"
> > +#include "lj_gc.h"
> 
> This include seems to be excess.

Yes, thanks!

> 
> > +#include "lj_dispatch.h"
> > +
> > +#if LJ_HASJIT
> > +#include "lj_jit.h"
> > +#endif
> > +
> > +LUAM_API struct luam_Metrics *
> 
> OK, I guess <void> is enough here.
> 
> > +luaM_metrics(lua_State *L, struct luam_Metrics *dst)
> 
> Since there is no "src", just "metrics" are fine.
> 

OK, not a big deal:)

> > +{
> 
> I see not a single word or check for the case when dst is NULL. If you
> forbid NULL argument with the function contract, please add the
> corresponding assert. Otherwise the behaviour should be adjusted to it.

Sure, I'll add <lua_assert> here.

> 
> > +	memset(dst, 0, sizeof(*dst));
> 
> I was misguided with this <memset>, it looked excess. Later, I realized
> that the fields in the resulting structure are present despite the build
> flags, but their initialization respects the way LuaJIT is built. It's
> definitely worth to mention this fact with the comment nearby.

It's unclear indeed. I will rewrote it in this way:

| #if LJ_HASFFI
| 	dst->cdatanum = gc->cdatanum;
| #else
| 	dst->cdatanum = 0;
| #endif

> > +	global_State *g = G(L);
> > +	GCState *gc = &g->gc;
> > +#if LJ_HASJIT
> > +	jit_State *J = G2J(g);
> > +#endif
> > +
> > +	dst->strhash_hit = g->strhash_hit;
> > +	dst->strhash_miss = g->strhash_miss;
> > +
> > +	dst->strnum = g->strnum;
> > +	dst->tabnum = gc->tabnum;
> > +	dst->udatanum = gc->udatanum;
> > +#if LJ_HASFFI
> > +	dst->cdatanum = gc->cdatanum;
> > +#endif
> > +
> > +	dst->gc_total = gc->total;
> > +	dst->gc_freed = gc->freed;
> > +	dst->gc_allocated = gc->allocated;
> > +
> > +	dst->gc_steps_pause = gc->state_count[GCSpause];
> > +	dst->gc_steps_propagate = gc->state_count[GCSpropagate];
> > +	dst->gc_steps_atomic = gc->state_count[GCSatomic];
> > +	dst->gc_steps_sweepstring = gc->state_count[GCSsweepstring];
> > +	dst->gc_steps_sweep = gc->state_count[GCSsweep];
> > +	dst->gc_steps_finalize = gc->state_count[GCSfinalize];
> > +
> > +#if LJ_HASJIT
> > +	dst->jit_snap_restore = J->nsnaprestore;
> > +	dst->jit_trace_abort = J->ntraceabort;
> > +
> 
> Minor: Still don't get the rule you split blocks with whitespace...

OK, I'll reorder this fields according to the structure definition and
remove excess empty lines.

> 
> > +	dst->jit_mcode_size = J->szallmcarea;
> > +	dst->jit_trace_num = J->freetrace;
> > +#endif
> > +
> > +	return dst;
> > +}
> > diff --git a/src/lmisclib.h b/src/lmisclib.h
> > new file mode 100644
> > index 0000000..87423c1
> > --- /dev/null
> > +++ b/src/lmisclib.h
> > @@ -0,0 +1,61 @@
> > +/*
> 
> Meh, you've written at least one sentence in the new LJ private sources,
> but there is not a word here (in the public header, to be distributed).
> I guess, this is the most important new file to be described.

Sure! My bad!

> 
> > + * Major portions taken verbatim or adapted from the LuaVela.
> > + * Copyright (C) 2015-2019 IPONWEB Ltd.
> > + */
> > +
> > +#ifndef _LMISCLIB_H_INCLUDED
> > +#define _LMISCLIB_H_INCLUDED
> > +
> > +#include "lua.h"
> > +
> > +/* API for obtaining various metrics from the platform. */
> > +
> > +struct luam_Metrics {
> > +	/*
> > +	 * Strings amount founded in string hash
> 
> Typo: s/founded/found/.

Thanks!

> 
> > +	 * instead of allocation of new one.
> > +	 */
> > +	size_t strhash_hit;
> > +	/* Strings amount allocated and put into string hash. */
> > +	size_t strhash_miss;
> > +
> > +	size_t strnum;   /* Amount of allocated string objects. */
> > +	size_t tabnum;   /* Amount of allocated table objects. */
> > +	size_t udatanum; /* Amount of allocated udata objects. */
> > +	size_t cdatanum; /* Amount of allocated cdata objects. */
> 
> Why do you mix inline comments with those going prior to the field?
> Please choose a single way to write comments here.

LuaJIT code style uses inline comments. May I use comments going prior
to corresponding field?

> 
> > +
> > +	/* Memory currently allocated. */
> > +	size_t gc_total;
> > +	/* Total amount of freed memory. */
> > +	size_t gc_freed;
> > +	/* Total amount of allocated memory. */
> > +	size_t gc_allocated;
> > +
> > +	/* Count of incremental GC steps per state. */
> > +	size_t gc_steps_pause;
> > +	size_t gc_steps_propagate;
> > +	size_t gc_steps_atomic;
> > +	size_t gc_steps_sweepstring;
> > +	size_t gc_steps_sweep;
> > +	size_t gc_steps_finalize;
> > +
> > +	/*
> > +	 * Overall number of snap restores (and number of stopped
> 
> Strictly saying this is not true, since execution can leave the trace
> without restoring the snapshot (via <lj_vm_exit_interp>).

Is it correct to say that it is equal to amount of guard assertions
leading to stopping trace executions?

> 
> > +	 * trace executions) for given jit_State.
> 
> A mess again: you mention the fact snap restores are counted for the
> "given jit_State", *but* the overall traces amount is not? This remark
> looks irrelevant and a bit misguiding. I believe it can be dropped.

Sure!

> 
> > +	 */
> > +	size_t jit_snap_restore;
> > +	/* Overall number of abort traces for given jit_State. */
> > +	size_t jit_trace_abort;
> > +	/* Total size of all allocated machine code areas. */
> > +	size_t jit_mcode_size;
> > +	/* Amount of JIT traces. */
> > +	unsigned int jit_trace_num;
> > +};
> > +
> > +LUAM_API struct luam_Metrics *luaM_metrics(lua_State *L,
> > +					   struct luam_Metrics *dst);
> > +
> > +#define LUAM_MISCLIBNAME "misc"
> > +LUALIB_API int luaopen_misc(lua_State *L);
> > +
> > +#endif /* _LMISCLIB_H_INCLUDED */
> > diff --git a/src/luaconf.h b/src/luaconf.h
> > index 60cb928..cf01e36 100644
> > --- a/src/luaconf.h
> > +++ b/src/luaconf.h
> > @@ -144,6 +144,7 @@
> >  #endif
> >  
> >  #define LUALIB_API	LUA_API
> > +#define LUAM_API	LUA_API
> 
> Minor: LUAMISC_API name looks more suitable to me (it visually fits
> LUALIB_API). LUAM_API would be fine if LUALIB_API was named as LUAL_API.

Yes, makes sense!

> 
> >  
> >  /* Support for internal assertions. */
> >  #if defined(LUA_USE_ASSERT) || defined(LUA_USE_APICHECK)
> > -- 
> > 2.24.1
> > 
> 
> And last but not least: what about tests?
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API

[Igor Munkin]

Sergey,

On 03.09.20, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 

<snipped>

> > 
> > This comment relates to all created files: I see no reason to violate
> > LuaJIT code style, so please adjust the sources considering the current
> > practices.
> 
> Should I use 2 space indent?

Yep, but don't forget tabs for 8-space indent (dunno, whether we can
call such indenting "fake quarter tabs").

> 
> > 
> > >  Makefile           |  2 +-
> > >  src/Makefile       |  5 ++--
> > >  src/Makefile.dep   |  3 ++
> > >  src/lib_init.c     |  2 ++
> > >  src/lib_misc.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  src/lj_misc_capi.c | 59 ++++++++++++++++++++++++++++++++++++
> > >  src/lmisclib.h     | 61 +++++++++++++++++++++++++++++++++++++
> > >  src/luaconf.h      |  1 +
> > >  8 files changed, 205 insertions(+), 3 deletions(-)
> > >  create mode 100644 src/lib_misc.c
> > >  create mode 100644 src/lj_misc_capi.c
> > >  create mode 100644 src/lmisclib.h
> > > 

<snipped>

> 
> > Well, I've already mentioned my concerns while discussing offline the
> > first series. Since I see no corresponding changes, I leave them in this
> > reply.
> > 
> > User will face precision loss if any counter exceeds 1 << 53. Consider
> > the following example:
> > | $ luajit -e 'print(2^53 == 2^53 + 1)'
> > | true
> > 
> > I guess some counters (e.g. gc_freed and gc_allocated) should be cdata
> > 64-bit numbers instead of the Lua (i.e. doubles) ones. Thoughts?
> 
> It's interesting point. In the one way this changes really omit
> precision loss of data. But also when we build LuaJIT with
> <-DLUAJIT_DISABLE_FFI> we should return number type instead of cdata
> again. It leads to inconsistent behaviour of one function depends on
> build type.

Yes, didn't take this fact into account.

> 
> In the other way we can loss maximum ~2^10, IINM:
> | $ luajit -e 'print(2^63 == 2^63 + 1024)'
> | true
> | $ luajit -e 'print(2^63 == 2^63 + 1025)'
> | false
> 
> It means that users can't distinguish between values with a 1Kb
> difference.  But for so fast growing up <gc_freed> and <gc_allocated>
> (2^63 bytes) value it shouldn't become a very big problem to them. It's
> really rare situation when you need get your metrics so frequent, that
> you fail to allocate/free 1Kb of objects in due time.

Well, then this fact should be precisely mentioned in the RFC to be
showed to Lua devs. If precision loss doesn't bother users, I'm for the
current way (i.e. Lua numbers) since it makes less pressure on GC.

> 
> P.S. We can s/size_t/uint64_t/ to guarantee maximum fields value.

IIRC they have the same size, don't they?

> 

<snipped>

> > 
> > Why do you mix inline comments with those going prior to the field?
> > Please choose a single way to write comments here.
> 
> LuaJIT code style uses inline comments. May I use comments going prior
> to corresponding field?

I believe we can excuse Mike's brevity in comments and use comments
going prior to the fields.

> 

<snipped>

> > > +	/*
> > > +	 * Overall number of snap restores (and number of stopped
> > 
> > Strictly saying this is not true, since execution can leave the trace
> > without restoring the snapshot (via <lj_vm_exit_interp>).
> 
> Is it correct to say that it is equal to amount of guard assertions
> leading to stopping trace executions?

I guess so, but see no reason for it. Nevertheless feel free to adjust
the comment the way you want.

> 

<snipped>

> > 
> > And last but not least: what about tests?

Ping? Hope to see them in v3 series.

> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API

[Sergey Kaplun]

Igor,

On 03.09.20, Igor Munkin wrote:
> Sergey,
> 
> On 03.09.20, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the review!
> > 
> 
> <snipped>
> 
> > > 
> > > This comment relates to all created files: I see no reason to violate
> > > LuaJIT code style, so please adjust the sources considering the current
> > > practices.
> > 
> > Should I use 2 space indent?
> 
> Yep, but don't forget tabs for 8-space indent (dunno, whether we can
> call such indenting "fake quarter tabs").

Well, I'll use fake quarter tabs here :)

> 
> > 
> > > 
> > > >  Makefile           |  2 +-
> > > >  src/Makefile       |  5 ++--
> > > >  src/Makefile.dep   |  3 ++
> > > >  src/lib_init.c     |  2 ++
> > > >  src/lib_misc.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  src/lj_misc_capi.c | 59 ++++++++++++++++++++++++++++++++++++
> > > >  src/lmisclib.h     | 61 +++++++++++++++++++++++++++++++++++++
> > > >  src/luaconf.h      |  1 +
> > > >  8 files changed, 205 insertions(+), 3 deletions(-)
> > > >  create mode 100644 src/lib_misc.c
> > > >  create mode 100644 src/lj_misc_capi.c
> > > >  create mode 100644 src/lmisclib.h
> > > > 
> 
> <snipped>
> 
> > 
> > > Well, I've already mentioned my concerns while discussing offline the
> > > first series. Since I see no corresponding changes, I leave them in this
> > > reply.
> > > 
> > > User will face precision loss if any counter exceeds 1 << 53. Consider
> > > the following example:
> > > | $ luajit -e 'print(2^53 == 2^53 + 1)'
> > > | true
> > > 
> > > I guess some counters (e.g. gc_freed and gc_allocated) should be cdata
> > > 64-bit numbers instead of the Lua (i.e. doubles) ones. Thoughts?
> > 
> > It's interesting point. In the one way this changes really omit
> > precision loss of data. But also when we build LuaJIT with
> > <-DLUAJIT_DISABLE_FFI> we should return number type instead of cdata
> > again. It leads to inconsistent behaviour of one function depends on
> > build type.
> 
> Yes, didn't take this fact into account.
> 
> > 
> > In the other way we can loss maximum ~2^10, IINM:
> > | $ luajit -e 'print(2^63 == 2^63 + 1024)'
> > | true
> > | $ luajit -e 'print(2^63 == 2^63 + 1025)'
> > | false
> > 
> > It means that users can't distinguish between values with a 1Kb
> > difference.  But for so fast growing up <gc_freed> and <gc_allocated>
> > (2^63 bytes) value it shouldn't become a very big problem to them. It's
> > really rare situation when you need get your metrics so frequent, that
> > you fail to allocate/free 1Kb of objects in due time.
> 
> Well, then this fact should be precisely mentioned in the RFC to be
> showed to Lua devs. If precision loss doesn't bother users, I'm for the
> current way (i.e. Lua numbers) since it makes less pressure on GC.
> 

I'll add this mention to RFC v3.

> > 
> > P.S. We can s/size_t/uint64_t/ to guarantee maximum fields value.
> 
> IIRC they have the same size, don't they?

Formally not necessarily. As mentioned at C11 standard (ISO/IEC
9899:2011), 7.20.3 Limits of other integer types:
| Its implementation-defined  value  shall  be  equal  to  or  greater
| in  magnitude (absolute  value)  than  the  corresponding  value  given
| below,  with  the  same  sign.

So it is possible that this value is greater than uint64_t in some
implementations. And we can loss more than 1Kb here.

But we can just use <size_t> for reasons of common sense.

> 
> > 
> 
> <snipped>
> 
> > > 
> > > Why do you mix inline comments with those going prior to the field?
> > > Please choose a single way to write comments here.
> > 
> > LuaJIT code style uses inline comments. May I use comments going prior
> > to corresponding field?
> 
> I believe we can excuse Mike's brevity in comments and use comments
> going prior to the fields.
> 

Good news :)

> > 
> 
> <snipped>
> 
> > > > +	/*
> > > > +	 * Overall number of snap restores (and number of stopped
> > > 
> > > Strictly saying this is not true, since execution can leave the trace
> > > without restoring the snapshot (via <lj_vm_exit_interp>).
> > 
> > Is it correct to say that it is equal to amount of guard assertions
> > leading to stopping trace executions?
> 
> I guess so, but see no reason for it. Nevertheless feel free to adjust
> the comment the way you want.

As Sergos mentioned here [1], this comment should be more descriptive
for Lua users. I agreed with him -- this metric should be characterized
more clearly than just "Overall number of snap restores".

> 
> > 
> 
> <snipped>
> 
> > > 
> > > And last but not least: what about tests?
> 
> Ping? Hope to see them in v3 series.

Oh, sorry! Of course I'll add test for this patch in v3.

> 
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018823.html

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH v2] rfc: luajit metrics

[Sergey Kaplun]

Part of #5187
---

This patch adds RFC to LuaJIT metrics interfaces. Nevertheless name
`misc` for builtin library is not good and should be discussed, because
tons of user modules can use that name for their own libraies.

Branch: https://github.com/tarantool/tarantool/tree/skaplun/5187-luajit-metrics
Issue: https://github.com/tarantool/tarantool/issues/5187

Changes in v2:
- Fixed typos
- Made comments more verbose
- Avoided flushing any of metrics after each call of luaM_metrics()

 doc/rfc/5187-luajit-metrics.md | 126 +++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 doc/rfc/5187-luajit-metrics.md

diff --git a/doc/rfc/5187-luajit-metrics.md b/doc/rfc/5187-luajit-metrics.md
new file mode 100644
index 000000000..2bd64cff4
--- /dev/null
+++ b/doc/rfc/5187-luajit-metrics.md
@@ -0,0 +1,126 @@
+# LuaJIT metrics
+
+* **Status**: In progress
+* **Start date**: 17-07-2020
+* **Authors**: Sergey Kaplun @Buristan skaplun@tarantool.org,
+               Igor Munkin @igormunkin imun@tarantool.org,
+               Sergey Ostanevich @sergos sergos@tarantool.org
+* **Issues**: [#5187](https://github.com/tarantool/tarantool/issues/5187)
+
+## Summary
+
+LuaJIT metrics provide extra information about the Lua state. They consists of
+GC metrics (overall amount of objects and memory usage), JIT stats (both
+related to the compiled traces and the engine itself), string hash hits/misses.
+
+## Background and motivation
+
+One can be curious about their application performance. We are going to provide
+various metrics about the several platform subsystems behaviour. GC pressure
+produced by user code can weight down all application performance. Irrelevant
+traces compiled by the JIT engine can just burn CPU time with no benefits as a
+result. String hash collisions can lead to DoS caused by a single request. All
+these metrics should be well monitored by users wanting to improve the
+performance of their application.
+
+## Detailed design
+
+For C API we introduce additional extension header <lmisclib.h> that provides
+interfaces for new LuaJIT C API extensions. The first interface in this header
+will be the following:
+
+```
+/* API for obtaining various metrics from the platform. */
+
+LUAM_API struct luam_Metrics *luaM_metrics(lua_State *L,
+					   struct luam_Metrics *dst);
+```
+
+This function fills the structure pointed by `dst` with the corresponding
+metrics related to Lua state anchored to the given coroutine `L`. The result of
+the function is a pointer to the filled structure (the same `dst` points to).
+
+The `struct luam_Metrics` has the following definition:
+
+```
+struct luam_Metrics {
+	/*
+	 * Strings amount founded in string hash
+	 * instead of allocation of new one.
+	 */
+	size_t strhash_hit;
+	/* Strings amount allocated and put into string hash. */
+	size_t strhash_miss;
+
+	size_t strnum;   /* Amount of allocated string objects. */
+	size_t tabnum;   /* Amount of allocated table objects. */
+	size_t udatanum; /* Amount of allocated udata objects. */
+	size_t cdatanum; /* Amount of allocated cdata objects. */
+
+	/* Memory currently allocated. */
+	size_t gc_total;
+	/* Total amount of freed memory. */
+	size_t gc_freed;
+	/* Total amount of allocated memory. */
+	size_t gc_allocated;
+
+	/* Count of incremental GC steps per state. */
+	size_t gc_steps_pause;
+	size_t gc_steps_propagate;
+	size_t gc_steps_atomic;
+	size_t gc_steps_sweepstring;
+	size_t gc_steps_sweep;
+	size_t gc_steps_finalize;
+
+	/*
+	 * Overall number of snap restores (and number of stopped
+	 * trace executions) for given jit_State.
+	 */
+	size_t jit_snap_restore;
+	/* Overall number of abort traces for given jit_State. */
+	size_t jit_trace_abort;
+	/* Total size of all allocated machine code areas. */
+	size_t jit_mcode_size;
+	/* Amount of JIT traces. */
+	unsigned int jit_trace_num;
+};
+```
+
+All metrics are collected throughout the platform uptime. But some of them
+(namely `strhash_hit`, `strhash_miss`, `gc_freed`, `gc_allocated`,
+`gc_steps_pause`, `gc_steps_propagate`, `gc_steps_atomic`,
+`gc_steps_sweepstring`, `gc_steps_sweep`, `gc_steps_finalize`,
+`jit_snap_restore` and `jit_trace_abort`) increase monotonic and can overflow.
+They make sense only with comparing with their value from a previous
+`luaM_metrics()` call.
+
+There is also a complement introduced for Lua space -- `misc.getmetrics()`.
+This function is just a wrapper for `luaM_metrics()` returning a Lua table with
+the similar metrics. Its usage is quite simple:
+```
+$ ./src/tarantool
+Tarantool 2.5.0-267-gbf047ad44
+type 'help' for interactive help
+tarantool> misc.getmetrics()
+---
+- tabnum: 1812
+  gc_total: 1369927
+  strnum: 5767
+  jit_trace_num: 0
+  cdatanum: 89
+  jit_mcode_size: 0
+  udatanum: 17
+  jit_snap_restore: 0
+  gc_freed: 2239391
+  strhash_hit: 53759
+  gc_steps_finalize: 0
+  gc_allocated: 3609318
+  gc_steps_atomic: 6
+  gc_steps_sweep: 296
+  gc_steps_sweepstring: 17920
+  jit_trace_abort: 0
+  strhash_miss: 6874
+  gc_steps_propagate: 10106
+  gc_steps_pause: 7
+...
+```
-- 
2.24.1

Re: [Tarantool-patches] [PATCH v2] rfc: luajit metrics

[Igor Munkin]

Sergey,

Thanks, this RFC is almost great! Please consider my comments below.

On 26.07.20, Sergey Kaplun wrote:
> Part of #5187
> ---
> 
> This patch adds RFC to LuaJIT metrics interfaces. Nevertheless name
> `misc` for builtin library is not good and should be discussed, because
> tons of user modules can use that name for their own libraies.
> 
> Branch: https://github.com/tarantool/tarantool/tree/skaplun/5187-luajit-metrics
> Issue: https://github.com/tarantool/tarantool/issues/5187
> 
> Changes in v2:
> - Fixed typos
> - Made comments more verbose
> - Avoided flushing any of metrics after each call of luaM_metrics()
> 
>  doc/rfc/5187-luajit-metrics.md | 126 +++++++++++++++++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 doc/rfc/5187-luajit-metrics.md
> 
> diff --git a/doc/rfc/5187-luajit-metrics.md b/doc/rfc/5187-luajit-metrics.md
> new file mode 100644
> index 000000000..2bd64cff4
> --- /dev/null
> +++ b/doc/rfc/5187-luajit-metrics.md
> @@ -0,0 +1,126 @@
> +# LuaJIT metrics
> +
> +* **Status**: In progress
> +* **Start date**: 17-07-2020
> +* **Authors**: Sergey Kaplun @Buristan skaplun@tarantool.org,
> +               Igor Munkin @igormunkin imun@tarantool.org,
> +               Sergey Ostanevich @sergos sergos@tarantool.org
> +* **Issues**: [#5187](https://github.com/tarantool/tarantool/issues/5187)
> +
> +## Summary
> +
> +LuaJIT metrics provide extra information about the Lua state. They consists of

Typo: s/consists/consist/.

> +GC metrics (overall amount of objects and memory usage), JIT stats (both
> +related to the compiled traces and the engine itself), string hash hits/misses.
> +
> +## Background and motivation
> +
> +One can be curious about their application performance. We are going to provide
> +various metrics about the several platform subsystems behaviour. GC pressure
> +produced by user code can weight down all application performance. Irrelevant
> +traces compiled by the JIT engine can just burn CPU time with no benefits as a
> +result. String hash collisions can lead to DoS caused by a single request. All
> +these metrics should be well monitored by users wanting to improve the
> +performance of their application.
> +
> +## Detailed design
> +
> +For C API we introduce additional extension header <lmisclib.h> that provides
> +interfaces for new LuaJIT C API extensions. The first interface in this header
> +will be the following:

I propose the following rewording:
| The additional header <lmisclib.h> is introduced to extend the existing
| LuaJIT C API with new interfaces. The first function provided via this
| header is the following:

> +
> +```
> +/* API for obtaining various metrics from the platform. */

Typo: s/metrics from the platform/platform metrics/.

> +
> +LUAM_API struct luam_Metrics *luaM_metrics(lua_State *L,
> +					   struct luam_Metrics *dst);

Please, address the comments I left regarding the function signature
here[1].

> +```
> +
> +This function fills the structure pointed by `dst` with the corresponding
> +metrics related to Lua state anchored to the given coroutine `L`. The result of
> +the function is a pointer to the filled structure (the same `dst` points to).
> +
> +The `struct luam_Metrics` has the following definition:
> +
> +```
> +struct luam_Metrics {

Please, address the comments I left regarding the structure definition
here[1].

> +	/*
> +	 * Strings amount founded in string hash
> +	 * instead of allocation of new one.
> +	 */
> +	size_t strhash_hit;
> +	/* Strings amount allocated and put into string hash. */
> +	size_t strhash_miss;
> +
> +	size_t strnum;   /* Amount of allocated string objects. */
> +	size_t tabnum;   /* Amount of allocated table objects. */
> +	size_t udatanum; /* Amount of allocated udata objects. */
> +	size_t cdatanum; /* Amount of allocated cdata objects. */
> +
> +	/* Memory currently allocated. */
> +	size_t gc_total;
> +	/* Total amount of freed memory. */
> +	size_t gc_freed;
> +	/* Total amount of allocated memory. */
> +	size_t gc_allocated;
> +
> +	/* Count of incremental GC steps per state. */
> +	size_t gc_steps_pause;
> +	size_t gc_steps_propagate;
> +	size_t gc_steps_atomic;
> +	size_t gc_steps_sweepstring;
> +	size_t gc_steps_sweep;
> +	size_t gc_steps_finalize;
> +
> +	/*
> +	 * Overall number of snap restores (and number of stopped
> +	 * trace executions) for given jit_State.
> +	 */
> +	size_t jit_snap_restore;
> +	/* Overall number of abort traces for given jit_State. */
> +	size_t jit_trace_abort;
> +	/* Total size of all allocated machine code areas. */
> +	size_t jit_mcode_size;
> +	/* Amount of JIT traces. */
> +	unsigned int jit_trace_num;
> +};
> +```
> +
> +All metrics are collected throughout the platform uptime. But some of them
> +(namely `strhash_hit`, `strhash_miss`, `gc_freed`, `gc_allocated`,
> +`gc_steps_pause`, `gc_steps_propagate`, `gc_steps_atomic`,
> +`gc_steps_sweepstring`, `gc_steps_sweep`, `gc_steps_finalize`,
> +`jit_snap_restore` and `jit_trace_abort`) increase monotonic and can overflow.

Ouch, let's list these metrics in a bullet list for better readability.

Typo: s/monotonic/monotonically/.

> +They make sense only with comparing with their value from a previous
> +`luaM_metrics()` call.
> +
> +There is also a complement introduced for Lua space -- `misc.getmetrics()`.
> +This function is just a wrapper for `luaM_metrics()` returning a Lua table with
> +the similar metrics. Its usage is quite simple:
> +```
> +$ ./src/tarantool
> +Tarantool 2.5.0-267-gbf047ad44
> +type 'help' for interactive help
> +tarantool> misc.getmetrics()
> +---
> +- tabnum: 1812
> +  gc_total: 1369927
> +  strnum: 5767
> +  jit_trace_num: 0
> +  cdatanum: 89
> +  jit_mcode_size: 0
> +  udatanum: 17
> +  jit_snap_restore: 0
> +  gc_freed: 2239391
> +  strhash_hit: 53759
> +  gc_steps_finalize: 0
> +  gc_allocated: 3609318
> +  gc_steps_atomic: 6
> +  gc_steps_sweep: 296
> +  gc_steps_sweepstring: 17920
> +  jit_trace_abort: 0
> +  strhash_miss: 6874
> +  gc_steps_propagate: 10106
> +  gc_steps_pause: 7
> +...
> +```
> -- 
> 2.24.1
> 

Otherwise, LGTM.


[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019208.html

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2] rfc: luajit metrics

[Sergey Kaplun]

Igor,

Thanks for your review!

On 27.08.20, Igor Munkin wrote:
> Sergey,
> 
> Thanks, this RFC is almost great! Please consider my comments below.
> 
> On 26.07.20, Sergey Kaplun wrote:
> > Part of #5187
> > ---
> > 
> > This patch adds RFC to LuaJIT metrics interfaces. Nevertheless name
> > `misc` for builtin library is not good and should be discussed, because
> > tons of user modules can use that name for their own libraies.
> > 
> > Branch: https://github.com/tarantool/tarantool/tree/skaplun/5187-luajit-metrics
> > Issue: https://github.com/tarantool/tarantool/issues/5187
> > 
> > Changes in v2:
> > - Fixed typos
> > - Made comments more verbose
> > - Avoided flushing any of metrics after each call of luaM_metrics()
> > 
> >  doc/rfc/5187-luajit-metrics.md | 126 +++++++++++++++++++++++++++++++++
> >  1 file changed, 126 insertions(+)
> >  create mode 100644 doc/rfc/5187-luajit-metrics.md
> > 
> > diff --git a/doc/rfc/5187-luajit-metrics.md b/doc/rfc/5187-luajit-metrics.md
> > new file mode 100644
> > index 000000000..2bd64cff4
> > --- /dev/null
> > +++ b/doc/rfc/5187-luajit-metrics.md
> > @@ -0,0 +1,126 @@
> > +# LuaJIT metrics
> > +
> > +* **Status**: In progress
> > +* **Start date**: 17-07-2020
> > +* **Authors**: Sergey Kaplun @Buristan skaplun@tarantool.org,
> > +               Igor Munkin @igormunkin imun@tarantool.org,
> > +               Sergey Ostanevich @sergos sergos@tarantool.org
> > +* **Issues**: [#5187](https://github.com/tarantool/tarantool/issues/5187)
> > +
> > +## Summary
> > +
> > +LuaJIT metrics provide extra information about the Lua state. They consists of
> 
> Typo: s/consists/consist/.

Thanks!

> 
> > +GC metrics (overall amount of objects and memory usage), JIT stats (both
> > +related to the compiled traces and the engine itself), string hash hits/misses.
> > +
> > +## Background and motivation
> > +
> > +One can be curious about their application performance. We are going to provide
> > +various metrics about the several platform subsystems behaviour. GC pressure
> > +produced by user code can weight down all application performance. Irrelevant
> > +traces compiled by the JIT engine can just burn CPU time with no benefits as a
> > +result. String hash collisions can lead to DoS caused by a single request. All
> > +these metrics should be well monitored by users wanting to improve the
> > +performance of their application.
> > +
> > +## Detailed design
> > +
> > +For C API we introduce additional extension header <lmisclib.h> that provides
> > +interfaces for new LuaJIT C API extensions. The first interface in this header
> > +will be the following:
> 
> I propose the following rewording:
> | The additional header <lmisclib.h> is introduced to extend the existing
> | LuaJIT C API with new interfaces. The first function provided via this
> | header is the following:
> 

Sounds good, thanks!

> > +
> > +```
> > +/* API for obtaining various metrics from the platform. */
> 
> Typo: s/metrics from the platform/platform metrics/.

Thanks!

> 
> > +
> > +LUAM_API struct luam_Metrics *luaM_metrics(lua_State *L,
> > +					   struct luam_Metrics *dst);
> 
> Please, address the comments I left regarding the function signature
> here[1].
> 

Sure!

> > +```
> > +
> > +This function fills the structure pointed by `dst` with the corresponding
> > +metrics related to Lua state anchored to the given coroutine `L`. The result of
> > +the function is a pointer to the filled structure (the same `dst` points to).
> > +
> > +The `struct luam_Metrics` has the following definition:
> > +
> > +```
> > +struct luam_Metrics {
> 
> Please, address the comments I left regarding the structure definition
> here[1].

Sure!

> 
> > +	/*
> > +	 * Strings amount founded in string hash
> > +	 * instead of allocation of new one.
> > +	 */
> > +	size_t strhash_hit;
> > +	/* Strings amount allocated and put into string hash. */
> > +	size_t strhash_miss;
> > +
> > +	size_t strnum;   /* Amount of allocated string objects. */
> > +	size_t tabnum;   /* Amount of allocated table objects. */
> > +	size_t udatanum; /* Amount of allocated udata objects. */
> > +	size_t cdatanum; /* Amount of allocated cdata objects. */
> > +
> > +	/* Memory currently allocated. */
> > +	size_t gc_total;
> > +	/* Total amount of freed memory. */
> > +	size_t gc_freed;
> > +	/* Total amount of allocated memory. */
> > +	size_t gc_allocated;
> > +
> > +	/* Count of incremental GC steps per state. */
> > +	size_t gc_steps_pause;
> > +	size_t gc_steps_propagate;
> > +	size_t gc_steps_atomic;
> > +	size_t gc_steps_sweepstring;
> > +	size_t gc_steps_sweep;
> > +	size_t gc_steps_finalize;
> > +
> > +	/*
> > +	 * Overall number of snap restores (and number of stopped
> > +	 * trace executions) for given jit_State.
> > +	 */
> > +	size_t jit_snap_restore;
> > +	/* Overall number of abort traces for given jit_State. */
> > +	size_t jit_trace_abort;
> > +	/* Total size of all allocated machine code areas. */
> > +	size_t jit_mcode_size;
> > +	/* Amount of JIT traces. */
> > +	unsigned int jit_trace_num;
> > +};
> > +```
> > +
> > +All metrics are collected throughout the platform uptime. But some of them
> > +(namely `strhash_hit`, `strhash_miss`, `gc_freed`, `gc_allocated`,
> > +`gc_steps_pause`, `gc_steps_propagate`, `gc_steps_atomic`,
> > +`gc_steps_sweepstring`, `gc_steps_sweep`, `gc_steps_finalize`,
> > +`jit_snap_restore` and `jit_trace_abort`) increase monotonic and can overflow.
> 
> Ouch, let's list these metrics in a bullet list for better readability.
> 
> Typo: s/monotonic/monotonically/.
> 

Yes, it will be better.

> > +They make sense only with comparing with their value from a previous
> > +`luaM_metrics()` call.
> > +
> > +There is also a complement introduced for Lua space -- `misc.getmetrics()`.
> > +This function is just a wrapper for `luaM_metrics()` returning a Lua table with
> > +the similar metrics. Its usage is quite simple:
> > +```
> > +$ ./src/tarantool
> > +Tarantool 2.5.0-267-gbf047ad44
> > +type 'help' for interactive help
> > +tarantool> misc.getmetrics()
> > +---
> > +- tabnum: 1812
> > +  gc_total: 1369927
> > +  strnum: 5767
> > +  jit_trace_num: 0
> > +  cdatanum: 89
> > +  jit_mcode_size: 0
> > +  udatanum: 17
> > +  jit_snap_restore: 0
> > +  gc_freed: 2239391
> > +  strhash_hit: 53759
> > +  gc_steps_finalize: 0
> > +  gc_allocated: 3609318
> > +  gc_steps_atomic: 6
> > +  gc_steps_sweep: 296
> > +  gc_steps_sweepstring: 17920
> > +  jit_trace_abort: 0
> > +  strhash_miss: 6874
> > +  gc_steps_propagate: 10106
> > +  gc_steps_pause: 7
> > +...
> > +```
> > -- 
> > 2.24.1
> > 
> 
> Otherwise, LGTM.
> 
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019208.html
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun