From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 AC029430D56 for ; Tue, 29 Oct 2019 01:54:44 +0300 (MSK) References: <5b3c1fae-1c49-4b13-3a0e-0774c4add3fb@tarantool.org> <4d729ef4-6b21-fa22-b379-dd670ac6fcc5@tarantool.org> <0E4E89B7-CA95-48D2-A719-F19DF6541CD0@tarantool.org> From: Vladislav Shpilevoy Message-ID: <14d5809e-ee70-cb13-9bb7-3bcb7b356589@tarantool.org> Date: Tue, 29 Oct 2019 00:00:12 +0100 MIME-Version: 1.0 In-Reply-To: <0E4E89B7-CA95-48D2-A719-F19DF6541CD0@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH v2 2/2] lua: add fiber.top() listing fiber cpu consumption List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko Cc: tarantool-patches@freelists.org, tarantool-patches@dev.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.