Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] lua: add fiber.top() listing fiber cpu consumption

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

I think it is worth mentioning in the doc request,
that fiber.top may affect performance when enabled, so
users should not enable it when possible.

> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index e3fe77e21..4c646b278 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -87,31 +87,32 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
>  })
>  
>  #if ENABLE_FIBER_TOP
> +static bool fiber_top_enabled = false;

I just realized that we might want to make it
thread local (__thread). We have fibers in other
threads, and I don't think we want to turn on fiber
top for them accidentally.

On the other hand sometimes access to a thread local
variable might be an expensive thing in my experience.
So probably we might want not to care about other
threads starting to collect top together with the main.
Just so as to keep performance unaffected when top is
disabled.

I would bench thread local fiber_top_enabled vs
global fiber_top_enabled, both with disabled top.
If they are the same, then lets make it thread local.

Note, this has nothing to do with fiber_top_init(),
which you call for the main thread only. Check for
fiber_top_enabled happens regardless of whether the
top was initialized, in all threads.

Other than that the patch LGTM. I think you need to
get a second review from Georgy or Alexander Tu.