From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 BC2A94765E0 for ; Fri, 25 Dec 2020 00:26:05 +0300 (MSK) Date: Fri, 25 Dec 2020 00:25:20 +0300 From: Sergey Kaplun Message-ID: <20201224212520.GN9101@root> References: <174b6bcbb0651777a919f8f9ecee38ccb8415694.1608142899.git.skaplun@tarantool.org> <77487E42-42F7-48B0-A795-2117966BAA98@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <77487E42-42F7-48B0-A795-2117966BAA98@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v1 09/11] misc: add Lua API for memory profiler List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.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 > > > + > > +/* 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