From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 1416D441840 for ; Wed, 1 Apr 2020 02:57:20 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 1 Apr 2020 01:57:17 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] fiber: abort trace recording on fiber yield List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin , Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 7 comments below. > diff --git a/src/lua/fiber.c b/src/lua/fiber.c > index 45bc03787..c79aa7c2b 100644 > --- a/src/lua/fiber.c > +++ b/src/lua/fiber.c > @@ -454,6 +455,25 @@ lua_fiber_run_f(MAYBE_UNUSED va_list ap) > return result; > } > > +static int > +fiber_on_yield(struct trigger *trigger, void *event) > +{ > + (void) trigger; > + (void) event; > + > + /* > + * XXX: According to LuaJIT API reference luaJIT_setmode 1. What is 'XXX'? > + * function returns 0 on failure and 1 on success. Since > + * this call is aimed to abort the trace recording on > + * fiber_yield call and prevent already compiled traces > + * execution, the mode parameter is an invalid value (-1) > + * so JIT mode change is expected to fail and 0 is > + * returned. Otherwise non-zero return value signals > + * trigger_run routine about this trigger failure. 2. So correct me if I am wrong - we deliberately fail this function assuming that its call stops current trace collection/execution, right? And we pass -1 because there is no other way to stop trace collection/execution except this function. Why can't we call lj_trace_abort() directly? And most importantly, how does it affect perf? New trigger is +1 virtual call on each yield of every Lua fiber and +1 execution of non-trival function luaJIT_setmode(). I think it is better to write a micro bench checking how many yields can we do per time unit before and after this patch. From Lua fibers. > + */ > + return luaJIT_setmode(tarantool_L, 0, -1); 3. In case that function ever returns 1, that breaks our error return convention, that error means -1 (it will work in triggers, since they do '!= 0', but anyway). Also when there is an error, diag is expected to be set. Better do diag_set(LuajitError) here when this fails. Or even panic() since it is not supposed to return 1 when the mode is bad. Or assume it is always 0 (add assert()), and return const 0 from the trigger. > +} > + > /** > * Utility function for fiber.create and fiber.new > */ > @@ -467,10 +487,18 @@ fiber_create(struct lua_State *L) > > struct fiber *f = fiber_new("lua", lua_fiber_run_f); > if (f == NULL) { > - luaL_unref(L, LUA_REGISTRYINDEX, coro_ref); > - luaT_error(L); > + /* diagnostics is set in fiber_new. */ > + goto error; > } > > + struct trigger *t = malloc(sizeof(*t)); > + if (t == NULL) { > + diag_set(OutOfMemory, sizeof(*t), "malloc", "t"); > + goto error; > + } > + trigger_create(t, fiber_on_yield, NULL, (trigger_f0) free); > + trigger_add(&f->on_yield, t); 4. Yeah, now I understand why did you ask about when triggers are deleted. In this particular case there is a better option. You can allocate the trigger right on the stack, in lua_fiber_run_f(). This is the trampoline for all Lua fibers, regardless how they are created: via fiber.new(), or fiber.create(). You can add the trigger before result = luaT_call(L, lua_gettop(L) - 1, LUA_MULTRET); And remove afterwards. No need to use the heap then, nor move some error handling code. 5. Also there is a question what will happen, if Lua is accessed from a fiber, created not in Lua? That is basically all pooled fibers, which serve IProto requests. See struct fiber_pool and its single usage place. These fibers are started from C, and may end up executing Lua code, if, for example, an IPROTO_CALL arrived. The same question can be applied to fibers, created from C API: API_EXPORT struct fiber * fiber_new(const char *name, fiber_func f); These fibers won't have the on_yield trigger, but can access Lua. For example, they can run Lua on_replace triggers in a space by making an insertion into it. Also they can access Lua API directly. By calling things like luaT_call() (see module.h). You may consider adding Jit collection/tracing termination to the scheduler fiber. It is just one fiber, and from what I know, every fiber after yield switches current thread to the scheduler fiber. This also maybe less expensive. We probably won't even notice. 6. There is also a question about how does it affect collection of valid traces? I don't know how Jit does it, so ask just in case. Won't this make traces never collected? Because every fiber yields soon or late. 7. Does this patch allow to yield in ffi?