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 F053A469719 for ; Mon, 21 Sep 2020 22:33:54 +0300 (MSK) Date: Mon, 21 Sep 2020 22:23:21 +0300 From: Igor Munkin Message-ID: <20200921192321.GO18920@tarantool.org> References: <20200707222436.GG5559@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH] fiber: abort trace recording on fiber yield List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Sergos, Thanks for your review! Please, consider my comments below. On 10.07.20, sergos@tarantool.org wrote: > Hi! > > Thanks for the patch and investigation! > > > > On 8 Jul 2020, at 01:24, Igor Munkin wrote: > > > > Vlad, > > > > Thanks for your review! > > > > On 01.04.20, Vladislav Shpilevoy wrote: > >> Hi! Thanks for the patch! > >> > >> See 7 comments below. > >> > > > >> > >> Why can't we call lj_trace_abort() directly? > > > > It's the internal API. Its usage complicates a switch between various > > LuaJIT implementations (we faced several challenges when tried to build > > Tarantool with uJIT). There is a public API to be used here (though in a > > bit hacky way). > > This hacky way looks fragile, since luaJIT_setmode() may change its behaviour > in the future and cause some unpredictable result. We have to mention it > somewhere as a warninig for future LuaJIT updates from upstream. For example, > introduce a comment inside luaJIT_setmode() that will conflict with plain > patch. We discussed this in the nearby thread[1] with Vlad and finally came to the solution with . I dropped several comments regarding the rationale for the fix in v2 version. > > > > > Looks like this way is slower than the one implemented via triggers. > > But does it catch more cases, as Vlad supposed? Do you have an extra > test for it? I provided several benchmarks results in the nearby thread[2]. For the chosen solution (via internal macro) it has almost no performance degradation (omitting the noise). > > Also, I would like to see the impact on some ‘real’ test - such as box > insertion/select or so? I tried yours benchmark[3] and got the following numbers: * Vanilla (insert per second): | min (15 runs): 809387.28574453 | median (15 runs): 822854.30884267 | mean (15 runs): 821996.668288715 | max (15 runs): 837764.83604149 * Patched (insert per second): | min (15 runs): 816005.94236505 | median (15 runs): 829281.27443029 | mean (15 runs): 828522.48986598 | max (15 runs): 839318.90025576 Em... It looks like a performance improvement, doesn't it? It seems like a compiler side-effect (e.g. invalid traces blacklisting), but I didn't make a deep investigation for this. > > > Regards, > Sergos > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-September/019306.html [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-September/019521.html [3]: https://gist.github.com/sergos/feb397ed4d5a5f739ee501f768da31e6 -- Best regards, IM