From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 13AD2430409 for ; Thu, 3 Sep 2020 17:44:32 +0300 (MSK) Date: Thu, 3 Sep 2020 17:44:15 +0300 From: Sergey Kaplun Message-ID: <20200903144415.GA15704@root> References: <97bddbd3a946463b135fb6ae559b0d72d4f2148c.1595794764.git.skaplun@tarantool.org> <20200827182534.GB18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200827182534.GB18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, 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 > > 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 > | header provides C interface that fills the given > | structure with the platform metrics. Additionally > | module is loaded to Lua space and provides > | 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 > > > > > > > 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 in the middle of LJCORE_O? It's next to . 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. > > > > > 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)); > > 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 and (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 structure fields > ordering. It also differs from the order fields are filled > within . 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: 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 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 here. > > > + memset(dst, 0, sizeof(*dst)); > > I was misguided with this , 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 ). 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