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

[Serge Petrenko <sergepetrenko@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

Thanks for reviewing this!

> 29 окт. 2019 г., в 2:00, 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.

I added the warning to the doc request.

> 
>> 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.
> 

I’ve tested your proposal. The difference is ±2%.
I can post the results if you want me to.
So, let it be `static __thread bool fiber_top_enabled`.

> 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.
> 

I’ve resent v3 with the fixes mentioned above and one additional feature
requested by Mons (added «time» entry listing cpu time consumed by each
fiber both to fiber.info <http://fiber.info/>() and fiber.top <http://fiber.top/>())

--
Serge Petrenko
sergepetrenko@tarantool.org




[-- Attachment #2: Type: text/html, Size: 3997 bytes --]