From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 75F356EC40; Tue, 21 Sep 2021 14:14:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 75F356EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632222872; bh=tzMDehOBkdNWsoAlOs6MXUa7Epz5qYiFVTf/hQUgQh0=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=rR/xSF4+ILtj6CMkEXrSKXqGIkDgVeelgJIkf42+AZPqaLKHzkmbrmShkbdRjDavn t4QXoWzkaBrHyMiQjcYe00UFW/UGctN5e86CbkTinZRSLYKcJwO5MfLVrtFfGNb2E9 d0+pZc88B2bB30OSxsJpJ29ExrcmeZ8NjC14ic6Y= Received: from smtp35.i.mail.ru (smtp35.i.mail.ru [94.100.177.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8D8306EC40 for ; Tue, 21 Sep 2021 14:14:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8D8306EC40 Received: by smtp35.i.mail.ru with esmtpa (envelope-from ) id 1mSdjK-00015T-HN; Tue, 21 Sep 2021 14:14:31 +0300 Date: Tue, 21 Sep 2021 14:13:04 +0300 To: Maxim Kokryashkin , Mikhail Shishatskiy Message-ID: References: <5068b8cd68b31e4b9cfb381db23bda6e9791f7fb.1631122521.git.m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5068b8cd68b31e4b9cfb381db23bda6e9791f7fb.1631122521.git.m.kokryashkin@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91AE02D33A9C88A2FAC52BB37A480E789638BB4D06A25224200894C459B0CD1B9FC3231A5A9CCD133C3DEFCBC4A6F663A4A9C044C50BD12F3F5EC2512189958EF X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7624C4D757C4F5837EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CDA1F22AAA442F218638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89237E4BBF78B91B4F853864A75ED10E4117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B2EE5AD8F952D28FBA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C565C1E6824D8037B43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A52247887954E65DBAE34E59C1FB536B5E1367A9363CAC59A0D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA756EBE15880F1DDB596D6546786ADF492D5A0AA20F8A030721A0D681C3DB17998A1098AAFFB0A1231D8E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34F7CC4EA888783AE05505AC00B5D4C74FACE6BC22BE27B52CEF4E67395B646627084721AEA3218DC21D7E09C32AA3244C384B75DA632BDAE40851618500F1E7D969B6CAE0477E908D927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnvI84oHUDXDgsKRWHyGrkg== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4CD787FB956F453B1A6EC65A9C35AA1F0A86CF03D785F44ABF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/7] core: separate the profiling timer from lj_profile X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim, Mikhail! Thanks for the patch! Please consider my comments below. On 08.09.21, Maxim Kokryashkin wrote: > From: Mikhail Shishatskiy > > 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 > 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 > -#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) > -{ > -} > - > -#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) > -{ > -} > > -/* Stop profiling timer thread. */ > -static void profile_timer_stop(ProfileState *ps) > +static void profile_handler() > { > -} > - > -#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) > -{ > } > > #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) > 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) { > +} > + > +#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) { > +} > + > +#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) { > +} > + > +#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 It's better to use "lj_def.h" here (see it to explanation). > + > +#include "lj_arch.h" > + > +#if LJ_HASPROFILE > + > +#if LJ_PROFILE_SIGPROF > +#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