From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.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 07A1A469719 for ; Thu, 8 Oct 2020 13:11:48 +0300 (MSK) Date: Thu, 8 Oct 2020 13:11:23 +0300 From: Sergey Kaplun Message-ID: <20201008101123.GB11972@root> References: <2280bc3a2e32356455c3aebae711bafe2c4332f5.1601878708.git.skaplun@tarantool.org> <20201007141106.GP18920@tarantool.org> <20201007195558.GA20188@root> <20201007201601.GR18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201007201601.GR18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 1/2] core: introduce various platform metrics 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 07.10.20, Igor Munkin wrote: > Sergey, > > Thanks for your fixes! There is still a comment regarding CNEW > assembling and a couple minors below. > > On 07.10.20, Sergey Kaplun wrote: > > On 07.10.20, Igor Munkin wrote: > > > Sergey, > > > > > > Thanks for the patch! Please consider my comments below. > > > > > > On 05.10.20, Sergey Kaplun wrote: > > > > > > > > > > > > + emit_setgl(as, RID_RET+2, gc.cdatanum); > > > > > > Well, I glanced a MIPS register-usage convention and AFAICS $4 register > > > (RID_RET + 2) is a general-purpose (i.e. doesn't store 0 or preserved by > > > kernel) caller-safe one. Ergo it should be allocated it in a proper way > > > from scratch set, shouldn't it? > > > > > > > AFAIK, $a0 - $a3 ($4 - $7) registers are arguments to functions - not > > preserved by subprograms. > > Yes, but there is e.g. $8, that is temporary one, isn't it? Anyway, you Yes, for example. Side note: We can omit the call to the register allocator, since these registers can change during the call, but in order not to be tied to ABI we can use a lightweight `ra_scratch`, indeed. > can't just pick the particular register, since it can be already > allocated by RA. So it *has* to be explicitly allocated to avoid data > clash on the trace. I strongly believe the reason you see no failure on > tests is simply a lucky coincidence (or tiny traces). > > > But anyway explicit allocation is better here. Added. > > > > > > /* Initialize gct and ctypeid. lj_mem_newgco() already sets marked. */ > > > > > > > > > I've changed commit message as follows: > > > > =================================================================== > > core: introduce various platform metrics > > > > This patch introduces the following counters: > > - overall amount of allocated tables, cdata and udata objects > > - number of incremental GC steps grouped by GC state > > - number of string hashes hits and misses > > - amount of allocated and freed memory > > - number of trace aborts, number of traces and restored snapshots > > > > Also this patch fixes alignment for 64-bit architectures. > > > > NB: MSize and BCIns are the only fixed types that equal 32 bits. GCRef, > > MRef and GCSize sizes depend on LJ_GC64 define. > > > > struct GCState is terminated by three fields: GCSize estimate, MSize > > stepmul and MSize pause, which are aligned. The introduces size_t > > Typo: s/introduces/introduced/. Thanks! Updated on branch. > > > fields do not violate the alignment too. > > =================================================================== > > > > [1]: https://alisdair.mcdiarmid.org/arm-immediate-value-encoding/ > > > > -- > > Best regards, > > Sergey Kaplun > > -- > Best regards, > IM -- Best regards, Sergey Kaplun