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>,
	Mikhail Shishatskiy <m.shishatskiy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 2/7] core: separate the profiling timer from lj_profile
Date: Tue, 21 Sep 2021 14:13:04 +0300
Message-ID: <YUm+QJQE4hzCdMlB@root> (raw)
In-Reply-To: <5068b8cd68b31e4b9cfb381db23bda6e9791f7fb.1631122521.git.m.kokryashkin@tarantool.org>

Hi, Maxim, Mikhail!

Thanks for the patch!

Please consider my comments below.

On 08.09.21, Maxim Kokryashkin wrote:
> From: Mikhail Shishatskiy <m.shishatskiy@tarantool.org>
> 
> This patch makes timer machinery in lj_profile self-reliant by
> introducing lj_profile_timer structure and start/stop interface
> for it. This makes the timer useful for other sampling profiling

Minor: s/This makes the timer useful/This timer is useful/

> features.
> 
> Part of tarantool/tarantool#781
> ---
>  src/CMakeLists.txt        |   1 +
>  src/Makefile.dep.original |   5 +-
>  src/lj_profile.c          | 178 +++-----------------------------------
>  src/lj_profile_timer.c    | 126 +++++++++++++++++++++++++++
>  src/lj_profile_timer.h    |  83 ++++++++++++++++++

Amalgamated build should be updated as well.

>  5 files changed, 227 insertions(+), 166 deletions(-)
>  create mode 100644 src/lj_profile_timer.c
>  create mode 100644 src/lj_profile_timer.h
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 809aac68..50c23cae 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -68,6 +68,7 @@ make_source_list(SOURCES_UTILS
>  make_source_list(SOURCES_PROFILER
>    SOURCES
>      lj_memprof.c
> +    lj_profile_timer.c
>      lj_profile.c

Nit: lj_profile.c should go first (alphabetically).

>  )
>  
> diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original

<snipped>

> diff --git a/src/lj_profile.c b/src/lj_profile.c
> index 7b09a63a..e7e1a303 100644
> --- a/src/lj_profile.c
> +++ b/src/lj_profile.c
> @@ -19,66 +19,19 @@
>  #include "lj_trace.h"
>  #endif
>  #include "lj_profile.h"
> +#include "lj_profile_timer.h"
>  
>  #include "luajit.h"
>  
> -#if LJ_PROFILE_SIGPROF

<snipped>

> -#endif
> -
>  /* Profiler state. */
>  typedef struct ProfileState {
>    global_State *g;		/* VM state that started the profiler. */
>    luaJIT_profile_callback cb;	/* Profiler callback. */
>    void *data;			/* Profiler callback data. */
>    SBuf sb;			/* String buffer for stack dumps. */
> -  int interval;			/* Sample interval in milliseconds. */
>    int samples;			/* Number of samples for next callback. */
>    int vmstate;			/* VM state when profile timer triggered. */
> -#if LJ_PROFILE_SIGPROF
> -  struct sigaction oldsa;	/* Previous SIGPROF state. */
> -#elif LJ_PROFILE_PTHREAD
> -  pthread_mutex_t lock;		/* g->hookmask update lock. */
> -  pthread_t thread;		/* Timer thread. */
> -  int abort;			/* Abort timer thread. */
> -#elif LJ_PROFILE_WTHREAD
> -#if LJ_TARGET_WINDOWS
> -  HINSTANCE wmm;		/* WinMM library handle. */
> -  WMM_TPFUNC wmm_tbp;		/* WinMM timeBeginPeriod function. */
> -  WMM_TPFUNC wmm_tep;		/* WinMM timeEndPeriod function. */
> -#endif
> -  CRITICAL_SECTION lock;	/* g->hookmask update lock. */
> -  HANDLE thread;		/* Timer thread. */
> -  int abort;			/* Abort timer thread. */
> -#endif
> +  lj_profile_timer timer;  /* Profiling timer */

Minor: Use tabs for allignment here.

>  } ProfileState;
>  
>  /* Sadly, we have to use a static profiler state.
> @@ -145,6 +98,7 @@ void LJ_FASTCALL lj_profile_interpreter(lua_State *L)
>    profile_unlock(ps);
>  }
>  
> +

This change is excess.

>  /* Trigger profile hook. Asynchronous call from OS-specific profile timer. */
>  static void profile_trigger(ProfileState *ps)
>  {
> @@ -168,129 +122,21 @@ static void profile_trigger(ProfileState *ps)
>    profile_unlock(ps);
>  }
>  
> -/* -- OS-specific profile timer handling ---------------------------------- */
> -
>  #if LJ_PROFILE_SIGPROF
>  
> -/* SIGPROF handler. */
> -static void profile_signal(int sig)
> +static void profile_handler(int sig, siginfo_t *info, void* ctx)

Why do we need to change signature of this function?

Nit: s/void* ctx/void *ctx/

>  {
>    UNUSED(sig);
> +  UNUSED(info);
> +  UNUSED(ctx);
>    profile_trigger(&profile_state);
>  }
>  
> -/* Start profiling timer. */
> -static void profile_timer_start(ProfileState *ps)
> -{
> -  int interval = ps->interval;
> -  struct itimerval tm;
> -  struct sigaction sa;
> -  tm.it_value.tv_sec = tm.it_interval.tv_sec = interval / 1000;
> -  tm.it_value.tv_usec = tm.it_interval.tv_usec = (interval % 1000) * 1000;
> -  setitimer(ITIMER_PROF, &tm, NULL);
> -  sa.sa_flags = SA_RESTART;
> -  sa.sa_handler = profile_signal;
> -  sigemptyset(&sa.sa_mask);
> -  sigaction(SIGPROF, &sa, &ps->oldsa);
> -}
> -
> -/* Stop profiling timer. */
> -static void profile_timer_stop(ProfileState *ps)
> -{

<snipped>

> -}
> -
> -#elif LJ_PROFILE_PTHREAD
> -
> -/* POSIX timer thread. */
> -static void *profile_thread(ProfileState *ps)
> -{
> -  int interval = ps->interval;
> -#if !LJ_TARGET_PS3
> -  struct timespec ts;
> -  ts.tv_sec = interval / 1000;
> -  ts.tv_nsec = (interval % 1000) * 1000000;
> -#endif
> -  while (1) {
> -#if LJ_TARGET_PS3
> -    sys_timer_usleep(interval * 1000);
>  #else
> -    nanosleep(&ts, NULL);
> -#endif
> -    if (ps->abort) break;
> -    profile_trigger(ps);
> -  }
> -  return NULL;
> -}
> -
> -/* Start profiling timer thread. */
> -static void profile_timer_start(ProfileState *ps)
> -{

<snipped>

> -}
>  
> -/* Stop profiling timer thread. */
> -static void profile_timer_stop(ProfileState *ps)
> +static void profile_handler()
>  {

<snipped>

> -}
> -
> -#elif LJ_PROFILE_WTHREAD
> -
> -/* Windows timer thread. */
> -static DWORD WINAPI profile_thread(void *psx)
> -{
> -  ProfileState *ps = (ProfileState *)psx;
> -  int interval = ps->interval;
> -#if LJ_TARGET_WINDOWS
> -  ps->wmm_tbp(interval);
> -#endif
> -  while (1) {
> -    Sleep(interval);
> -    if (ps->abort) break;
> -    profile_trigger(ps);
> -  }
> -#if LJ_TARGET_WINDOWS
> -  ps->wmm_tep(interval);
> -#endif
> -  return 0;
> -}
> -
> -/* Start profiling timer thread. */
> -static void profile_timer_start(ProfileState *ps)
> -{
> -#if LJ_TARGET_WINDOWS
> -  if (!ps->wmm) {  /* Load WinMM library on-demand. */
> -    ps->wmm = LoadLibraryExA("winmm.dll", NULL, 0);
> -    if (ps->wmm) {
> -      ps->wmm_tbp = (WMM_TPFUNC)GetProcAddress(ps->wmm, "timeBeginPeriod");
> -      ps->wmm_tep = (WMM_TPFUNC)GetProcAddress(ps->wmm, "timeEndPeriod");
> -      if (!ps->wmm_tbp || !ps->wmm_tep) {
> -	ps->wmm = NULL;
> -	return;
> -      }
> -    }
> -  }
> -#endif
> -  InitializeCriticalSection(&ps->lock);
> -  ps->abort = 0;
> -  ps->thread = CreateThread(NULL, 0, profile_thread, ps, 0, NULL);
> -}
> -
> -/* Stop profiling timer thread. */
> -static void profile_timer_stop(ProfileState *ps)
> -{

<snipped>

>  }
>  
>  #endif
> @@ -327,12 +173,14 @@ LUA_API void luaJIT_profile_start(lua_State *L, const char *mode,
>      if (ps->g) return;  /* Profiler in use by another VM. */
>    }
>    ps->g = G(L);
> -  ps->interval = interval;
>    ps->cb = cb;
>    ps->data = data;
>    ps->samples = 0;
>    lj_buf_init(L, &ps->sb);
> -  profile_timer_start(ps);
> +

Nit: I think this empty line is excess.
Feel free to ignore.

> +  ps->timer.opt.interval_msec = interval;
> +  ps->timer.opt.handler = profile_handler;
> +  lj_profile_timer_start(&ps->timer);
>  }
>  
>  /* Stop profiling. */
> @@ -341,7 +189,7 @@ LUA_API void luaJIT_profile_stop(lua_State *L)

<snipped>

> diff --git a/src/lj_profile_timer.c b/src/lj_profile_timer.c
> new file mode 100644
> index 00000000..ce420a85
> --- /dev/null
> +++ b/src/lj_profile_timer.c
> @@ -0,0 +1,126 @@
> +/*
> +** Simple profiling timer extracted from inbuilt luajit profiler

Nit: I suppose we can omit the part about extracting.

> +*/
> +
> +#define lj_profile_timer_c
> +#define LUA_CORE
> +
> +#include "lj_profile_timer.h"
> +
> +#if LJ_HASPROFILE
> +
> +#if LJ_PROFILE_SIGPROF
> +
> +/* Start profiling timer. */
> +void lj_profile_timer_start(lj_profile_timer *timer) {

Minor: use the newline for curly bracket after function declaration.
Here and below.

> +  int interval = timer->opt.interval_msec;

Minor: looks like interval may be declared as `const int`.
Feel free to ignore.

> +  struct itimerval tm;
> +  struct sigaction sa;
> +  tm.it_value.tv_sec = tm.it_interval.tv_sec = interval / 1000;
> +  tm.it_value.tv_usec = tm.it_interval.tv_usec = (interval % 1000) * 1000;
> +  setitimer(ITIMER_PROF, &tm, NULL);
> +  sa.sa_flags = SA_RESTART | SA_SIGINFO;
> +  sa.sa_sigaction = timer->opt.handler;

Why do we need to forcify using of 3 argument handler?

| -  sa.sa_flags = SA_RESTART;
| -  sa.sa_handler = profile_signal;

Looks like the old format is enough.

> +  sigemptyset(&sa.sa_mask);
> +  sigaction(SIGPROF, &sa, &timer->oldsa);
> +}
> +
> +/* Stop profiling timer. */
> +void lj_profile_timer_stop(lj_profile_timer *timer) {

<snipped>

> +}
> +
> +#elif LJ_PROFILE_PTHREAD
> +
> +/* POSIX timer thread. */
> +static void *profile_thread(lj_timer *timer) {

Its lj_profile_timer, not lj_timer :).

Also, `timer_thread()` name is looks better here, as far as it only
handler timer in another thread.
Thoughts?

> +  int interval = timer->opt.interval_msec;
> +#if !LJ_TARGET_PS3
> +  struct timespec ts;
> +  ts.tv_sec = interval / 1000;
> +  ts.tv_nsec = (interval % 1000) * 1000000;
> +#endif
> +  while (1) {
> +#if LJ_TARGET_PS3
> +    sys_timer_usleep(interval * 1000);
> +#else
> +    nanosleep(&ts, NULL);
> +#endif
> +    if (timer->abort)
> +      break;


Minor: I suggest to save old code style here, i.e. without newline.

> +    timer->opt.handler();
> +  }
> +  return NULL;
> +}
> +
> +/* Start profiling timer thread. */
> +void lj_profile_timer_start(lj_profile_timer *timer) {
> +  pthread_mutex_init(&timer->lock, 0);
> +  timer->abort = 0;
> +  pthread_create(&timer->thread, NULL, (void *(*)(void *))profile_thread,
> +                 timer);

Minor: Use tabs for allignment here.

Side note: may be `t` stands for timer is good enough here.
Feel free to ignore.

> +}
> +
> +/* Stop profiling timer thread. */
> +void lj_profile_timer_stop(lj_profile_timer *timer) {

<snipped>

> +}
> +
> +#elif LJ_PROFILE_WTHREAD
> +
> +/* Windows timer thread. */
> +static DWORD WINAPI profile_thread(void *timerx) {

Also, `timer_thread()` name is looks better here, as far as it only
handler timer in another thread.
Thoughts?

> +  lj_profile_timer *timer = (lj_profile_timer *)timerx;
> +  int interval = timer->opt.interval_msec;
> +#if LJ_TARGET_WINDOWS
> +  timer->wmm_tbp(interval);
> +#endif
> +  while (1) {
> +    Sleep(interval);
> +    if (timer->abort)
> +      break;

Minor: I suggest to save old code style here, i.e. without newline.

> +    timer->opt.handler();
> +  }
> +#if LJ_TARGET_WINDOWS
> +  timer->wmm_tep(interval);
> +#endif
> +  return 0;
> +}
> +
> +/* Start profiling timer thread. */
> +void lj_profile_timer_start(lj_profile_timer *timer) {
> +#if LJ_TARGET_WINDOWS
> +  if (!timer->wmm) { /* Load WinMM library on-demand. */
> +    timer->wmm = LoadLibraryExA("winmm.dll", NULL, 0);
> +    if (timer->wmm) {
> +      timer->wmm_tbp =
> +          (WMM_TPFUNC)GetProcAddress(timer->wmm, "timeBeginPeriod");
> +      timer->wmm_tep = (WMM_TPFUNC)GetProcAddress(timer->wmm, "timeEndPeriod");
> +      if (!timer->wmm_tbp || !timer->wmm_tep) {
> +        timer->wmm = NULL;
> +        return;

LuaJIT use quoter-half tabs codestyle. Please, return 1 tab instead 8
spaces.

> +      }
> +    }
> +  }
> +#endif
> +  InitializeCriticalSection(&timer->lock);
> +  timer->abort = 0;
> +  timer->thread = CreateThread(NULL, 0, profile_thread, timer, 0, NULL);
> +}
> +
> +/* Stop profiling timer thread. */
> +void lj_profile_timer_stop(lj_profile_timer *timer) {

<snipped>

> +}
> +
> +#endif
> +
> +#endif  /* LJ_HASPROFILE */
> diff --git a/src/lj_profile_timer.h b/src/lj_profile_timer.h
> new file mode 100644
> index 00000000..af9eabce
> --- /dev/null
> +++ b/src/lj_profile_timer.h
> @@ -0,0 +1,83 @@
> +/*
> +** Simple profiling timer extracted from inbuilt luajit profiler

Nit: I suppose we can omit the part about extracting.

> +*/
> +
> +#ifndef _LJ_PROFILE_TIMER_H
> +#define _LJ_PROFILE_TIMER_H
> +
> +#include <stdint.h>

It's better to use "lj_def.h" here (see it to explanation).

> +
> +#include "lj_arch.h"
> +
> +#if LJ_HASPROFILE
> +
> +#if LJ_PROFILE_SIGPROF

<snipped>

> +#endif
> +
> +typedef struct {
> +#if LJ_PROFILE_SIGPROF
> +  void (*handler)(int, siginfo_t*, void*);
> +#else
> +  void (*handler)(void);
> +#endif

Don't get why do we need different handlers signature here.
If we use the old one i.e. void (*)(void).

> +  uint32_t interval_msec;
> +} lj_profile_timer_opt;

Why do we need a separate structure for these options?

> +

Minor: Please fix mess with tabs and spaces below.

> +typedef struct {
> +  lj_profile_timer_opt opt;
> +#if LJ_PROFILE_SIGPROF
> +  struct sigaction oldsa;  /* Previous SIGPROF state. */
> +#elif LJ_PROFILE_PTHREAD
> +  pthread_mutex_t lock;		 /* g->hookmask update lock. */
> +  pthread_t thread;		     /* Timer thread. */
> +  int abort;			         /* Abort timer thread. */
> +#elif LJ_PROFILE_WTHREAD
> +#if LJ_TARGET_WINDOWS
> +  HINSTANCE wmm;		       /* WinMM library handle. */
> +  WMM_TPFUNC wmm_tbp;		   /* WinMM timeBeginPeriod function. */
> +  WMM_TPFUNC wmm_tep;		   /* WinMM timeEndPeriod function. */
> +#endif
> +  CRITICAL_SECTION lock;	 /* g->hookmask update lock. */
> +  HANDLE thread;		       /* Timer thread. */
> +  int abort;			         /* Abort timer thread. */
> +#endif
> +} lj_profile_timer;
> +
> +/* Start profiling timer */

Typo: missed dot at the end of the sentence.

> +void lj_profile_timer_start(lj_profile_timer *timer);
> +
> +/* Stop profiling timer */

Typo: missed dot at the end of the sentence.

> +void lj_profile_timer_stop(lj_profile_timer *timer);
> +
> +#endif  /* LJ_HASPROFILE */
> +
> +#endif
> -- 
> 2.33.0
> 

-- 
Best regards,
Sergey Kaplun

  reply	other threads:[~2021-09-21 11:14 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 [this message]
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
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=YUm+QJQE4hzCdMlB@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.shishatskiy@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