Re: [Tarantool-patches] [PATCH luajit v2 5/7] core: introduce memory profiler

[Sergey Ostanevich <sergos@tarantool.org>]

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

LGTM

Sergos

> On 27 Dec 2020, at 14:54, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 27.12.20, Sergey Ostanevich wrote:
>> Hi!
>> 
>> Thanks for the patch! Just one question below, looks good otherwise.
>> 
>> Sergos
>> 
>>> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
>>> new file mode 100644
>>> index 0000000..e0df057
>>> --- /dev/null
>>> +++ b/src/lj_memprof.c
>> <snipped>
>>> +static int memprof_stop(const struct lua_State *L)
>>> +{
>>> +  struct memprof *mp = &memprof;
>>> +  struct alloc *oalloc = &mp->orig_alloc;
>>> +  struct lj_wbuf *out = &mp->out;
>>> +  int return_status = PROFILE_SUCCESS;
>>> +  int saved_errno = 0;
>>> +  struct lua_State *main_L;
>>> +  int cb_status;
>>> +
>>> +  memprof_lock();
>>> +
>>> +  if (mp->state == MPS_HALT) {
>>> +    errno = mp->saved_errno;
>>> +    mp->state = MPS_IDLE
>>> +    memprof_unlock();
>>> +    return PROFILE_ERRIO;
>>> +  }
>>> +
>>> +  if (mp->state != MPS_PROFILE) {
>>> +    memprof_unlock();
>>> +    return PROFILE_ERRRUN;
>>> +  }
>>> +
>> 
>>> +  if (L != NULL && mp->g != G(L)) {
>>> +    memprof_unlock();
>>> +    return PROFILE_ERR;
>>> +  }
>>> +
>>> +  mp->state = MPS_IDLE;
>>> +
>>> +  lua_assert(mp->g != NULL);
>>> +  main_L = mainthread(mp->g);
>>> +
>>> +  lua_assert(memprof_allocf == lua_getallocf(main_L, NULL));
>>> +  lua_assert(oalloc->allocf != NULL);
>>> +  lua_assert(oalloc->state != NULL);
>>> +  lua_setallocf(main_L, oalloc->allocf, oalloc->state);
>>> +
>>> +  if (LJ_UNLIKELY(lj_wbuf_test_flag(out, STREAM_STOP))) {
>>> +    lua_assert(lj_wbuf_test_flag(out, STREAM_ERR_IO));
>>> +    mp->state = MPS_HALT;
>>> +    /* on_stop call may change errno value. */
>>> +    mp->saved_errno = lj_wbuf_errno(out);
>>> +    /* Ignore possible errors. mp->opt.buf == NULL here. */
>>> +    mp->opt.on_stop(mp->opt.ctx, mp->opt.buf);
>>> +    lj_wbuf_terminate(out);
>>> +    memprof_unlock();
>>> +    return PROFILE_ERRIO;
>>> +  }
>>> +  lj_wbuf_addbyte(out, LJM_EPILOGUE_HEADER);
>>> +
>>> +  lj_wbuf_flush(out);
>>> +
>>> +  cb_status = mp->opt.on_stop(mp->opt.ctx, mp->opt.buf);
>>> +  if (LJ_UNLIKELY(lj_wbuf_test_flag(out, STREAM_ERR_IO) || cb_status != 0)) {
>>> +    saved_errno = lj_wbuf_errno(out);
>> 
>> Previous return of PROFILE_ERRIO causes MPS_HALT. Should it be the same?
> 
> Yes, you're right! Previous MPS_HALT is redundant -- profiler anyway
> should stop (IDLE) immediately, there is no reason to set HALT there.
> Thanks! Fixed!
> 
> See the iterative diff below. Branch is force-pushed.
> 
>> 
>>> +    return_status = PROFILE_ERRIO;
>>> +  }
>>> +
>>> +  lj_wbuf_terminate(out);
>>> +
>>> +  memprof_unlock();
>>> +  errno = saved_errno;
>>> +  return return_status;
>>> +}
>> 
> 
> ===================================================================
> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
> index e0df057..998cbea 100644
> --- a/src/lj_memprof.c
> +++ b/src/lj_memprof.c
> @@ -354,13 +354,13 @@ static int memprof_stop(const struct lua_State *L)
> 
>   if (LJ_UNLIKELY(lj_wbuf_test_flag(out, STREAM_STOP))) {
>     lua_assert(lj_wbuf_test_flag(out, STREAM_ERR_IO));
> -    mp->state = MPS_HALT;
>     /* on_stop call may change errno value. */
> -    mp->saved_errno = lj_wbuf_errno(out);
> +    saved_errno = lj_wbuf_errno(out);
>     /* Ignore possible errors. mp->opt.buf == NULL here. */
>     mp->opt.on_stop(mp->opt.ctx, mp->opt.buf);
>     lj_wbuf_terminate(out);
>     memprof_unlock();
> +    errno = saved_errno;
>     return PROFILE_ERRIO;
>   }
>   lj_wbuf_addbyte(out, LJM_EPILOGUE_HEADER);
> ===================================================================
> 
> -- 
> Best regards,
> Sergey Kaplun


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