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 6D4B86FF9F; Tue, 5 Oct 2021 13:50:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6D4B86FF9F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1633431016; bh=9/zBGdqgXCQLDgKPNXPUD1VZ6PuPDwFpv9x8PMFrCss=; 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=kUdlfb5XpCp4HAxYRksCW7ynPBSAoQnCMPR2rWqZg4NppFI62Uv4K8RdAeqhDfx/u YavrQx9MefC5247rJDKmwDj/8cFHeiUsoUbEFzQlzgVaTmRAEvvj4j8j+5CZbxmzCN 31niDwqgZGr/EsB7nZ089QONOUbLqixCAMBCUKbw= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 38D7E6FF9F for ; Tue, 5 Oct 2021 13:50:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 38D7E6FF9F Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mXi1W-0002AF-7y; Tue, 05 Oct 2021 13:50:14 +0300 Date: Tue, 5 Oct 2021 13:48:45 +0300 To: Maxim Kokryashkin Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9064ADF4728AA0EE9F29F6F937CFD73092774A1760F25EB43182A05F5380850409413AE4746D25DA74E94FD99B0BCAD1F8D0CE43E9D375D942F322158D1F90E36 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7081BBE264C6D7F42EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637160171C9EBC7AFE48638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8145369D9A8479C32983B62A06479DCED117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D0C9BB9AE6BD5D69089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5ED8B32272B9B90C977BCAEE9CFD7C25FE240819CD8ED093BD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75C4D20244F7083972410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3455AC8BF8E3153BA00BA25F84265126272A91AB2C68431A4AF44CEFAB7A4A3CCF60334950DBB5D1351D7E09C32AA3244C7678BB40E58B80D50C077CCCE086CB3DD08D48398F32B4A6927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojhAh8SZXECpB2gIqWupt5fw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D323899887508E5239A5087120691DFEC0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED567EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 5/7] memprof: add profile common section 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! 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