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

[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