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

[Igor Munkin <imun@tarantool.org>]

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