Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Ostanevich <sergos@tarantool.org>
To: Igor Munkin <imun@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 18:52:07 +0300	[thread overview]
Message-ID: <20200826155207.GA49@tarantool.org> (raw)
In-Reply-To: <20200826144837.GA18920@tarantool.org>

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

  reply	other threads:[~2020-08-26 15:52 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
2020-08-26 15:52     ` Sergey Ostanevich [this message]
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=20200826155207.GA49@tarantool.org \
    --to=sergos@tarantool.org \
    --cc=imun@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