From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergos@tarantool.org>
Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107])
 (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 A091F4765E0
 for <tarantool-patches@dev.tarantool.org>;
 Thu, 24 Dec 2020 19:32:22 +0300 (MSK)
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
From: Sergey Ostanevich <sergos@tarantool.org>
In-Reply-To: <174b6bcbb0651777a919f8f9ecee38ccb8415694.1608142899.git.skaplun@tarantool.org>
Date: Thu, 24 Dec 2020 19:32:20 +0300
Content-Transfer-Encoding: quoted-printable
Message-Id: <77487E42-42F7-48B0-A795-2117966BAA98@tarantool.org>
References: <cover.1608142899.git.skaplun@tarantool.org>
 <174b6bcbb0651777a919f8f9ecee38ccb8415694.1608142899.git.skaplun@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH luajit v1 09/11] misc: add Lua API
	for memory profiler
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

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 =3D misc.memprof.start(fname) */
> +LJLIB_CF(misc_memprof_start)
> +{
> +  struct luam_Prof_options opt =3D {0};
> +  struct memprof_ctx *ctx;
> +  const char *fname;
> +  int memprof_status;
> +  int started;
> +
> +  fname =3D strdata(lj_lib_checkstr(L, 1));
> +
> +  ctx =3D lj_mem_new(L, sizeof(*ctx));
> +  if (ctx =3D=3D NULL)
> +    goto errmem;
> +
> +  opt.ctx =3D ctx;
> +  opt.writer =3D buffer_writer_default;
> +  opt.on_stop =3D on_stop_cb_default;
> +  opt.len =3D STREAM_BUFFER_SIZE;
> +  opt.buf =3D (uint8_t *)lj_mem_new(L, STREAM_BUFFER_SIZE);
> +  if (NULL =3D=3D opt.buf) {
> +    lj_mem_free(G(L), ctx, sizeof(*ctx));
> +    goto errmem;
> +  }
> +
> +  ctx->g =3D G(L);
> +  ctx->stream =3D fopen(fname, "wb");
> +
> +  if (ctx->stream =3D=3D NULL) {
> +    memprof_ctx_free(ctx, opt.buf);
> +    return luaL_fileresult(L, 0, fname);
> +  }
> +
> +  memprof_status =3D ljp_memprof_start(L, &opt);
> +  started =3D memprof_status =3D=3D 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 =E2=80=98false=E2=80=99 without any =
err/errno set.

> +    }
> +  }
> +  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 =3D misc.memprof.stop() */
> +LJLIB_CF(misc_memprof_stop)
> +{
> +  int status =3D ljp_memprof_stop();
> +  int stopped_successfully =3D status =3D=3D 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;
> +}
> +