From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [94.100.177.91]) (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 7D8E14765E0 for ; Mon, 28 Dec 2020 08:20:09 +0300 (MSK) Date: Mon, 28 Dec 2020 08:19:22 +0300 From: Sergey Kaplun Message-ID: <20201228051922.GJ14702@root> References: <20201228020537.1913-1-skaplun@tarantool.org> <20201228024914.GN5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201228024914.GN5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v3 2/2] misc: add Lua API for memory profiler List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, Thanks for the review! On 28.12.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! LGTM with the several comments below. > > On 28.12.20, Sergey Kaplun wrote: > > This patch introduces Lua API for LuaJIT memory profiler implemented in > > the scope of the previous patch. > > > > Profiler returns true value if started/stopped successfully, > > returns nil on failure (plus an error message as a second result and a > > system-dependent error code as a third result). > > If LuaJIT is build without memory profiler both return `false`. > > Typo: s/`false`/false/ considering true and nil above. Done. > > > > > have adjusted with three new errors > > Typo: s/have adjusted/has been adjusted/. Fixed. > > > PROF_MISUSE/PROF_ISRUNNING/PROF_NOTRUNNING returned in case when > > profiler has used incorrectly/started/stopped already correspondingly. > > > > Part of tarantool/tarantool#5442 > > --- > > > > Changes in v3: > > * Fixed lj_mem_new misuse. > > * Moved buffer inside ctx. > > * Codestyle fixes. > > > > src/Makefile.dep | 5 +- > > src/lib_misc.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++ > > src/lj_errmsg.h | 7 ++ > > 3 files changed, 176 insertions(+), 2 deletions(-) > > > > > > > diff --git a/src/lib_misc.c b/src/lib_misc.c > > index 6f7b9a9..619cfb7 100644 > > --- a/src/lib_misc.c > > +++ b/src/lib_misc.c > > > > > @@ -67,8 +75,166 @@ LJLIB_CF(misc_getmetrics) > > > > > +/* local started, err, errno = misc.memprof.start(fname) */ > > +LJLIB_CF(misc_memprof_start) > > +{ > > > > > + if (LJ_UNLIKELY(memprof_status != PROFILE_SUCCESS)) { > > + lj_mem_free(ctx->g, ctx, sizeof(*ctx)); > > This deallocation causes double free if PROFILE_ERRIO occurs, since the > ctx is released within on_stop callback. > > ctx->stream should be closed if PROFILE_ERR(USE|RUN) occurs. This is the reason why I suggested not to call on_stop callback on finish -- this "corner case" looks weird. Nice catch! See the iterative patch below. > > > + switch (memprof_status) { > > > > > + } > > + } > > + lua_pushboolean(L, 1); > > + > > Typo: Excess newline. Fixed. > > > + return 1; > > +} > > > > > -- > > 2.28.0 > > > > -- > Best regards, > IM =================================================================== diff --git a/src/lib_misc.c b/src/lib_misc.c index 619cfb7..958c189 100644 --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -177,7 +177,10 @@ LJLIB_CF(misc_memprof_start) memprof_status = lj_memprof_start(L, &opt); if (LJ_UNLIKELY(memprof_status != PROFILE_SUCCESS)) { - lj_mem_free(ctx->g, ctx, sizeof(*ctx)); + if (memprof_status == PROFILE_ERRIO) { + fclose(ctx->stream); + lj_mem_free(ctx->g, ctx, sizeof(*ctx)); + } switch (memprof_status) { case PROFILE_ERRUSE: lua_pushnil(L); @@ -197,7 +200,6 @@ LJLIB_CF(misc_memprof_start) } } lua_pushboolean(L, 1); - return 1; } =================================================================== -- Best regards, Sergey Kaplun