Re: [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API

[Sergey Kaplun <skaplun@tarantool.org>]

Igor,

On 03.09.20, Igor Munkin wrote:
> Sergey,
> 
> On 03.09.20, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the review!
> > 
> 
> <snipped>
> 
> > > 
> > > This comment relates to all created files: I see no reason to violate
> > > LuaJIT code style, so please adjust the sources considering the current
> > > practices.
> > 
> > Should I use 2 space indent?
> 
> Yep, but don't forget tabs for 8-space indent (dunno, whether we can
> call such indenting "fake quarter tabs").

Well, I'll use fake quarter tabs here :)

> 
> > 
> > > 
> > > >  Makefile           |  2 +-
> > > >  src/Makefile       |  5 ++--
> > > >  src/Makefile.dep   |  3 ++
> > > >  src/lib_init.c     |  2 ++
> > > >  src/lib_misc.c     | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  src/lj_misc_capi.c | 59 ++++++++++++++++++++++++++++++++++++
> > > >  src/lmisclib.h     | 61 +++++++++++++++++++++++++++++++++++++
> > > >  src/luaconf.h      |  1 +
> > > >  8 files changed, 205 insertions(+), 3 deletions(-)
> > > >  create mode 100644 src/lib_misc.c
> > > >  create mode 100644 src/lj_misc_capi.c
> > > >  create mode 100644 src/lmisclib.h
> > > > 
> 
> <snipped>
> 
> > 
> > > Well, I've already mentioned my concerns while discussing offline the
> > > first series. Since I see no corresponding changes, I leave them in this
> > > reply.
> > > 
> > > User will face precision loss if any counter exceeds 1 << 53. Consider
> > > the following example:
> > > | $ luajit -e 'print(2^53 == 2^53 + 1)'
> > > | true
> > > 
> > > I guess some counters (e.g. gc_freed and gc_allocated) should be cdata
> > > 64-bit numbers instead of the Lua (i.e. doubles) ones. Thoughts?
> > 
> > It's interesting point. In the one way this changes really omit
> > precision loss of data. But also when we build LuaJIT with
> > <-DLUAJIT_DISABLE_FFI> we should return number type instead of cdata
> > again. It leads to inconsistent behaviour of one function depends on
> > build type.
> 
> Yes, didn't take this fact into account.
> 
> > 
> > In the other way we can loss maximum ~2^10, IINM:
> > | $ luajit -e 'print(2^63 == 2^63 + 1024)'
> > | true
> > | $ luajit -e 'print(2^63 == 2^63 + 1025)'
> > | false
> > 
> > It means that users can't distinguish between values with a 1Kb
> > difference.  But for so fast growing up <gc_freed> and <gc_allocated>
> > (2^63 bytes) value it shouldn't become a very big problem to them. It's
> > really rare situation when you need get your metrics so frequent, that
> > you fail to allocate/free 1Kb of objects in due time.
> 
> Well, then this fact should be precisely mentioned in the RFC to be
> showed to Lua devs. If precision loss doesn't bother users, I'm for the
> current way (i.e. Lua numbers) since it makes less pressure on GC.
> 

I'll add this mention to RFC v3.

> > 
> > P.S. We can s/size_t/uint64_t/ to guarantee maximum fields value.
> 
> IIRC they have the same size, don't they?

Formally not necessarily. As mentioned at C11 standard (ISO/IEC
9899:2011), 7.20.3 Limits of other integer types:
| Its implementation-defined  value  shall  be  equal  to  or  greater
| in  magnitude (absolute  value)  than  the  corresponding  value  given
| below,  with  the  same  sign.

So it is possible that this value is greater than uint64_t in some
implementations. And we can loss more than 1Kb here.

But we can just use <size_t> for reasons of common sense.

> 
> > 
> 
> <snipped>
> 
> > > 
> > > Why do you mix inline comments with those going prior to the field?
> > > Please choose a single way to write comments here.
> > 
> > LuaJIT code style uses inline comments. May I use comments going prior
> > to corresponding field?
> 
> I believe we can excuse Mike's brevity in comments and use comments
> going prior to the fields.
> 

Good news :)

> > 
> 
> <snipped>
> 
> > > > +	/*
> > > > +	 * Overall number of snap restores (and number of stopped
> > > 
> > > Strictly saying this is not true, since execution can leave the trace
> > > without restoring the snapshot (via <lj_vm_exit_interp>).
> > 
> > Is it correct to say that it is equal to amount of guard assertions
> > leading to stopping trace executions?
> 
> I guess so, but see no reason for it. Nevertheless feel free to adjust
> the comment the way you want.

As Sergos mentioned here [1], this comment should be more descriptive
for Lua users. I agreed with him -- this metric should be characterized
more clearly than just "Overall number of snap restores".

> 
> > 
> 
> <snipped>
> 
> > > 
> > > And last but not least: what about tests?
> 
> Ping? Hope to see them in v3 series.

Oh, sorry! Of course I'll add test for this patch in v3.

> 
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018823.html

-- 
Best regards,
Sergey Kaplun