Re: [Tarantool-patches] [PATCH] fiber: abort trace recording on fiber yield

[Vladislav Shpilevoy <v.shpilevoy@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 <x86intrin.h> /* __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%.