From: Igor Munkin <imun@tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v4 1/2] core: introduce various platform metrics Date: Wed, 7 Oct 2020 23:16:01 +0300 [thread overview] Message-ID: <20201007201601.GR18920@tarantool.org> (raw) In-Reply-To: <20201007195558.GA20188@root> 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 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/. > fields do not violate the alignment too. > > vmstate 32-bit field goes right after GCState field within global_State > structure. The next field tmpbuf consists of several MRef fields that > have 64-bit size each. This issue can be solved by moving vmstate field > below. However DynASM doesn't work well with unaligned memory access on > 64-bit bigendian MIPS, so vmstate should be aligned to a 64-bit > boundary. > > Furthermore field order has been changed to be able to compile code by > DynASM for 32-bit ARM too (see also > https://github.com/openresty/luajit2/issues/37#issuecomment-459145226). > > Interfaces to obtain these metrics via both Lua and C API are > introduced in the next patch. > > Part of tarantool/tarantool#5187 > =================================================================== > > Side note: If you want read a little bit more about ARM immediate value > encoding (and play with it) see also [1]. Thanks. > <snipped> > > See iterative patch in the bottom. Branch force-pushed. > > =================================================================== <snipped> > diff --git a/src/lj_asm_mips.h b/src/lj_asm_mips.h > index f4b4b5d..0341701 100644 > --- a/src/lj_asm_mips.h > +++ b/src/lj_asm_mips.h > @@ -1430,7 +1430,9 @@ static void asm_cnew(ASMState *as, IRIns *ir) > CTInfo info = lj_ctype_info(cts, id, &sz); > const CCallInfo *ci = &lj_ir_callinfo[IRCALL_lj_mem_newgco]; > IRRef args[4]; > + RegSet allow = (RSET_GPR & ~RSET_SCRATCH); > RegSet drop = RSET_SCRATCH; > + Reg tmp; > lua_assert(sz != CTSIZE_INVALID || (ir->o == IR_CNEW && ir->op2 != REF_NIL)); > > as->gcsteps++; > @@ -1442,7 +1444,6 @@ static void asm_cnew(ASMState *as, IRIns *ir) > > /* Initialize immutable cdata object. */ > if (ir->o == IR_CNEWI) { > - RegSet allow = (RSET_GPR & ~RSET_SCRATCH); > #if LJ_32 > int32_t ofs = sizeof(GCcdata); > if (sz == 8) { > @@ -1473,15 +1474,16 @@ static void asm_cnew(ASMState *as, IRIns *ir) > return; > } > > + tmp = ra_scratch(as, allow); Since there are registers allocated in scope of IR_CNEWI assembling above, you need to exclude those registers from <allow> set prior to scratching a new one. > /* Code incrementing cdatanum is sparse to avoid mips data hazards. */ > - emit_setgl(as, RID_RET+2, gc.cdatanum); > + emit_setgl(as, tmp, gc.cdatanum); > /* Initialize gct and ctypeid. lj_mem_newgco() already sets marked. */ > emit_tsi(as, MIPSI_SB, RID_RET+1, RID_RET, offsetof(GCcdata, gct)); > emit_tsi(as, MIPSI_SH, RID_TMP, RID_RET, offsetof(GCcdata, ctypeid)); > - emit_tsi(as, MIPSI_AADDIU, RID_RET+2, RID_RET+2, 1); > + emit_tsi(as, MIPSI_AADDIU, tmp, tmp, 1); > emit_ti(as, MIPSI_LI, RID_RET+1, ~LJ_TCDATA); > emit_ti(as, MIPSI_LI, RID_TMP, id); /* Lower 16 bit used. Sign-ext ok. */ > - emit_getgl(as, RID_RET+2, gc.cdatanum); > + emit_getgl(as, tmp, gc.cdatanum); > args[0] = ASMREF_L; /* lua_State *L */ > args[1] = ASMREF_TMP1; /* MSize size */ > asm_gencall(as, ci, args); <snipped> > =================================================================== > > [1]: https://alisdair.mcdiarmid.org/arm-immediate-value-encoding/ > > -- > Best regards, > Sergey Kaplun -- Best regards, IM
next prev parent reply other threads:[~2020-10-07 20:26 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-05 6:30 [Tarantool-patches] [PATCH v4 0/2] Implement LuaJIT " Sergey Kaplun 2020-10-05 6:30 ` [Tarantool-patches] [PATCH v4 1/2] core: introduce various " Sergey Kaplun 2020-10-07 14:11 ` Igor Munkin 2020-10-07 19:55 ` Sergey Kaplun 2020-10-07 20:16 ` Igor Munkin [this message] 2020-10-08 9:28 ` Igor Munkin 2020-10-08 10:11 ` Sergey Kaplun 2020-10-08 12:44 ` Igor Munkin 2020-10-09 14:39 ` Sergey Ostanevich 2020-10-05 6:30 ` [Tarantool-patches] [PATCH v4 2/2] misc: add C and Lua API for " Sergey Kaplun 2020-10-06 22:17 ` Igor Munkin 2020-10-07 5:57 ` Igor Munkin 2020-10-07 14:35 ` Sergey Kaplun 2020-10-07 18:23 ` Igor Munkin 2020-10-07 20:09 ` Sergey Kaplun 2020-10-09 14:45 ` Sergey Ostanevich 2020-10-13 6:01 ` Sergey Kaplun 2020-10-05 6:30 ` [Tarantool-patches] [RFC v4] rfc: luajit metrics Sergey Kaplun 2020-10-07 14:46 ` Sergey Kaplun 2020-10-08 17:25 ` Igor Munkin 2020-10-08 19:29 ` Sergey Kaplun 2020-10-08 20:26 ` Igor Munkin 2020-10-09 6:06 ` Sergey Kaplun 2020-12-22 9:07 ` Kirill Yukhin 2020-10-08 17:33 ` [Tarantool-patches] [PATCH v4 0/2] Implement LuaJIT platform metrics Igor Munkin 2020-10-13 13:17 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201007201601.GR18920@tarantool.org \ --to=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v4 1/2] core: introduce various platform metrics' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox