<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="">Thanks for reviewing this!<div class=""><br class=""><div><blockquote type="cite" class=""><div class="">29 окт. 2019 г., в 2:00, 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 fixes!<br class=""><br class="">I think it is worth mentioning in the doc request,<br class="">that <a href="http://fiber.top" class="">fiber.top</a> may affect performance when enabled, so<br class="">users should not enable it when possible.<br class=""></div></div></blockquote><div><br class=""></div><div>I added the warning to the doc request.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br class="">index e3fe77e21..4c646b278 100644<br class="">--- a/src/lib/core/fiber.c<br class="">+++ b/src/lib/core/fiber.c<br class="">@@ -87,31 +87,32 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);<br class=""> })<br class=""><br class=""> #if ENABLE_FIBER_TOP<br class="">+static bool fiber_top_enabled = false;<br class=""></blockquote><br class="">I just realized that we might want to make it<br class="">thread local (__thread). We have fibers in other<br class="">threads, and I don't think we want to turn on fiber<br class="">top for them accidentally.<br class=""><br class="">On the other hand sometimes access to a thread local<br class="">variable might be an expensive thing in my experience.<br class="">So probably we might want not to care about other<br class="">threads starting to collect top together with the main.<br class="">Just so as to keep performance unaffected when top is<br class="">disabled.<br class=""><br class="">I would bench thread local fiber_top_enabled vs<br class="">global fiber_top_enabled, both with disabled top.<br class="">If they are the same, then lets make it thread local.<br class=""><br class=""></div></div></blockquote><div><br class=""></div><div>I’ve tested your proposal. The difference is ±2%.</div><div>I can post the results if you want me to.</div><div>So, let it be `static __thread bool fiber_top_enabled`.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">Note, this has nothing to do with fiber_top_init(),<br class="">which you call for the main thread only. Check for<br class="">fiber_top_enabled happens regardless of whether the<br class="">top was initialized, in all threads.<br class=""><br class="">Other than that the patch LGTM. I think you need to<br class="">get a second review from Georgy or Alexander Tu.<br class=""><br class=""></div></div></blockquote></div><br class=""><div class="">I’ve resent v3 with the fixes mentioned above and one additional feature</div><div class="">requested by Mons (added «time» entry listing cpu time consumed by each</div><div class="">fiber both to <a href="http://fiber.info" class="">fiber.info</a>() and <a href="http://fiber.top" class="">fiber.top</a>())</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></div></body></html>