Re: [Tarantool-patches] [PATCH v4 1/2] core: introduce various platform metrics

[Sergey Kaplun <skaplun@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:
> 
> <snipped>
> 
> > 
> > > 
> > > > +  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. */
> > 
> 
> <snipped>
> 
> > 
> > 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.

<snipped>

> > ===================================================================
> > 
> > [1]: https://alisdair.mcdiarmid.org/arm-immediate-value-encoding/
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun