[Tarantool-patches] [PATCH 1/2] metrics: add counters for metrics interested in

[Igor Munkin]

Sergey,

On 24.07.20, Sergey Kaplun wrote:
> Hi Igor! Thanks for the review!
> 
> On 23.07.20, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! Please consider my comments below.
> > 
> > On 21.07.20, Sergey Kaplun wrote:
> > 
> > We still don't have strict prefixes, so I propose to use 'core' for this
> > patch. Does it look fine to you? Feel free to propose your own one.
> 
> Maybe we should use 'misc' for this kind of patches?

Pardon, I don't get your proposal:
* if you refer the introduced <misc> library, then no, cause this
  patch doesn't relate to it
* if you refer to "miscellaneous changes", then also no, cause this
  patch directly related to the core components of platform runtime
  (e.g. GC machinery, string interning and lookup, JIT engine).

> 

<snipped>

> 
> > > +
> > >    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..1fee04f 100644
> > > --- a/src/lj_obj.h
> > > +++ b/src/lj_obj.h
> > > @@ -571,13 +571,28 @@ typedef enum {
> > >  #define basemt_obj(g, o)	((g)->gcroot[GCROOT_BASEMT+itypemap(o)])
> > >  #define mmname_str(g, mm)	(strref((g)->gcroot[GCROOT_MMNAME+(mm)]))
> > >  
> > > +/* Garbage collector states. Order matters. */
> > > +enum {
> > > +  GCSpause,
> > > +  GCSpropagate,
> > > +  GCSatomic,
> > > +  GCSsweepstring,
> > > +  GCSsweep,
> > > +  GCSfinalize,
> > > +  GCSlast
> > 
> > Minor: GCSMAX looks better, doesn't it? Feel free to ignore.
> 
> Really, GCSlast is unreachable. GCSmax for this kind of naming is good
> enough. GCSunreachable also looks nice. What do you think?

GCSmax and GCSMAX are totally fine. Use the most convenient to you.

> 
> > > +};
> > > +
> > >  typedef struct GCState {
> > >    GCSize total;		/* Memory currently allocated. */
> > > +  size_t freed;		/* Memory freed since last luaM_metrics() call. */
> > > +  size_t allocated;	/* Memory allocated since last luaM_metrics() call. */
> > 
> > Well, why <total> field type differs from <freed> and <allocated> fields
> > type? And again, remark about <luaM_metrics> can be dropped (see above).
> 
> GCSize for total memfory usage is defines by LJ_GC64 macro definition.
> | #if LJ_GC64
> | typedef uint64_t GCSize;
> | #else
> | typedef uint32_t GCSize;
> | #endif
> 
> But summarized allocation size can be many times over maximum memory
> size). So these field types should be independet.

OK, thanks, now I see. However, there is another issue with these
fields: if LJ_GC64 is disabled (AFAIR any Tarantool build except MacOS
ones) the structure will have unused pad after <total> field. Otherwise,
if LuaJIT is build in LJ_GC64 mode, such pad occurs after <unused2>
field due to <state_count> field. Please reorder the fields considering
the structure alignment.

> 
> > >    GCSize threshold;	/* Memory threshold. */
> > >    uint8_t currentwhite;	/* Current white color. */
> > >    uint8_t state;	/* GC state. */
> > >    uint8_t nocdatafin;	/* No cdata finalizer called. */
> > >    uint8_t unused2;
> > > +  size_t state_count[GCSlast]; /* Count of GC invocations with different states
> > > +			       ** since previous call of luaM_metrics().  */

<snipped>

> > >    MSize sweepstr;	/* Sweep position in string table. */
> > >    GCRef root;		/* List of all collectable objects. */
> > >    MRef sweep;		/* Sweep position in root list. */
> > > @@ -589,6 +604,12 @@ typedef struct GCState {
> > >    GCSize estimate;	/* Estimate of memory actually in use. */
> > >    MSize stepmul;	/* Incremental GC step granularity. */
> > >    MSize pause;		/* Pause between successive GC cycles. */
> > > +
> > > +  size_t tabnum;	/* Current amount of table objects. */
> > > +  size_t udatanum;	/* Current amount of udata objects. */
> > 
> > Minor: These comments are a bit misleading. "Current" sounds like the
> > metric is reset somewhen. But you're talking about "alive" objects of
> > the particular type. Yes, formally (in GC terms) some of them are not
> > alive, but are still allocated (so called "dead") objects. So the term
> > "alive" is also a misleading one. I guess you can freely reword the
> > comments the following way:
> > | /* Number of allocated <GCtype> objects */
> > 
> 
> I think that "allocated" is unnecessarily. We can omit it easily.

Side note: The comment is better, but I still don't agree that
"allocated" can be easily omitted. Since it's a side note, no changes
are obliged.

> 

<snipped>

> 
> > > +#endif
> > >  } GCState;
> > >  
> > >  /* Global state, shared by all threads of a Lua universe. */
> > > @@ -602,6 +623,10 @@ typedef struct global_State {
> > >      BloomFilter next[2];
> > >    } strbloom;
> > >  #endif
> > > +  size_t strhash_hit;	/* New string has been found in the storage
> > > +			** since last luaM_metrics() call. */
> > > +  size_t strhash_miss;	/* New string has been added to the storage
> > > +			** since last luaM_metrics() call. */
> > 
> > Well, such indentation is closer to the desired one, but I would use two
> > spaces prior to these tabs. What do you think? Also please, don't forget
> > about <luaM_metrics> remark.
> 
> Sorry, but ^\s+\t+\s+ stylistics looks very bad. In *.c files Mike use
> exactly ^\t+ style for code, so we have to use the same for comments.
> 
> Thoughts?

I failed to find multiline comments for structures. So if you need one,
it looks more convenient to me to save the original indent of the
structure (i.e. spaces) and then align the wording with tabs considering
the first line. Or make the situation much easier to you and other
developers: just avoid multiline inline comments :)

> 
> > >    lua_Alloc allocf;	/* Memory allocator. */
> > >    void *allocd;		/* Memory allocator data. */
> > >    GCState gc;		/* Garbage collector. */

<snipped>

> > > -- 
> > > 2.24.1
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018845.html
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM