LGTM Best regards, Sergos  Sunday, 27 December 2020, 16:43 +0300 from skaplun@tarantool.org : > >Hi! > >Thanks for the review! > >On 27.12.20, Sergey Ostanevich wrote: >> Hi! >> >> Thanks for the patch! >> >> 2 nits below, LGTM after the fix. >> >> Sergos >> >> >> > On 25 Dec 2020, at 18:26, Sergey Kaplun < skaplun@tarantool.org > wrote: >> > >> > This patch introduces Lua API for LuaJIT memory profiler implemented in >> > the scope of the previous patch. >> > >> > Profiler returns some 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 build without memory profiler both return `false`. >> ^^^^^^ was built > >Changed to `is build`. > >> > >> > have adjusted with two new errors >> > PROF_ISRUNNING/PROF_NOTRUNNING returned in case when profiler has >> > started/stopped already correspondingly. >> > >> > Part of tarantool/tarantool#5442 >> > --- >> > >> > Changes in v2: >> > - Added pushing of errno for ERR_PROF* and ERRMEM >> > - Added forgotten assert. >> > >> > src/Makefile.dep | 5 +- >> > src/lib_misc.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++ >> > src/lj_errmsg.h | 6 ++ >> > 3 files changed, 176 insertions(+), 2 deletions(-) >> > >> > diff --git a/src/Makefile.dep b/src/Makefile.dep >> > index 8ae14a5..c3d0977 100644 >> > --- a/src/Makefile.dep >> > +++ b/src/Makefile.dep >> > @@ -29,8 +29,9 @@ lib_jit.o: lib_jit.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h lj_def.h \ >> > lj_vm.h lj_vmevent.h lj_lib.h luajit.h lj_libdef.h >> > lib_math.o: lib_math.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h \ >> > lj_def.h lj_arch.h lj_lib.h lj_vm.h lj_libdef.h >> > -lib_misc.o: lib_misc.c lua.h luaconf.h lmisclib.h lj_obj.h lj_def.h lj_arch.h \ >> > - lj_str.h lj_tab.h lj_lib.h lj_libdef.h >> > +lib_misc.o: lib_misc.c lua.h luaconf.h lmisclib.h lauxlib.h lj_obj.h \ >> > + lj_def.h lj_arch.h lj_str.h lj_tab.h lj_lib.h lj_gc.h lj_err.h \ >> > + lj_errmsg.h lj_memprof.h lj_libdef.h >> > lib_os.o: lib_os.c lua.h luaconf.h lauxlib.h lualib.h lj_obj.h lj_def.h \ >> > lj_arch.h lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h lj_lib.h \ >> > lj_libdef.h >> > diff --git a/src/lib_misc.c b/src/lib_misc.c >> > index 6f7b9a9..36fe29f 100644 >> > --- a/src/lib_misc.c >> > +++ b/src/lib_misc.c >> > @@ -8,13 +8,21 @@ >> > #define lib_misc_c >> > #define LUA_LIB >> > >> > +#include >> > +#include >> > + >> > #include "lua.h" >> > #include "lmisclib.h" >> > +#include "lauxlib.h" >> > >> > #include "lj_obj.h" >> > #include "lj_str.h" >> > #include "lj_tab.h" >> > #include "lj_lib.h" >> > +#include "lj_gc.h" >> > +#include "lj_err.h" >> > + >> > +#include "lj_memprof.h" >> > >> > /* ------------------------------------------------------------------------ */ >> > >> > @@ -67,8 +75,167 @@ LJLIB_CF(misc_getmetrics) >> > >> > #include "lj_libdef.h" >> > >> > +/* ----- misc.memprof module ---------------------------------------------- */ >> > + >> > +#define LJLIB_MODULE_misc_memprof >> > + >> > +/* >> > +** Yep, 8Mb. Tuned in order not to bother the platform with too often flushes. >> > +*/ >> > +#define STREAM_BUFFER_SIZE (8 * 1024 * 1024) >> > + >> > +/* Structure given as ctx to memprof writer and on_stop callback. */ >> > +struct memprof_ctx { >> > + /* Output file stream for data. */ >> > + FILE *stream; >> > + /* Profiled global_State for lj_mem_free at on_stop callback. */ >> > + global_State *g; >> > +}; >> > + >> > +static LJ_AINLINE void memprof_ctx_free(struct memprof_ctx *ctx, uint8_t *buf) >> > +{ >> > + lj_mem_free(ctx->g, buf, STREAM_BUFFER_SIZE); >> > + lj_mem_free(ctx->g, ctx, sizeof(*ctx)); >> > +} >> > + >> > +/* Default buffer writer function. Just call fwrite to corresponding FILE. */ >> > +static size_t buffer_writer_default(const void **buf_addr, size_t len, >> > + void *opt) >> > +{ >> > + FILE *stream = ((struct memprof_ctx *)opt)->stream; >> > + const void * const buf_start = *buf_addr; >> > + const void *data = *buf_addr; >> > + size_t write_total = 0; >> > + >> > + lua_assert(len <= STREAM_BUFFER_SIZE); >> > + >> > + for (;;) { >> > + const size_t written = fwrite(data, 1, len, stream); >> > + >> > + if (LJ_UNLIKELY(written == 0)) { >> > + /* Re-tries write in case of EINTR. */ >> > + if (errno == EINTR) { >> > + errno = 0; >> > + continue; >> > + } >> > + break; >> > + } >> > + >> > + write_total += written; >> > + >> > + if (write_total == len) >> > + break; >> > + >> > + data = (uint8_t *)data + (ptrdiff_t)written; >> >> After incomplete write you’ll return to the fwrite() call with >> data pointer moved, but with len untouched -> you’ll read beyond >> the buffer. > >Oh! I keep stepping on the same rake... >Thank you very much! Fixed! > >See the iterative patch below. Branch is force pushed. > >> >> > + } >> > + lua_assert(write_total <= len); >> > + >> > + *buf_addr = buf_start; >> > + return write_total; >> > +} >> > + >> > +/* Default on stop callback. Just close corresponding stream. */ >> > +static int on_stop_cb_default(void *opt, uint8_t *buf) >> > +{ >> > + struct memprof_ctx *ctx = opt; >> > + FILE *stream = ctx->stream; >> > + memprof_ctx_free(ctx, buf); >> > + return fclose(stream); >> > +} >> > + >> > +/* local started, err, errno = misc.memprof.start(fname) */ >> > +LJLIB_CF(misc_memprof_start) >> > +{ >> > + struct lua_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 = lj_memprof_start(L, &opt); >> > + started = memprof_status == PROFILE_SUCCESS; >> > + >> > + if (LJ_UNLIKELY(!started)) { >> > + fclose(ctx->stream); >> > + remove(fname); >> > + memprof_ctx_free(ctx, opt.buf); >> > + switch (memprof_status) { >> > + case PROFILE_ERRRUN: >> > + lua_pushnil(L); >> > + lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING)); >> > + lua_pushinteger(L, EINVAL); >> > + return 3; >> > + case PROFILE_ERRIO: >> > + return luaL_fileresult(L, 0, fname); >> > + default: >> > + lua_assert(0); >> > + break; >> > + } >> > + } >> > + lua_pushboolean(L, started); >> > + >> > + return 1; >> > +errmem: >> > + lua_pushnil(L); >> > + lua_pushstring(L, err2msg(LJ_ERR_ERRMEM)); >> > + lua_pushinteger(L, ENOMEM); >> > + return 3; >> > +} >> > + >> > +/* local stopped, err, errno = misc.memprof.stop() */ >> > +LJLIB_CF(misc_memprof_stop) >> > +{ >> > + int status = lj_memprof_stop(); >> > + int stopped_successfully = status == PROFILE_SUCCESS; >> > + if (!stopped_successfully) { >> > + switch (status) { >> > + case PROFILE_ERRRUN: >> > + lua_pushnil(L); >> > + lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING)); >> > + lua_pushinteger(L, EINVAL); >> > + return 3; >> > + case PROFILE_ERRIO: >> > + return luaL_fileresult(L, 0, NULL); >> > + default: >> > + lua_assert(0); >> > + break; >> > + } >> > + } >> > + lua_pushboolean(L, stopped_successfully); >> > + return 1; >> > +} >> > + >> > +#include "lj_libdef.h" >> > + >> > +/* ------------------------------------------------------------------------ */ >> > + >> > LUALIB_API int luaopen_misc(struct lua_State *L) >> > { >> > LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc); >> > + LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof); >> > return 1; >> > } >> > diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h >> > index de7b867..6816da2 100644 >> > --- a/src/lj_errmsg.h >> > +++ b/src/lj_errmsg.h >> > @@ -185,6 +185,12 @@ ERRDEF(FFI_NYIPACKBIT, "NYI: packed bit fields") >> > ERRDEF(FFI_NYICALL, "NYI: cannot call this C function (yet)") >> > #endif >> > >> > +#if LJ_HASPROFILE || LJ_HASMEMPROF >> > +/* Profiler errors. */ >> > +ERRDEF(PROF_ISRUNNING, "profiler is running already") >> > +ERRDEF(PROF_NOTRUNNING, "profiler is not running") >> > +#endif >> > + >> > #undef ERRDEF >> > >> > /* Detecting unused error messages: >> > -- >> > 2.28.0 >> > >> > >=================================================================== >diff --git a/src/lib_misc.c b/src/lib_misc.c >index 36fe29f..f69f933 100644 >--- a/src/lib_misc.c >+++ b/src/lib_misc.c >@@ -110,7 +110,7 @@ static size_t buffer_writer_default(const void **buf_addr, size_t len, >   lua_assert(len <= STREAM_BUFFER_SIZE); >  >   for (;;) { >- const size_t written = fwrite(data, 1, len, stream); >+ const size_t written = fwrite(data, 1, len - write_total, stream); >  >     if (LJ_UNLIKELY(written == 0)) { >       /* Re-tries write in case of EINTR. */ >@@ -122,13 +122,13 @@ static size_t buffer_writer_default(const void **buf_addr, size_t len, >     } >  >     write_total += written; >+ lua_assert(write_total <= len); >  >     if (write_total == len) >       break; >  >     data = (uint8_t *)data + (ptrdiff_t)written; >   } >- lua_assert(write_total <= len); >  >   *buf_addr = buf_start; >   return write_total; >=================================================================== > >-- >Best regards, >Sergey Kaplun