From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4FA8043040C for ; Fri, 4 Sep 2020 08:29:58 +0300 (MSK) Date: Fri, 4 Sep 2020 08:29:41 +0300 From: Sergey Kaplun Message-ID: <20200904052941.GA5215@root> References: <97bddbd3a946463b135fb6ae559b0d72d4f2148c.1595794764.git.skaplun@tarantool.org> <20200827182534.GB18920@tarantool.org> <20200903144415.GA15704@root> <20200903152237.GE18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200903152237.GE18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/2] metrics: add C and Lua API List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, On 03.09.20, Igor Munkin wrote: > Sergey, > > On 03.09.20, Sergey Kaplun wrote: > > Igor, > > > > Thanks for the review! > > > > > > > > > > > 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 > > > > > > > > > > > > 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 and > > (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 for reasons of common sense. > > > > > > > > > > > > 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 :) > > > > > > > > > + /* > > > > + * 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 ). > > > > 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". > > > > > > > > > > > > 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