[Tarantool-patches] [PATCH luajit 5/7] memprof: add profile common section
Sergey Kaplun
skaplun at tarantool.org
Tue Oct 5 13:48:45 MSK 2021
Hi, Maxim!
Thanks for the patch!
Please consider my comments below.
On 08.09.21, Maxim Kokryashkin wrote:
> A Lua API for sysprof needs to be introduced, but sysprof's C API is quite
Nevertheless, there is a dramatic difference: memprof's callback isn't
called inside a signal handler, but sysprof's is. So in sysprof we can't
use non asinc-signal-safe functions and system calls. It means that we
can't use `fwrite()`, for example, because it manages internal buffer
associated with `FILE *`. It is possible that another one signal is
handled when we write via `FILE *` which using pointer related to buffer
or other related variables aren't updated yet. Then the second call to
`fwrite()` will operate on inconsistent data.
See also:
| man 7 signal-safety
> similar with memprof's. Considering this, there are some structures and
> functions that should be common between memprof's and sysprof's
Typo? s/between/among/
> implementations of Lua API to avoid duplication.
Typo: s/Lua API/the Lua API/
Minor: Please align the commit message. See Code-Review procedure [1].
| Commit message should fit 72 symbols line width. This comes from GitHub
| UI restrictions, and from general rules of formatting commits in git.
| For example 72 comes from being able to add 4 whitespaces shift, so as
| the result message is aligned in the middle of 80. However sometimes 50
| is too small, so this is an optional limit. The commit message limit of
| 72 symbols is not applied when need to copy-paste something long without
| changes, such as a failed test output.
>
> Part of tarantool/tarantool#781
> ---
> src/lib_misc.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 1dab08cc..a44476e0 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -75,9 +75,7 @@ LJLIB_CF(misc_getmetrics)
>
> #include "lj_libdef.h"
>
> -/* ----- misc.memprof module ---------------------------------------------- */
> -
> -#define LJLIB_MODULE_misc_memprof
> +/* --------- profile common ---------------------------------------------- */
Minor: I prefer "profile common section".
Feel free to ignore.
>
> /*
> ** Yep, 8Mb. Tuned in order not to bother the platform with too often flushes.
> @@ -85,7 +83,7 @@ LJLIB_CF(misc_getmetrics)
> #define STREAM_BUFFER_SIZE (8 * 1024 * 1024)
>
> /* Structure given as ctx to memprof writer and on_stop callback. */
> -struct memprof_ctx {
> +struct profile_ctx {
> /* Output file stream for data. */
> FILE *stream;
> /* Profiled global_State for lj_mem_free at on_stop callback. */
> @@ -101,7 +99,7 @@ struct memprof_ctx {
> static size_t buffer_writer_default(const void **buf_addr, size_t len,
> void *opt)
> {
> - struct memprof_ctx *ctx = opt;
> + struct profile_ctx *ctx = opt;
> FILE *stream = ctx->stream;
> const void * const buf_start = *buf_addr;
> const void *data = *buf_addr;
> @@ -140,19 +138,22 @@ static size_t buffer_writer_default(const void **buf_addr, size_t len,
> /* Default on stop callback. Just close the corresponding stream. */
> static int on_stop_cb_default(void *opt, uint8_t *buf)
> {
> - struct memprof_ctx *ctx = opt;
> + struct profile_ctx *ctx = opt;
> FILE *stream = ctx->stream;
> UNUSED(buf);
> lj_mem_free(ctx->g, ctx, sizeof(*ctx));
> return fclose(stream);
> }
>
> +/* ----- misc.memprof module ---------------------------------------------- */
> +
> +#define LJLIB_MODULE_misc_memprof
> /* local started, err, errno = misc.memprof.start(fname) */
> LJLIB_CF(misc_memprof_start)
> {
> struct lj_memprof_options opt = {0};
> const char *fname = strdata(lj_lib_checkstr(L, 1));
> - struct memprof_ctx *ctx;
> + struct profile_ctx *ctx;
> int memprof_status;
>
> /*
> --
> 2.33.0
>
[1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list