Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <max.kokryashkin@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 5/7] memprof: add profile common section
Date: Tue, 5 Oct 2021 13:48:45 +0300
Message-ID: <YVwtjc6tBF3OPdCd@root> (raw)
In-Reply-To: <e5d6b75269b5d38b754f009fad781d59d061df87.1631122521.git.m.kokryashkin@tarantool.org>

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

  reply	other threads:[~2021-10-05 10:50 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-08 17:50 [Tarantool-patches] [PATCH luajit 0/7] misc: introduce sysprof Maxim Kokryashkin via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 1/7] core: save current frame top to lj_obj Maxim Kokryashkin via Tarantool-patches
2021-09-20 17:21   ` Sergey Kaplun via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 2/7] core: separate the profiling timer from lj_profile Maxim Kokryashkin via Tarantool-patches
2021-09-21 11:13   ` Sergey Kaplun via Tarantool-patches
2021-09-23 11:37     ` Mikhail Shishatskiy via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 3/7] memprof: move symtab to a separate module Maxim Kokryashkin via Tarantool-patches
2021-09-22  7:51   ` Sergey Kaplun via Tarantool-patches
2021-09-22  8:14     ` Sergey Kaplun via Tarantool-patches
2021-09-23 14:51   ` [Tarantool-patches] [PATCH luajit v2] " Maxim Kokryashkin via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 4/7] core: introduce lua and platform profiler Maxim Kokryashkin via Tarantool-patches
2021-09-29  6:53   ` Sergey Kaplun via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 5/7] memprof: add profile common section Maxim Kokryashkin via Tarantool-patches
2021-10-05 10:48   ` Sergey Kaplun via Tarantool-patches [this message]
2021-10-06 19:15     ` [Tarantool-patches] [PATCH luajit v2] " Maxim Kokryashkin via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 6/7] sysprof: introduce Lua API Maxim Kokryashkin via Tarantool-patches
2021-10-05 15:36   ` Sergey Kaplun via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 7/7] tools: introduce parsers for sysprof Maxim Kokryashkin via Tarantool-patches
2021-10-07 11:28   ` Sergey Kaplun via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVwtjc6tBF3OPdCd@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=skaplun@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git