* Re: [Tarantool-patches] [PATCH luajit] Save trace recorder state around VM event call.\
[not found] ` <Y1k5ON/YOyhYz/Zx@root>
[not found] ` <1667026151.120548758@f533.i.mail.ru>
@ 2022-12-22 11:40 ` sergos via Tarantool-patches
1 sibling, 0 replies; 2+ messages in thread
From: sergos via Tarantool-patches @ 2022-12-22 11:40 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 2+ messages in thread