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

[Igor Munkin]

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").

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

> 
> P.S. We can s/size_t/uint64_t/ to guarantee maximum fields value.

IIRC they have the same size, don't they?

> 

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

> 

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

> 

<snipped>

> > 
> > And last but not least: what about tests?

Ping? Hope to see them in v3 series.

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

-- 
Best regards,
IM