Tarantool development patches archive
 help / color / mirror / Atom feed
  • * 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

  • end of thread, other threads:[~2022-12-22 11:40 UTC | newest]
    
    Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
    -- links below jump to the message on this page --
         [not found] <20221019123730.25039-1-skaplun@tarantool.org>
         [not found] ` <B2597DDA-8DF4-48FC-A8FF-CD1250BF61D1@tarantool.org>
         [not found]   ` <Y1k5ON/YOyhYz/Zx@root>
         [not found]     ` <1667026151.120548758@f533.i.mail.ru>
    2022-11-10  6:27       ` [Tarantool-patches] [PATCH luajit] Save trace recorder state around VM event call.\ Sergey Kaplun via Tarantool-patches
    2022-12-22 11:40     ` sergos via Tarantool-patches
    

    This is a public inbox, see mirroring instructions
    for how to clone and mirror all data and code used for this inbox