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 [thread overview]
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
next prev parent reply other threads:[~2021-09-21 11:14 UTC|newest]
Thread overview: 20+ 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
2022-04-07 12:15 ` [Tarantool-patches] [PATCH luajit 0/7] misc: introduce sysprof 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 \
--subject='Re: [Tarantool-patches] [PATCH luajit 2/7] core: separate the profiling timer from lj_profile' \
/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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox