Re: [Tarantool-patches] [PATCH] fiber: abort trace recording on fiber yield

["Sergey Ostanevich" <sergos@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2950 bytes --]


Hi, Igor!

Thanks for detailed report, the results are LGTM.
As for the measured speedup - I tend to ignore it, since the min-max spread is way bigger.

Regards,
Sergos 
Monday, 21 September 2020, 22:33 +0300 from Igor Munkin  <imun@tarantool.org>:
>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 < imun@tarantool.org > wrote:
>> > 
>> > Vlad,
>> > 
>> > Thanks for your review!
>> > 
>> > On 01.04.20, Vladislav Shpilevoy wrote:
>> >> Hi! Thanks for the patch!
>> >> 
>> >> See 7 comments below.
>> >> 
>
><snipped>
>
>> > 
>> >> 
>> >> 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 <lj_trace_abort>. I dropped several comments regarding
>the rationale for the fix in v2 version.
>
>> 
>
><snipped>
>
>> > 
>> > 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
>> 
>
><snipped>
>
>> 
>
>[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

[-- Attachment #2: Type: text/html, Size: 8831 bytes --]