<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="">Hi! Thanks for your review!<div class="">I pushed the changes on the branch, the diff is below.</div><div class=""><br class=""></div><div class=""><div class="">
<div dir="auto" style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; 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><br class="Apple-interchange-newline"><br class="Apple-interchange-newline">
</div>
<div><br class=""><blockquote type="cite" class=""><div class="">2 нояб. 2019 г., в 20:12, 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="">Hi! Thanks for the patch!<br class=""><br class="">See 3 comments below.<br class=""><br class=""><blockquote type="cite" class="">diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br class="">index 93f22ae68..52888cc64 100644<br class="">--- a/src/lib/core/fiber.c<br class="">+++ b/src/lib/core/fiber.c<br class="">@@ -82,6 +86,38 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);<br class="">+#if ENABLE_FIBER_TOP<br class="">+static __thread bool fiber_top_enabled = false;<br class="">+<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="">+ */<br class="">+static inline void<br class="">+clock_set_on_csw(struct fiber *caller)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>caller->csw++;<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>return;<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>uint64_t clock;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>uint32_t cpu_id;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>clock = __rdtscp(&cpu_id);<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if (cpu_id == cord()->cpu_id_last) {<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;<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;<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><span class="Apple-tab-span" style="white-space:pre"> </span>cord()->cpu_id_last = cpu_id;<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++;<br class="">+<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;<br class="">+}<br class="">+<br class="">+#else<br class="">+#define clock_set_on_csw(caller) ;<br class=""></blockquote><br class="">1. With undefined ENABLE_FIBER_TOP you don't update csw counter.<br class="">I would move this #if ENABLE to the clock_set_on_csw() body,<br class="">right after csw is incremented.<br class=""></div></div></blockquote><div><br class=""></div><div>Fixed.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+#endif /* ENABLE_FIBER_TOP */<br class="">+<br class="">diff --git a/src/lua/fiber.c b/src/lua/fiber.c<br class="">index 124908a05..a030e444d 100644<br class="">--- a/src/lua/fiber.c<br class="">+++ b/src/lua/fiber.c<br class="">@@ -319,6 +323,67 @@ lbox_fiber_statof_nobt(struct fiber *f, void *cb_ctx)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>return lbox_fiber_statof(f, cb_ctx, false);<br class=""> }<br class=""><br class="">+#if ENABLE_FIBER_TOP<br class="">+static int<br class="">+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="">+<span class="Apple-tab-span" style="white-space:pre"> </span>char name_buf[64];<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>snprintf(name_buf, sizeof(name_buf), "%u/%s", f->fid, f->name);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushstring(L, name_buf);<br class=""></blockquote><br class="">2. A piece of advice - use tt_sprintf:<br class=""><br class=""> lua_pushstring(L, tt_sprintf("%u/%s", f->fid, f->name));<br class=""></div></div></blockquote><div><br class=""></div><div>Thanks! Changed.</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>lua_newtable(L);<br class="">+<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, "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_pushliteral(L, "time");<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushnumber(L, f->cputime / (double) FIBER_TIME_RES);<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="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>return 0;<br class="">+}<br class="">diff --git a/test/app/fiber.result b/test/app/fiber.result<br class="">index 3c6115a33..3b9e5da9a 100644<br class="">--- a/test/app/fiber.result<br class="">+++ b/test/app/fiber.result<br class="">@@ -1462,6 +1462,91 @@ 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="">+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="">+-- and is indexed by name<br class="">+a["1/sched"] ~= 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="">+-- 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="">+for k, v in pairs(a) do\<br class="">+ if type(v) == 'table' then\<br class=""></blockquote><br class="">3. This looks hard to use. The fact, that one table contains<br class="">records totally different in their structure. I would propose<br class="">to return cpu misses and fibers separately:<br class=""><br class=""> <a href="http://fiber.top" class="">fiber.top</a>() =<br class=""><br class=""> cpu_misses = <number>,<br class=""> time = [<br class=""> '<id>/<name>' = {...},<br class=""> '<id>/<name>' = {...},<br class=""> '<id>/<name>' = {...},<br class=""> ...<br class=""> ]<br class=""><br class="">Then you can take <a href="http://fiber.top" class="">fiber.top</a>().time and be sure, that all<br class="">records here have the same structure. As far as I remember<br class="">we already had similar problems with other statistics, so<br class="">it is better to design it know in the most extendible way.<br class="">And it will be easier to add new global statistics to the<br class="">top in future.<br class=""><br class=""></div></div></blockquote><br class=""></div><div>Good point. I named the subtable `cpu` instead of `time`. It makes more</div><div>sense imo. Also renamed `cpu misses` to `cpu_misses` so that it can be</div><div>accessed as <a href="http://fiber.top" class="">fiber.top</a>().cpu_misses</div><div><br class=""></div><br class=""></div><span class="">diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br class="">index 52888cc64..aebaba7f0 100644<br class="">--- a/src/lib/core/fiber.c<br class="">+++ b/src/lib/core/fiber.c<br class="">@@ -88,6 +88,7 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);<br class=""> <br class=""> #if ENABLE_FIBER_TOP<br class=""> static __thread bool fiber_top_enabled = false;<br class="">+#endif /* ENABLE_FIBER_TOP */<br class=""> <br class=""> /**<br class=""> * An action performed each time a context switch happens.<br class="">@@ -97,6 +98,8 @@ static inline void<br class=""> clock_set_on_csw(struct fiber *caller)<br class=""> {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>caller->csw++;<br class="">+<br class="">+#if ENABLE_FIBER_TOP<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>return;<br class=""> <br class="">@@ -112,12 +115,10 @@ clock_set_on_csw(struct fiber *caller)<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>cord()->cpu_miss_count++;<br class=""> <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;<br class="">-}<br class="">-<br class="">-#else<br class="">-#define clock_set_on_csw(caller) ;<br class=""> #endif /* ENABLE_FIBER_TOP */<br class=""> <br class="">+}<br class="">+<br class=""> /*<br class=""> * Defines a handler to be executed on exit from cord's thread func,<br class=""> * accessible via cord()->on_exit (normally NULL). It is used to<br class="">diff --git a/src/lua/fiber.c b/src/lua/fiber.c<br class="">index a030e444d..8b3b22e55 100644<br class="">--- a/src/lua/fiber.c<br class="">+++ b/src/lua/fiber.c<br class="">@@ -33,6 +33,7 @@<br class=""> #include <fiber.h><br class=""> #include "lua/utils.h"<br class=""> #include "backtrace.h"<br class="">+#include "tt_static.h"<br class=""> <br class=""> #include <lua.h><br class=""> #include <lauxlib.h><br class="">@@ -328,10 +329,8 @@ static int<br class=""> 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="">-<span class="Apple-tab-span" style="white-space:pre"> </span>char name_buf[64];<br class=""> <br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>snprintf(name_buf, sizeof(name_buf), "%u/%s", f->fid, f->name);<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushstring(L, name_buf);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>lua_pushstring(L, tt_sprintf("%u/%s", f->fid, f->name));<br class=""> <br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>lua_newtable(L);<br class=""> <br class="">@@ -357,12 +356,15 @@ lbox_fiber_top(struct lua_State *L)<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="">-<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_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>lua_pushliteral(L, "cpu");<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>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="">+<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>return 1;<br class=""> }<br class="">diff --git a/test/app/fiber.result b/test/app/fiber.result<br class="">index 3b9e5da9a..4a094939f 100644<br class="">--- a/test/app/fiber.result<br class="">+++ b/test/app/fiber.result<br class="">@@ -1478,11 +1478,11 @@ type(a)<br class=""> ...<br class=""> -- scheduler is present in <a href="http://fiber.top" class="">fiber.top</a>()<br class=""> -- and is indexed by name<br class="">-a["1/sched"] ~= nil<br class="">+a.cpu["1/sched"] ~= nil<br class=""> ---<br class=""> - true<br class=""> ...<br class="">-type(a["cpu misses"]) == 'number'<br class="">+type(a.cpu_misses) == 'number'<br class=""> ---<br class=""> - true<br class=""> ...<br class="">@@ -1495,14 +1495,12 @@ sum_avg = 0<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="">+a = <a href="http://fiber.top" class="">fiber.top</a>().cpu<br class=""> ---<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["instant"]\<br class="">- sum_avg = sum_avg + v["average"]\<br class="">- end\<br class="">+ sum_inst = sum_inst + v["instant"]\<br class="">+ sum_avg = sum_avg + v["average"]\<br class=""> end<br class=""> ---<br class=""> ...<br class="">@@ -1521,7 +1519,7 @@ tbl = nil<br class=""> f = fiber.new(function()\<br class=""> for i = 1,1000 do end\<br class=""> fiber.yield()\<br class="">- tbl = <a href="http://fiber.top" class="">fiber.top</a>()[fiber.self().id()..'/'..fiber.self().name()]\<br class="">+ tbl = <a href="http://fiber.top" class="">fiber.top</a>().cpu[fiber.self().id()..'/'..fiber.self().name()]\<br class=""> end)<br class=""> ---<br class=""> ...<br class="">diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua<br class="">index ce1f55e8d..38b85d554 100644<br class="">--- a/test/app/fiber.test.lua<br class="">+++ b/test/app/fiber.test.lua<br class="">@@ -638,20 +638,18 @@ 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=""> -- and is indexed by name<br class="">-a["1/sched"] ~= nil<br class="">-type(a["cpu misses"]) == 'number'<br class="">+a.cpu["1/sched"] ~= nil<br class="">+type(a.cpu_misses) == 'number'<br class=""> sum_inst = 0<br class=""> sum_avg = 0<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="">+a = <a href="http://fiber.top" class="">fiber.top</a>().cpu<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="">+ sum_inst = sum_inst + v["instant"]\<br class="">+ sum_avg = sum_avg + v["average"]\<br class=""> end<br class=""> <br class=""> sum_inst<br class="">@@ -661,7 +659,7 @@ tbl = nil<br class=""> f = fiber.new(function()\<br class=""> for i = 1,1000 do end\<br class=""> fiber.yield()\<br class="">- tbl = <a href="http://fiber.top" class="">fiber.top</a>()[fiber.self().id()..'/'..fiber.self().name()]\<br class="">+ tbl = <a href="http://fiber.top" class="">fiber.top</a>().cpu[fiber.self().id()..'/'..fiber.self().name()]\<br class=""> end)<br class=""> while f:status() ~= 'dead' do fiber.sleep(0.01) end<br class=""> tbl["average"] > 0<br class=""></span><span class=""><br class=""></span></body></html>