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

[Sergey Kaplun <skaplun@tarantool.org>]

On 24.07.20, Igor Munkin wrote:
> 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).
> 

You are right, I'll use "core" here.

<snipped>
> > > > +};
> > > > +
> > > >  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.

Sure!

> > 
<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 :)

I suppose that one-line comment will satisfy both of us :)

> > 
> > > >    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

-- 
Best regards,
Sergey Kaplun