Re: [Tarantool-patches] [PATCH luajit v1 09/11] misc: add Lua API for memory profiler

[Sergey Kaplun <skaplun@tarantool.org>]

Hi!

Thanks for the review!

On 24.12.20, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> Although, the patch might be outdate, still I have some comments below
> that are still relevant in branch.
> 
> > diff --git a/src/lib_misc.c b/src/lib_misc.c
> > index 6f7b9a9..d3b5ab4 100644
> > --- a/src/lib_misc.c
> > +++ b/src/lib_misc.c
> <snipped>
> > +
> > +/* local started, err, errno = misc.memprof.start(fname) */
> > +LJLIB_CF(misc_memprof_start)
> > +{
> > +  struct luam_Prof_options opt = {0};
> > +  struct memprof_ctx *ctx;
> > +  const char *fname;
> > +  int memprof_status;
> > +  int started;
> > +
> > +  fname = strdata(lj_lib_checkstr(L, 1));
> > +
> > +  ctx = lj_mem_new(L, sizeof(*ctx));
> > +  if (ctx == NULL)
> > +    goto errmem;
> > +
> > +  opt.ctx = ctx;
> > +  opt.writer = buffer_writer_default;
> > +  opt.on_stop = on_stop_cb_default;
> > +  opt.len = STREAM_BUFFER_SIZE;
> > +  opt.buf = (uint8_t *)lj_mem_new(L, STREAM_BUFFER_SIZE);
> > +  if (NULL == opt.buf) {
> > +    lj_mem_free(G(L), ctx, sizeof(*ctx));
> > +    goto errmem;
> > +  }
> > +
> > +  ctx->g = G(L);
> > +  ctx->stream = fopen(fname, "wb");
> > +
> > +  if (ctx->stream == NULL) {
> > +    memprof_ctx_free(ctx, opt.buf);
> > +    return luaL_fileresult(L, 0, fname);
> > +  }
> > +
> > +  memprof_status = ljp_memprof_start(L, &opt);
> > +  started = memprof_status == LUAM_PROFILE_SUCCESS;
> > +
> > +  if (LJ_UNLIKELY(!started)) {
> > +    fclose(ctx->stream);
> > +    remove(fname);
> > +    memprof_ctx_free(ctx, opt.buf);
> > +    switch (memprof_status) {
> > +    case LUAM_PROFILE_ERRRUN:
> > +      lua_pushnil(L);
> > +      setstrV(L, L->top++, lj_err_str(L, LJ_ERR_PROF_ISRUNNING));
> > +      return 2;
> > +    case LUAM_PROFILE_ERRMEM:
> > +      /* Unreachable for now. */
> > +      goto errmem;
> > +    case LUAM_PROFILE_ERRIO:
> > +      return luaL_fileresult(L, 0, fname);
> > +    default:
> > +      break;
> 
> This one means, that you can return ‘false’ without any err/errno set.

Thanks! I've forgotten to add `lua_assert(0)`. This case is
unreachable, but in release build we should show to the user that
something goes wrong.

> 
> > +    }
> > +  }
> > +  lua_pushboolean(L, started);
> > +
> > +  return 1;
> > +errmem:
> > +  lua_pushnil(L);
> > +  setstrV(L, L->top++, lj_err_str(L, LJ_ERR_ERRMEM));
> > +  return 2;
> > +}
> > +
> > +/* local stopped, err, errno = misc.memprof.stop() */
> > +LJLIB_CF(misc_memprof_stop)
> > +{
> > +  int status = ljp_memprof_stop();
> > +  int stopped_successfully = status == LUAM_PROFILE_SUCCESS;
> > +  if (!stopped_successfully) {
> > +    switch (status) {
> > +    case LUAM_PROFILE_ERRRUN:
> > +      lua_pushnil(L);
> > +      setstrV(L, L->top++, lj_err_str(L, LJ_ERR_PROF_NOTRUNNING));
> > +      return 2;
> > +    case LUAM_PROFILE_ERRIO:
> > +      return luaL_fileresult(L, 0, NULL);
> > +    default:
> > +      break;
> 
> Ditto
> 
> > +    }
> > +  }
> > +  lua_pushboolean(L, stopped_successfully);
> > +  return 1;
> > +}
> > +
> 

-- 
Best regards,
Sergey Kaplun