From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp14.mail.ru (smtp14.mail.ru [94.100.181.95]) (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 94D0E445320 for ; Sat, 25 Jul 2020 13:00:10 +0300 (MSK) Date: Sat, 25 Jul 2020 13:00:03 +0300 From: Sergey Kaplun Message-ID: <20200725100003.GA27362@root> References: <20200721113451.25817-1-skaplun@tarantool.org> <20200721113451.25817-2-skaplun@tarantool.org> <20200723201219.GQ18920@tarantool.org> <20200724120000.GB4086@root> <20200724200501.GT18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200724200501.GT18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 1/2] metrics: add counters for metrics interested in List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.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 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. > > > > +}; > > > > + > > > > 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 field type differs from and fields > > > type? And again, remark about 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 field. Otherwise, > if LuaJIT is build in LJ_GC64 mode, such pad occurs after > field due to field. Please reorder the fields considering > the structure alignment. Sure! > > > > > > > > +#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 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. */ > > > > > > > -- > > > > 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