<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">Hi! Thank you for review!</div><div class=""><div class="">I addressed your comments and answered inline.</div><div class="">Incremental diff is below.</div></div><br class=""><div class=""><div><blockquote type="cite" class=""><div class="">23 окт. 2019 г., в 0:18, Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org" class="">v.shpilevoy@tarantool.org</a>> написал(а):</div><br class="Apple-interchange-newline"><div class=""><div class="">Thanks for the patch!<br class=""><br class="">Sorry, the version in the email is a bit outdated. I<br class="">pasted below the actual version from the branch.<br class=""><br class=""></div></div></blockquote><blockquote type="cite" class=""><div class=""><div class="">See 9 comments below.<br class=""><br class=""></div></div></blockquote><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class=""> lua: add <a href="http://fiber.top" class="">fiber.top</a>() listing fiber cpu consumption<br class=""><br class=""> Implement a new function in Lua fiber library: top(). It returns a table<br class=""> of alive fibers (including the scheduler). Each table entry has two<br class=""> fields: average cpu consumption, which is calculated with exponential<br class=""> moving average over event loop iterations, and current cpu consumption,<br class=""> which shows fiber's cpu usage over the last event loop iteration.<br class=""> The patch relies on CPU timestamp counter to measure each fiber's time<br class=""> share.<br class=""><br class=""> Closes #2694<br class=""><br class=""> @TarantoolBot document<br class=""> Title: fiber: new function `<a href="http://fiber.top" class="">fiber.top</a>()`<br class=""><br class=""> `<a href="http://fiber.top" class="">fiber.top</a>()` returns a table of all alive fibers and lists their cpu<br class=""> consumption. Let's take a look at the example:<br class=""> ```<br class=""> tarantool> <a href="http://fiber.top" class="">fiber.top</a>()<br class=""> ---<br class=""> - 1:<br class=""> cpu average (%): 10.779696493982<br class=""> cpu instant (%): 10.435256168573<br class=""></blockquote><br class="">1. Have you considered making these option names<br class="">one word? 'cpu average (%)' -> 'average'. The<br class="">same for instant. My motivation is that it could<br class="">simplify wrappers which are going to use the top<br class="">to show it in a GUI or something. You actually use<br class="">it in the tests.<br class=""><br class="">It would not hard readability IMO. It is obvious<br class="">that top is in percents, and about CPU time.<br class=""></div></div></blockquote><div><br class=""></div>No problem.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class=""> 115:<br class=""> cpu average (%): 5.4571279061075<br class=""> cpu instant (%): 5.9653973440576<br class=""> 120:<br class=""> cpu average (%): 21.944382148464<br class=""> cpu instant (%): 23.849021825646<br class=""> 116:<br class=""> cpu average (%): 8.6603872318158<br class=""> cpu instant (%): 9.6812031335093<br class=""> 119:<br class=""> cpu average (%): 21.933168871944<br class=""> cpu instant (%): 20.007540530351<br class=""> cpu misses: 0<br class=""> 118:<br class=""> cpu average (%): 19.342901995963<br class=""> cpu instant (%): 16.932679820703<br class=""> 117:<br class=""> cpu average (%): 11.549674814981<br class=""> cpu instant (%): 13.128901177161<br class=""> ...<br class=""> ```<br class=""></blockquote><br class="">2. This is super cool!<br class=""><br class="">Could we give names to predefined fibers? For<br class="">example, make an alias top.scheduler = top[1].<br class=""><br class="">So as in the console output I would see:<br class=""><br class=""> ---<br class=""> - scheduler:<br class=""> cpu average (%): 98.230148883023<br class=""> cpu instant (%): 100<br class=""><br class="">Instead of '1'. Because personally I didn't<br class="">even know that 1 is always the scheduler, and<br class="">I would be confused to see a regular fiber<br class="">consumed 99% of time in a console.<br class=""></div></div></blockquote><div><br class=""></div><div>Hmm, what about this?</div><div><br class=""></div><span class="">--- a/src/lua/fiber.c<br class="">+++ b/src/lua/fiber.c<br class="">@@ -325,13 +325,18 @@ lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)<br class=""> {<br class=""> struct lua_State *L = (struct lua_State *) cb_ctx;<br class=""> <br class="">- lua_pushinteger(L, f->fid);<br class="">+ /* Index system fibers by name instead of id */<br class="">+ if (f->fid <= FIBER_ID_MAX_RESERVED) {<br class="">+ lua_pushstring(L, f->name);<br class="">+ } else {<br class="">+ lua_pushinteger(L, f->fid);<br class="">+ }<br class=""> lua_newtable(L);<br class=""></span><span class=""><br class=""></span><span class=""><br class=""></span><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class=""> In the table above keys are fiber id's (and a single 'cpu misses' key<br class=""> which indicates the amount of times tx thread was rescheduled on a<br class=""> different cpu core. More on that later).<br class=""> The two metrics available for each fiber are:<br class=""> 1) cpu instant (per cent),<br class=""> which indicates the share of time fiber was executing during the<br class=""> previous event loop iteration<br class=""> 2) cpu average (per cent), which is calculated as an exponential moving<br class=""> average of `cpu instant` values over all previous event loop iterations.<br class=""><br class=""> More info on `cpu misses` field returned by `<a href="http://fiber.top" class="">fiber.top</a>()`:<br class=""> `cpu misses` indicates the amount of times tx thread detected it was<br class=""> rescheduled on a different cpu core during the last event loop<br class=""> iteration.<br class=""> <a href="http://fiber.top" class="">fiber.top</a>() uses cpu timestamp counter to measure each fiber's execution<br class=""> time. However, each cpu core may have its own counter value (you can<br class=""> only rely on counter deltas if both measurements were taken on the same<br class=""> core, otherwise the delta may even get negative).<br class=""> When tx thread is rescheduled to a different cpu core, tarantool just<br class=""> assumes cpu delta was zero for the lust measurement. This loweres<br class=""> precision of our computations, so the bigger `cpu misses` value the<br class=""> lower the precision of <a href="http://fiber.top" class="">fiber.top</a>() results.<br class=""><br class=""> <a href="http://Fiber.top" class="">Fiber.top</a>() doesn't work on arm architecture at the moment.<br class=""></blockquote><br class="">2. Have you done benchmarks how much top slows down fiber<br class="">switching? It should not be hard to measure. Just switching.<br class="">Lots of fibers which only yield. And measure loop iterations<br class="">per second. Won't work? Or average/median duration of one<br class="">loop iteration.<br class=""></div></div></blockquote><div><br class=""></div><div>Hi! I tested context switches per second.</div><div>The results are in the issue comments:</div><span class=""><a href="https://github.com/tarantool/tarantool/issues/2694#issuecomment-546381304" class="">https://github.com/tarantool/tarantool/issues/2694#issuecomment-546381304</a></span></div><div><span class="">Performance degradation is between 10 and 16 per cent.<br class=""></span><span class=""><br class=""></span><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class=""><br class="">diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br class="">index b813c1739..bea49eb41 100644<br class="">--- a/src/lib/core/fiber.c<br class="">+++ b/src/lib/core/fiber.c<br class="">@@ -37,11 +37,14 @@<br class=""> #include <stdlib.h><br class=""> #include <string.h><br class=""> #include <pmatomic.h><br class="">-<br class=""> #include "assoc.h"<br class=""></blockquote><br class="">3. Sorry, looks like stray diff. Could you<br class="">please drop it?<br class=""><br class=""></div></div></blockquote><div><br class=""></div><div>Thanks for noticing! Fixed.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class=""> #include "memory.h"<br class=""> #include "trigger.h"<br class="">@@ -82,6 +85,34 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>err;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class=""> })<br class=""><br class="">+#if ENABLE_FIBER_TOP<br class="">+/**<br class="">+ * An action performed each time a context switch happens.<br class="">+ * Used to count each fiber's processing time.<br class="">+ * This is a macro rather than a function, since it is used<br class="">+ * in scheduler too.<br class=""></blockquote><br class="">4. Hm. Not sure if I understand. Ok, it is used in the scheduler,<br class="">so why does it prevent making it a function? Caller is always<br class="">struct fiber *, so you can make it a function taking fiber *<br class="">parameter, can't you?<br class=""></div></div></blockquote><div><br class=""></div><div>You cannot call functions from scheduler cos it doesn’t have a</div><div>stack AFAIU. I can make it an inline function if you want me to.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+ */<br class="">+#define clock_set_on_csw(caller)<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+({<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>uint64_t clock;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>uint32_t cpu_id;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>clock = __rdtscp(&cpu_id);<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (cpu_id == cord()->cpu_id_last) {<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>(caller)->clock_delta += clock - cord()->clock_last;<span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->clock_delta += clock - cord()->clock_last;<span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>} else {<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->cpu_id_last = cpu_id;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->cpu_miss_count++;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>}<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>cord()->clock_last = clock;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+})> @@ -584,6 +617,7 @@ fiber_schedule_list(struct rlist *list)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>last->caller = fiber();<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>assert(fiber() == &cord()->sched);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>clock_set_on_csw(fiber());<br class=""></blockquote><br class="">5. Am I right, that we need to update CPU consumption each<br class="">time a context switch happens? </div></div></blockquote><div><br class=""></div><div>Yep.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">If so, then by logic you<br class="">would need to call clock_set_on_csw() on each fiber->csw++,<br class="">correct?<br class=""></div></div></blockquote><div><br class=""></div>Right. I also call clock_set_on_csw() on each event loop iteration</div><div>end.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Currently that happens in 2 places: fiber_call_impl() and<br class="">fiber_yield(). In both these places you already call<br class="">clock_set_on_csw():<br class=""><br class=""> - You always call clock_set_on_csw() right before<br class=""> fiber_call_impl();<br class=""><br class=""> - You call clock_set_on_csw() in fiber_yield().<br class=""><br class="">I would then move fiber->cws++ logic into your macros too.<br class="">So all the context switch and CPU time tracking logic<br class="">would be in one place.<br class=""><br class="">The only problem is that your macros takes caller, but csw++<br class="">is done for callee. Probably we could increment it for the<br class="">caller instead, I don't anything depends on that.<br class=""><br class="">Also it would lead to increment of the scheduler's csw<br class="">counter. Not sure whether it is ok.<br class=""></div></div></blockquote><div><br class=""></div><div>Loop iteration end isn’t a context switch, but I still update</div><div>clocks here. I don’t want to increment csw here as a side</div><div>effect, so maybe leave csw increment as it is?</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Like that (I didn't test it):<br class=""><br class="">============================================================<br class=""><br class="">diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br class="">index bea49eb41..e9aac99d0 100644<br class="">--- a/src/lib/core/fiber.c<br class="">+++ b/src/lib/core/fiber.c<br class="">@@ -105,6 +105,7 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->cpu_id_last = cpu_id;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->cpu_miss_count++;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>caller->csw++;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cord()->clock_last = clock;<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>\<br class=""> })<br class=""><br class="">@@ -258,7 +259,7 @@ fiber_call_impl(struct fiber *callee)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cord->fiber = callee;<br class=""><br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>callee->flags &= ~FIBER_IS_READY;<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>callee->csw++;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>clock_set_on_csw(caller);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>ASAN_START_SWITCH_FIBER(asan_state, 1,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>callee->stack,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>callee->stack_size);<br class="">@@ -277,7 +278,6 @@ fiber_call(struct fiber *callee)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>/** By convention, these triggers must not throw. */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>if (! rlist_empty(&caller->on_yield))<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>trigger_run(&caller->on_yield, NULL);<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>clock_set_on_csw(caller);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>callee->caller = caller;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>callee->flags |= FIBER_IS_READY;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>caller->flags |= FIBER_IS_READY;<br class="">@@ -511,7 +511,6 @@ fiber_yield(void)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>assert(callee->flags & FIBER_IS_READY || callee == &cord->sched);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>assert(! (callee->flags & FIBER_IS_DEAD));<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cord->fiber = callee;<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>callee->csw++;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>callee->flags &= ~FIBER_IS_READY;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>ASAN_START_SWITCH_FIBER(asan_state,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>(caller->flags & FIBER_IS_DEAD) == 0,<br class="">@@ -617,7 +616,6 @@ fiber_schedule_list(struct rlist *list)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>last->caller = fiber();<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>assert(fiber() == &cord()->sched);<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>clock_set_on_csw(fiber());<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>fiber_call_impl(first);<br class=""> }<br class=""><br class="">============================================================<br class=""><br class=""><blockquote type="cite" class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>fiber_call_impl(first);<br class=""> }<br class=""><br class="">@@ -1044,6 +1082,107 @@ fiber_destroy_all(struct cord *cord)> +bool<br class="">+fiber_top_is_enabled()<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>return fiber_top_enabled;<br class="">+}<br class="">+<br class="">+inline void<br class="">+fiber_top_enable()<br class=""></blockquote><br class="">6. Please, add 'static' modifier. Looks like<br class="">this function is used inside this file only.<br class="">The same for fiber_top_disable().<br class=""></div></div></blockquote><div><br class=""></div>It’s used in lua fiber module.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (!fiber_top_enabled) {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>ev_prepare_start(cord()->loop, &cord()->prepare_event);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>ev_check_start(cord()->loop, &cord()->check_event);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>fiber_top_enabled = true;<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->clock_acc = 0;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->cpu_miss_count_last = 0;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->clock_delta_last = 0;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+}<br class="">@@ -1077,6 +1216,14 @@ cord_create(struct cord *cord, const char *name)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>ev_async_init(&cord->wakeup_event, fiber_schedule_wakeup);<br class=""><br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>ev_idle_init(&cord->idle_event, fiber_schedule_idle);<br class="">+<br class="">+#if ENABLE_FIBER_TOP<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>/* <a href="http://fiber.top" class="">fiber.top</a>() currently works only for the main thread. */<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (cord_is_main()) {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>fiber_top_init();<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>fiber_top_enable();<br class=""></blockquote><br class="">7. I think, we need to enable top by default only<br class="">when we are sure that it does not affect performance.<br class=""></div></div></blockquote><div><br class=""></div>Ok, disabled it by default. User can turn it on with</div><div>fiber.top_enable(). This also resolved the issue I had</div><div>with your swim test.</div><div><br class=""></div><div>Looking at perf difference, maybe we can turn it on by default?</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+#endif /* ENABLE_FIBER_TOP */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cord_set_name(name);<br class=""><br class=""> #if ENABLE_ASAN<br class="">diff --git a/src/lua/fiber.c b/src/lua/fiber.c<br class="">index 336be60a2..2adff1536 100644<br class="">--- a/src/lua/fiber.c<br class="">+++ b/src/lua/fiber.c<br class="">@@ -319,6 +319,61 @@ lbox_fiber_statof_nobt(struct fiber *f, void *cb_ctx)<br class="">+<br class="">+static int<br class="">+lbox_fiber_top(struct lua_State *L)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (!fiber_top_is_enabled()) {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>luaL_error(L, "<a href="http://fiber.top" class="">fiber.top</a>() is disabled. enable it with"<br class=""></blockquote><br class="">8. 'enable' is a first word in the sentence. So probably<br class="">it is 'Enable’.<br class=""></div></div></blockquote><div><br class=""></div><div>Yep. Done.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span> " fiber.top_enable() first");<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_newtable(L);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushliteral(L, "cpu misses");<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushnumber(L, cord()->cpu_miss_count_last);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_settable(L, -3);<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lbox_fiber_top_entry(&cord()->sched, L);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>fiber_stat(lbox_fiber_top_entry, L);<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>return 1;<br class="">+}<br class="">diff --git a/test/app/fiber.result b/test/app/fiber.result<br class="">index 3c6115a33..196fae3b7 100644<br class="">--- a/test/app/fiber.result<br class="">+++ b/test/app/fiber.result<br class="">@@ -1462,6 +1462,84 @@ fiber.join(fiber.self())<br class=""> ---<br class=""> - error: the fiber is not joinable<br class=""> ...<br class="">+sum = 0<br class="">+---<br class="">+...<br class="">+-- gh-2694 <a href="http://fiber.top" class="">fiber.top</a>()<br class="">+a = <a href="http://fiber.top" class="">fiber.top</a>()<br class="">+---<br class="">+...<br class="">+-- scheduler is present in <a href="http://fiber.top" class="">fiber.top</a>()<br class="">+a[1] ~= nil<br class="">+---<br class="">+- true<br class="">+...<br class="">+type(a["cpu misses"]) == 'number'<br class="">+---<br class="">+- true<br class="">+...<br class="">+sum_inst = 0<br class="">+---<br class="">+...<br class="">+sum_avg = 0<br class="">+---<br class="">+...<br class="">+test_run:cmd('setopt delimiter ";"')<br class=""></blockquote><br class="">9. I would consider usage of '\' instead of a<br class="">delimiter change for such a short piece of<br class="">multiline code. But up to you. Just in case<br class="">you didn't know about '\', because I didn't<br class="">know either until recently.<br class=""><br class=""></div></div></blockquote></div><div class=""><br class=""></div><div class="">I didn’t know about ‘\’ as well. Thanks.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Here’s the incremental diff:</div><div class=""><br class=""></div><span class="">diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br class="">index bea49eb41..883fbac36 100644<br class="">--- a/src/lib/core/fiber.c<br class="">+++ b/src/lib/core/fiber.c<br class="">@@ -37,6 +37,7 @@<br class=""> #include <stdlib.h><br class=""> #include <string.h><br class=""> #include <pmatomic.h><br class="">+<br class=""> #include "assoc.h"<br class=""> #include "memory.h"<br class=""> #include "trigger.h"<br class="">@@ -1221,7 +1222,6 @@ cord_create(struct cord *cord, const char *name)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>/* <a href="http://fiber.top" class="">fiber.top</a>() currently works only for the main thread. */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>if (cord_is_main()) {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>fiber_top_init();<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>fiber_top_enable();<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> #endif /* ENABLE_FIBER_TOP */<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cord_set_name(name);<br class="">diff --git a/src/lua/fiber.c b/src/lua/fiber.c<br class="">index 62f3ccce4..c38bd886f 100644<br class="">--- a/src/lua/fiber.c<br class="">+++ b/src/lua/fiber.c<br class="">@@ -325,13 +325,18 @@ lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)<br class=""> {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>struct lua_State *L = (struct lua_State *) cb_ctx;<br class=""> <br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushinteger(L, f->fid);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>/* Index system fibers by name instead of id */<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (f->fid <= FIBER_ID_MAX_RESERVED) {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushstring(L, f->name);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>} else {<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushinteger(L, f->fid);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_newtable(L);<br class=""> <br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushliteral(L, "cpu average (%)");<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushliteral(L, "average");<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushnumber(L, f->clock_acc / (double)cord()->clock_acc * 100);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_settable(L, -3);<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushliteral(L, "cpu instant (%)");<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushliteral(L, "instant");<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushnumber(L, f->clock_delta_last / (double)cord()->clock_delta_last * 100);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_settable(L, -3);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_settable(L, -3);<br class="">@@ -343,7 +348,7 @@ static int<br class=""> lbox_fiber_top(struct lua_State *L)<br class=""> {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>if (!fiber_top_is_enabled()) {<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>luaL_error(L, "<a href="http://fiber.top" class="">fiber.top</a>() is disabled. enable it with"<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>luaL_error(L, "<a href="http://fiber.top" class="">fiber.top</a>() is disabled. Enable it with"<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span> " fiber.top_enable() first");<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>}<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_newtable(L);<br class="">diff --git a/test/app/fiber.result b/test/app/fiber.result<br class="">index 196fae3b7..e45cd7639 100644<br class="">--- a/test/app/fiber.result<br class="">+++ b/test/app/fiber.result<br class="">@@ -1466,11 +1466,19 @@ sum = 0<br class=""> ---<br class=""> ...<br class=""> -- gh-2694 <a href="http://fiber.top" class="">fiber.top</a>()<br class="">+fiber.top_enable()<br class="">+---<br class="">+...<br class=""> a = <a href="http://fiber.top" class="">fiber.top</a>()<br class=""> ---<br class=""> ...<br class="">+type(a)<br class="">+---<br class="">+- table<br class="">+...<br class=""> -- scheduler is present in <a href="http://fiber.top" class="">fiber.top</a>()<br class="">-a[1] ~= nil<br class="">+-- and is indexed by name<br class="">+a["sched"] ~= nil<br class=""> ---<br class=""> - true<br class=""> ...<br class="">@@ -1484,21 +1492,19 @@ sum_inst = 0<br class=""> sum_avg = 0<br class=""> ---<br class=""> ...<br class="">-test_run:cmd('setopt delimiter ";"')<br class="">----<br class="">-- true<br class="">-...<br class="">-for k, v in pairs(a) do<br class="">- if type(v) == 'table' then<br class="">- sum_inst = sum_inst + v["cpu instant (%)"]<br class="">- sum_avg = sum_avg + v["cpu average (%)"]<br class="">- end<br class="">-end;<br class="">+-- update table to make sure<br class="">+-- a full event loop iteration<br class="">+-- has ended<br class="">+a = <a href="http://fiber.top" class="">fiber.top</a>()<br class=""> ---<br class=""> ...<br class="">-test_run:cmd('setopt delimiter ""');<br class="">+for k, v in pairs(a) do\<br class="">+ if type(v) == 'table' then\<br class="">+ sum_inst = sum_inst + v["instant"]\<br class="">+ sum_avg = sum_avg + v["average"]\<br class="">+ end\<br class="">+end<br class=""> ---<br class="">-- true<br class=""> ...<br class=""> sum_inst<br class=""> ---<br class="">@@ -1518,11 +1524,11 @@ f = fiber.new(function() for i = 1,1000 do end fiber.yield() tbl = <a href="http://fiber.top" class="">fiber.top</a>()[f<br class=""> while f:status() ~= 'dead' do fiber.sleep(0.01) end<br class=""> ---<br class=""> ...<br class="">-tbl["cpu average (%)"] > 0<br class="">+tbl["average"] > 0<br class=""> ---<br class=""> - true<br class=""> ...<br class="">-tbl["cpu instant (%)"] > 0<br class="">+tbl["instant"] > 0<br class=""> ---<br class=""> - true<br class=""> ...<br class="">@@ -1531,14 +1537,7 @@ fiber.top_disable()<br class=""> ...<br class=""> <a href="http://fiber.top" class="">fiber.top</a>()<br class=""> ---<br class="">-- error: <a href="http://fiber.top" class="">fiber.top</a>() is disabled. enable it with fiber.top_enable() first<br class="">-...<br class="">-fiber.top_enable()<br class="">----<br class="">-...<br class="">-type(<a href="http://fiber.top" class="">fiber.top</a>())<br class="">----<br class="">-- table<br class="">+- error: <a href="http://fiber.top" class="">fiber.top</a>() is disabled. Enable it with fiber.top_enable() first<br class=""> ...<br class=""> -- cleanup<br class=""> test_run:cmd("clear filter")<br class="">diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua<br class="">index 15507c2c7..e30aba77e 100644<br class="">--- a/test/app/fiber.test.lua<br class="">+++ b/test/app/fiber.test.lua<br class="">@@ -632,33 +632,39 @@ fiber.join(fiber.self())<br class=""> sum = 0<br class=""> <br class=""> -- gh-2694 <a href="http://fiber.top" class="">fiber.top</a>()<br class="">+fiber.top_enable()<br class="">+<br class=""> a = <a href="http://fiber.top" class="">fiber.top</a>()<br class="">+type(a)<br class=""> -- scheduler is present in <a href="http://fiber.top" class="">fiber.top</a>()<br class="">-a[1] ~= nil<br class="">+-- and is indexed by name<br class="">+a["sched"] ~= nil<br class=""> type(a["cpu misses"]) == 'number'<br class=""> sum_inst = 0<br class=""> sum_avg = 0<br class="">-test_run:cmd('setopt delimiter ";"')<br class="">-for k, v in pairs(a) do<br class="">- if type(v) == 'table' then<br class="">- sum_inst = sum_inst + v["cpu instant (%)"]<br class="">- sum_avg = sum_avg + v["cpu average (%)"]<br class="">- end<br class="">-end;<br class="">-test_run:cmd('setopt delimiter ""');<br class="">+<br class="">+-- update table to make sure<br class="">+-- a full event loop iteration<br class="">+-- has ended<br class="">+a = <a href="http://fiber.top" class="">fiber.top</a>()<br class="">+for k, v in pairs(a) do\<br class="">+ if type(v) == 'table' then\<br class="">+ sum_inst = sum_inst + v["instant"]\<br class="">+ sum_avg = sum_avg + v["average"]\<br class="">+ end\<br class="">+end<br class="">+<br class=""> sum_inst<br class=""> -- not exact due to accumulated integer division errors<br class=""> sum_avg > 99 and sum_avg < 101 or sum_avg<br class=""> tbl = nil<br class=""> f = fiber.new(function() for i = 1,1000 do end fiber.yield() tbl = <a href="http://fiber.top" class="">fiber.top</a>()[fiber.self().id()] end)<br class=""> while f:status() ~= 'dead' do fiber.sleep(0.01) end<br class="">-tbl["cpu average (%)"] > 0<br class="">-tbl["cpu instant (%)"] > 0<br class="">+tbl["average"] > 0<br class="">+tbl["instant"] > 0<br class=""> <br class=""> fiber.top_disable()<br class=""> <a href="http://fiber.top" class="">fiber.top</a>()<br class="">-fiber.top_enable()<br class="">-type(<a href="http://fiber.top" class="">fiber.top</a>())<br class=""> <br class=""> -- cleanup<br class=""> test_run:cmd("clear filter")</span><span class=""><br class=""></span><span class=""><br class=""></span><div class=""><div dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div>--</div><div>Serge Petrenko</div><div><a href="mailto:sergepetrenko@tarantool.org" class="">sergepetrenko@tarantool.org</a></div><div class=""><br class=""></div></div></div></div></body></html>