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