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 7CEA3469719 for ; Thu, 17 Sep 2020 17:21:05 +0300 (MSK) References: <20200707222436.GG5559@tarantool.org> <20200907203502.GG18920@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 17 Sep 2020 16:21:03 +0200 MIME-Version: 1.0 In-Reply-To: <20200907203502.GG18920@tarantool.org> 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 Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the investigation! See 5 comments below. > diff --git a/src/lua/utils.c b/src/lua/utils.c > index 0b05d72..7d0962f 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -1308,3 +1308,7 @@ tarantool_lua_utils_init(struct lua_State *L) > luaT_newthread_ref = luaL_ref(L, LUA_REGISTRYINDEX); > return 0; > } > + > +void lua_on_yield(void) > +{ > +} > > ================================================================================ > > * Vanilla -> Patched [extern noop callback] (min, median, mean, max): > | fibers: 10; iters: 100 0% 2% 0% 0% > | fibers: 10; iters: 1000 1% 3% 1% -1% > | fibers: 10; iters: 10000 -1% 0% -1% -3% > | fibers: 10; iters: 100000 -2% 0% -1% 0% > | fibers: 100; iters: 100 0% -1% 0% -4% > | fibers: 100; iters: 1000 0% 1% 0% 0% > | fibers: 100; iters: 10000 0% 0% 0% -3% > | fibers: 100; iters: 100000 0% 1% 0% -2% > | fibers: 1000; iters: 100 0% 0% -1% -3% > | fibers: 1000; iters: 1000 0% 0% 0% 1% > | fibers: 1000; iters: 10000 0% 0% 0% 0% > | fibers: 1000; iters: 100000 0% 0% 0% -1% > | fibers: 10000; iters: 100 0% 0% 0% 2% > | fibers: 10000; iters: 1000 0% -1% 0% 2% > | fibers: 10000; iters: 10000 -1% -1% 0% 0% > | fibers: 10000; iters: 100000 -1% 0% -1% -3% > > And here is a final one. I personally don't like it (considering my > comments in the previous reply), but *for now* it can be a solution. 1. I couldn't find - why don't you like it? It seems to be the fastest solution, not affecting the microbench at all, and definitely not affecting any more complex scenarios. > ================================================================================ > > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c > index 483ae3ce1..ed6104c8d 100644 > --- a/src/lib/core/fiber.c > +++ b/src/lib/core/fiber.c > @@ -46,6 +46,8 @@ > #if ENABLE_FIBER_TOP > #include /* __rdtscp() */ > > +extern void lua_on_yield(void); > + > static inline void > clock_stat_add_delta(struct clock_stat *stat, uint64_t clock_delta) > { > @@ -416,6 +418,10 @@ fiber_call(struct fiber *callee) > /** By convention, these triggers must not throw. */ > if (! rlist_empty(&caller->on_yield)) > trigger_run(&caller->on_yield, NULL); > + > + if (cord_is_main()) > + lua_on_yield(); 2. Why not inside fiber_call_impl? I thought we need to call the abort on each coro_transfer(). > + > clock_set_on_csw(caller); > callee->caller = caller; > callee->flags |= FIBER_IS_READY; > @@ -645,6 +651,10 @@ fiber_yield(void) > /** By convention, these triggers must not throw. */ > if (! rlist_empty(&caller->on_yield)) > trigger_run(&caller->on_yield, NULL); > + > + if (cord_is_main()) > + lua_on_yield(); > + > clock_set_on_csw(caller); > > assert(callee->flags & FIBER_IS_READY || callee == &cord->sched); > diff --git a/src/lua/utils.c b/src/lua/utils.c > index af114b0a2..49e3c2bf0 100644 > --- a/src/lua/utils.c > +++ b/src/lua/utils.c > @@ -1308,3 +1308,9 @@ tarantool_lua_utils_init(struct lua_State *L) > luaT_newthread_ref = luaL_ref(L, LUA_REGISTRYINDEX); > return 0; > } > + > +#include "lj_trace.h" 3. Why is the header included here, and not in the beginning? 4. It is worth adding a comment. > +void lua_on_yield(void) > +{ > + lj_trace_abort(G(tarantool_L)); > +} > > ================================================================================ > > * Vanilla -> Patched [extern macro callback] (min, median, mean, max): > | fibers: 10; iters: 100 1% 1% 0% 0% > | fibers: 10; iters: 1000 0% 4% 0% -1% > | fibers: 10; iters: 10000 0% 5% 2% 6% > | fibers: 10; iters: 100000 0% 0% 0% 0% > | fibers: 100; iters: 100 0% -4% -3% -6% > | fibers: 100; iters: 1000 0% 3% 1% 0% > | fibers: 100; iters: 10000 0% 0% 0% -2% > | fibers: 100; iters: 100000 0% 1% 0% -2% > | fibers: 1000; iters: 100 0% 0% 0% -4% > | fibers: 1000; iters: 1000 0% 0% 0% -1% > | fibers: 1000; iters: 10000 0% 0% 0% 0% > | fibers: 1000; iters: 100000 0% 0% 0% -1% > | fibers: 10000; iters: 100 -1% 1% 1% 2% > | fibers: 10000; iters: 1000 -1% 0% 0% 2% > | fibers: 10000; iters: 10000 0% 0% 0% 0% > | fibers: 10000; iters: 100000 0% 0% 0% 0% > > There was also an alternative idea by Sergos: introduce a special > parameter to enable such feature by demand. 5. I am not sure it is so necessary - from your bench it looks the overhead is almost 0, not counting the rare noise about +-1%.