Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin <imun@tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 1/2] core: introduce various platform metrics
Date: Wed, 26 Aug 2020 17:48:37 +0300	[thread overview]
Message-ID: <20200826144837.GA18920@tarantool.org> (raw)
In-Reply-To: <35a19def79a9cbc46dabdfa579869af9e4e589fb.1595794764.git.skaplun@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

  reply	other threads:[~2020-08-26 14:59 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-26 20:40 [Tarantool-patches] [PATCH v2 0/2] Implement LuaJIT " Sergey Kaplun
2020-07-26 20:40 ` [Tarantool-patches] [PATCH v2 1/2] core: introduce various " Sergey Kaplun
2020-08-26 14:48   ` Igor Munkin [this message]
2020-08-26 15:52     ` Sergey Ostanevich
2020-08-27 18:42       ` Igor Munkin
2020-09-03 12:51     ` Sergey Kaplun
2020-07-26 20:40 ` [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API Sergey Kaplun
2020-08-27 18:25   ` Igor Munkin
2020-09-03 14:44     ` Sergey Kaplun
2020-09-03 15:22       ` Igor Munkin
2020-09-04  5:29         ` Sergey Kaplun
2020-07-26 20:42 ` [Tarantool-patches] [PATCH v2] rfc: luajit metrics Sergey Kaplun
2020-08-27 19:18   ` Igor Munkin
2020-09-03 12:57     ` Sergey Kaplun

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200826144837.GA18920@tarantool.org \
    --to=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 1/2] core: introduce various platform metrics' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox