Re: [Tarantool-patches] [PATCH luajit] Save trace recorder state around VM event call.\

[sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

Hi!

Thanks for updates, LGTM.

> On 26 Oct 2022, at 16:42, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 25.10.22, sergos wrote:
>> Hi!
>> 
>> Thanks for the patch!
>> 
>> Some comments on the commit message and the test showed flaky behavior,
>> it failed only 2 times out of 10 runs using the 
>> Tarantool 2.11.0-entrypoint-637-gdd7d46af3 on a 
>> Darwin s-ostanevich2 22.1.0 Darwin Kernel Version 22.1.0
> 
> OMG, `tap` in tarantool is slightly different so the test is flaky.
> 
> Fixes with increasing amount of calls of `fibb()` function.
> 
> Also, removes `jit.bc.dump()` output.
> 
> See the iterative patch below.
> ===================================================================
> diff --git a/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua b/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua
> index b5146f70..475d9200 100644
> --- a/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua
> +++ b/test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua
> @@ -18,11 +18,16 @@ local function fibb(n)
>   return n < 2 and n or fibb(n - 1) + fibb(n - 2)
> end
> 
> +local function empty() end
> -- Compile `jit.bc` functions, that are used in vmevent handler.
> -require('jit.bc').dump(loadstring(string.dump(fibb)))
> +require('jit.bc').dump(loadstring(string.dump(fibb)), {
> +  write = empty,
> +  close = empty,
> +  flush = empty,
> +})
> 
> -- Here we dump (to /dev/null) info about `fibb()` traces and run
> -- `jit.bc` functions inside.
> -test:ok(fibb(0) == 0, 'run compiled function inside vmevent handler')
> +test:ok(fibb(2) == 1, 'run compiled function inside vmevent handler')
> 
> os.exit(test:check() and 0 or 1)
> ===================================================================
> 
>> 
>> Regards,
>> Sergos
>> 
>> 
>>> On 19 Oct 2022, at 15:37, Sergey Kaplun <skaplun@tarantool.org> wrote:
>>> 
>>> From: Mike Pall <mike>
>>> 
>>> Reported by Sergey Kaplun.
>>> 
>>> (cherry picked from commit b5b20191f3a8a2e2d28f1362b11bd26a51083d89)
>>> 
>>> `J->exitno` means the number of an exit from a currently executed trace
>> The          can have two meanings: an exit or a trace to stitch after a C call.
> 
> Fixed.
> 
>> 
>>> and also a number of a trace with which we want to stitch after C
>>> function execution. If part of a function has been compiled before a
>>                                   ^^^^^^^ is it about a C one? 
> 
> No, it's about Lua function inside vmevent handler (in bc.dump() for
> example) that called on routine to be compiled later.
> 
>> Possibly its better to say: if an vmevent handler itself has a trace 
>> compiled then execution of this trace can change the `J->exitno` from the 
>> original trace we plan to stitch into an taken exit from the handler’s trace. 
> 
> Yes, It's really better:)! Thanks!
> 
>> 
>>> vmevent handler was called, its trace is used during the vmevent handler
>>> on recording. When recording trace to stitch `J->exitno` is set to the
>> 
>>> previously executed trace. After running trace in the vmevent handler
>>> and exit it by a snapshot `J->exitno` is set to the number of exit
>>> instead number of a trace to stitch.
>> 
>> 
>>> It's the moment when stitching is
>>> broken.
>>> 
>>> This patch saves all necessary fields (`J->exitno`, `J->parent`), which
>> And what about `J-parent`? Isn’t it a trace to stitch to?
> 
> Yes, it's a parent trace for the currently executed one.
> 
>> 
>>> may change during vmevent handler execution, before vmevent call and
>>> restores them after.
>>> 
>>> Sergey Kaplun:
>>> * added the description and the test for the problem
>>> 
>>> Resolves tarantool/tarantool#6782
>>> Part of tarantool/tarantool#7230
>>> ---
> 
> The updated commit message is the following:
> 
> | Save trace recorder state around VM event call.
> |
> | Reported by Sergey Kaplun.
> |
> | (cherry picked from commit b5b20191f3a8a2e2d28f1362b11bd26a51083d89)
> |
> | The `J->exitno` can have two meanings: an exit or a trace to stitch
> | after a C call.
> |
> | If an vmevent handler itself has a trace compiled, then an execution of
> | this trace can change the `J->exitno` from the original trace, we plan
> | to stitch, into an taken exit from the handler’s trace. It's the moment
> | when stitching is broken.
> |
> | This patch saves all necessary fields (`J->exitno`, `J->parent`), which
> | may change during vmevent handler execution, before vmevent call and
> | restores them after.
> |
> | Sergey Kaplun:
> | * added the description and the test for the problem
> |
> | Resolves tarantool/tarantool#6782
> | Part of tarantool/tarantool#7230
> 
> Branch is force pushed, PR is updated.
> 
>>> 
>>> Issues:
>>> * https://github.com/tarantool/tarantool/issues/7230
>>> * https://github.com/tarantool/tarantool/issues/6782
>>> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6782-fix-vmevent-handler
>>> PR: https://github.com/tarantool/tarantool/pull/7826
>>> PR contains huge change, because tarantool/luajit branch hasn't been
>>> bumped yet in Tarantool's repo.
>>> Also, lto checks on lua-Harness/303-package.t are red for LTO build,
>>> this is related to CI/Build changes that were merged a few days ago;
>>> Tested on the previous commit to my commit.
>>> 
>>> src/lj_trace.c                                |  6 +++-
>>> ...6782-stitching-in-vmevent-handler.test.lua | 28 +++++++++++++++++++
>>> 2 files changed, 33 insertions(+), 1 deletion(-)
>>> create mode 100644 test/tarantool-tests/gh-6782-stitching-in-vmevent-handler.test.lua
> 
> <snipped>
> 
>>> 2.34.1
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun


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