From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 4EA571D5FF5; Thu, 22 Dec 2022 14:40:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4EA571D5FF5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1671709256; bh=JgNq3ji+ILT7qGdtt2TSPwSTDkela4kuTFMscHSp03A=; h=Date:In-Reply-To:To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=gKTUuChjbWhFcXT3/nsp4CSRh3BkgkeITb4uDEQNd3MRJyK0u+X4iqhHhxxcWt8G5 DFhgaj2VvwMrFwx7qVsz3o6mBYDHTg9pXzqGzO8DSvQ3DRZ/V14Klm1LOjgzaN1XMM 3CIyK5Jv+uZr4tCHBcLb4vJz/2EruiLH5C7Fl4Z4= Received: from smtp35.i.mail.ru (smtp35.i.mail.ru [95.163.41.76]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 87C111416BF for ; Thu, 22 Dec 2022 14:40:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 87C111416BF Received: by smtp35.i.mail.ru with esmtpa (envelope-from ) id 1p8JwT-00Frpe-Jk; Thu, 22 Dec 2022 14:40:54 +0300 Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_6E132AF7-0041-4972-A31E-7F6C9EA9636C" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\)) Date: Thu, 22 Dec 2022 14:40:42 +0300 In-Reply-To: To: Sergey Kaplun References: <20221019123730.25039-1-skaplun@tarantool.org> X-Mailer: Apple Mail (2.3731.300.101.1.3) X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD90D1502B3BE350FE43E7BE418629BFF19C8530425EA330D9500894C459B0CD1B94E251EE5C2EA14B61BCD8413C9507EAC97809DED56869E80421CDA9B766B0189 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BF6702EC5472AA0FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B1BE0CF094621A628638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8837C80EA34864AA04447E7B348142391117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC974A882099E279BDA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520652FD71AFB96DC7D2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EEB28585415E75ADA9E0F2381F647739FAD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3D2A98E5A6551E3E576E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CB24F08513AFFAC7943847C11F186F3C59DAA53EE0834AAEE X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349A401E2B4D763A4AB88AAAF0BEEBD035994177397E249F6992AEFF914BB917B5D2EB1404FCBCC4D41D7E09C32AA3244C7F918941F51AA52F9C24EB52C613CE31259227199D06760AFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojbuD7sI+uhmOmzO6A5lTmFw== X-Mailru-Sender: 5AA3D5B9D8C486464BD4402E82A444E31A1E484186B7C12E56655B95A34D93E6DBE5843F163867CD19381EE24192DF5555834048F03EF5D4C9A814A92B2E3B1BA4250FC3964EA4964198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Save trace recorder state around VM event call.\ X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: sergos via Tarantool-patches Reply-To: sergos Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" --Apple-Mail=_6E132AF7-0041-4972-A31E-7F6C9EA9636C Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi! Thanks for updates, LGTM. > On 26 Oct 2022, at 16:42, Sergey Kaplun wrote: >=20 > Hi, Sergos! >=20 > Thanks for the review! >=20 > On 25.10.22, sergos wrote: >> Hi! >>=20 >> Thanks for the patch! >>=20 >> Some comments on the commit message and the test showed flaky = behavior, >> it failed only 2 times out of 10 runs using the=20 >> Tarantool 2.11.0-entrypoint-637-gdd7d46af3 on a=20 >> Darwin s-ostanevich2 22.1.0 Darwin Kernel Version 22.1.0 >=20 > OMG, `tap` in tarantool is slightly different so the test is flaky. >=20 > Fixes with increasing amount of calls of `fibb()` function. >=20 > Also, removes `jit.bc.dump()` output. >=20 > See the iterative patch below. > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 >=20 > +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 =3D empty, > + close =3D empty, > + flush =3D empty, > +}) >=20 > -- Here we dump (to /dev/null) info about `fibb()` traces and run > -- `jit.bc` functions inside. > -test:ok(fibb(0) =3D=3D 0, 'run compiled function inside vmevent = handler') > +test:ok(fibb(2) =3D=3D 1, 'run compiled function inside vmevent = handler') >=20 > os.exit(test:check() and 0 or 1) > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 >>=20 >> Regards, >> Sergos >>=20 >>=20 >>> On 19 Oct 2022, at 15:37, Sergey Kaplun = wrote: >>>=20 >>> From: Mike Pall >>>=20 >>> Reported by Sergey Kaplun. >>>=20 >>> (cherry picked from commit b5b20191f3a8a2e2d28f1362b11bd26a51083d89) >>>=20 >>> `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. >=20 > Fixed. >=20 >>=20 >>> 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?=20 >=20 > No, it's about Lua function inside vmevent handler (in bc.dump() for > example) that called on routine to be compiled later. >=20 >> Possibly its better to say: if an vmevent handler itself has a trace=20= >> compiled then execution of this trace can change the `J->exitno` from = the=20 >> original trace we plan to stitch into an taken exit from the = handler=E2=80=99s trace.=20 >=20 > Yes, It's really better:)! Thanks! >=20 >>=20 >>> 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 >>=20 >>> 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. >>=20 >>=20 >>> It's the moment when stitching is >>> broken. >>>=20 >>> This patch saves all necessary fields (`J->exitno`, `J->parent`), = which >> And what about `J-parent`? Isn=E2=80=99t it a trace to stitch to? >=20 > Yes, it's a parent trace for the currently executed one. >=20 >>=20 >>> may change during vmevent handler execution, before vmevent call and >>> restores them after. >>>=20 >>> Sergey Kaplun: >>> * added the description and the test for the problem >>>=20 >>> Resolves tarantool/tarantool#6782 >>> Part of tarantool/tarantool#7230 >>> --- >=20 > The updated commit message is the following: >=20 > | 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=E2=80=99s 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 >=20 > Branch is force pushed, PR is updated. >=20 >>>=20 >>> 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-handl= er >>> 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. >>>=20 >>> 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 >=20 > >=20 >>> 2.34.1 >>>=20 >>=20 >=20 > --=20 > Best regards, > Sergey Kaplun --Apple-Mail=_6E132AF7-0041-4972-A31E-7F6C9EA9636C Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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.
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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 =3D = empty,
+  close =3D = empty,
+  flush =3D = empty,
+})

-- Here we dump (to = /dev/null) info about `fibb()` traces and run
-- `jit.bc` functions = inside.
-test:ok(fibb(0) =3D=3D = 0, 'run compiled function inside vmevent handler')
+test:ok(fibb(2) =3D=3D = 1, 'run compiled function inside vmevent handler')

os.exit(test:check() and = 0 or 1)
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D


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
         &n= bsp;           &nbs= p;            = ^^^^^^^ 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=E2=80=99s 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=E2=80=99t 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=E2=80=99s 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-handl= er
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 =             &n= bsp;           &nbs= p;      |  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

= --Apple-Mail=_6E132AF7-0041-4972-A31E-7F6C9EA9636C--