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

[Igor Munkin <imun@tarantool.org>]

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