> On 20 Nov 2018, at 13:49, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > >> >> Thanks Frediano, >> >> >> Reviewed-by: Christophe de Dinechin <dinechin@xxxxxxxxxx> >> >> >>> On 19 Nov 2018, at 21:46, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: >>> >>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx> >> >> Replace with “Suggested-by: “ :-) >> >> >>> >>> Allow to use recorder library. See https://github.com/c3d/recorder for >>> details. >>> The main usage will be to collect statistics while the programs will run. >>> By default the recorder will be disabled at compile time. >>> Both autoconf and Meson are supported. >>> Autoconf requires the addition of SPICE_CHECK_RECORDER call in >>> configure.ac. >>> Meson requires to add recorder option. >>> >>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> >>> --- >>> .gitmodules | 3 ++ >>> common/Makefile.am | 10 +++++ >>> common/log.c | 3 ++ >>> common/meson.build | 12 +++++- >>> common/recorder | 1 + >>> common/recorder.h | 75 +++++++++++++++++++++++++++++++++++++ >>> configure.ac | 8 +++- >>> m4/spice-deps.m4 | 15 ++++++++ >>> meson.build | 12 +++++- >>> meson_options.txt | 6 +++ >>> tests/Makefile.am | 12 ++++++ >>> tests/test-dummy-recorder.c | 53 ++++++++++++++++++++++++++ >>> 12 files changed, 206 insertions(+), 4 deletions(-) >>> create mode 160000 common/recorder >>> create mode 100644 common/recorder.h >>> create mode 100644 tests/test-dummy-recorder.c >>> >>> diff --git a/.gitmodules b/.gitmodules >>> index e69de29..a7ea8b1 100644 >>> --- a/.gitmodules >>> +++ b/.gitmodules >>> @@ -0,0 +1,3 @@ >>> +[submodule "common/recorder"] >>> + path = common/recorder >>> + url = https://github.com/c3d/recorder.git >> >> Shouldn’t you keep a clone somewhere in the SPICE repo? >> > > Sure, if no complain I would clone to https://gitlab.freedesktop.org/spice group > and change url to "../recorder.git". > >> >>> diff --git a/common/Makefile.am b/common/Makefile.am >>> index da0d941..4c0a6cd 100644 >>> --- a/common/Makefile.am >>> +++ b/common/Makefile.am >>> @@ -51,8 +51,18 @@ libspice_common_la_SOURCES = \ >>> snd_codec.c \ >>> snd_codec.h \ >>> verify.h \ >>> + recorder.h \ >>> $(NULL) >>> >>> +if ENABLE_RECORDER >>> +libspice_common_la_SOURCES += \ >>> + recorder/recorder.c \ >>> + recorder/recorder.h \ >>> + recorder/recorder_ring.c \ >>> + recorder/recorder_ring.h \ >>> + $(NULL) >>> +endif >> >> I see you decided not to build it as a separate DLL after all. >> > > Was already "ready", but can be changed. Can be done later too. > >>> + >>> # These 2 files are not build as part of spice-common >>> # build system, but modules using spice-common will build >>> # them with the appropriate options. We need to let automake >>> diff --git a/common/log.c b/common/log.c >>> index b59c8a8..a7806c5 100644 >>> --- a/common/log.c >>> +++ b/common/log.c >>> @@ -27,6 +27,8 @@ >>> #include <unistd.h> >>> #endif >>> >>> +#include <common/recorder.h> >>> + >>> #include "log.h" >>> #include "backtrace.h" >>> >>> @@ -154,6 +156,7 @@ SPICE_CONSTRUCTOR_FUNC(spice_log_init) >>> if (!g_thread_supported()) >>> g_thread_init(NULL); >>> #endif >>> + recorder_dump_on_common_signals(0, 0); >>> } >>> >>> static void spice_logv(const char *log_domain, >>> diff --git a/common/meson.build b/common/meson.build >>> index 6ac55dc..e715e31 100644 >>> --- a/common/meson.build >>> +++ b/common/meson.build >>> @@ -35,9 +35,19 @@ spice_common_sources = [ >>> 'rop3.h', >>> 'snd_codec.c', >>> 'snd_codec.h', >>> - 'verify.h' >>> + 'verify.h', >>> + 'recorder.h' >>> ] >>> >>> +if get_option('recorder') >>> + spice_common_sources += [ >>> + 'recorder/recorder.c', >>> + 'recorder/recorder.h', >>> + 'recorder/recorder_ring.c', >>> + 'recorder/recorder_ring.h' >>> + ] >>> +endif >>> + >>> spice_common_lib = static_library('spice-common', spice_common_sources, >>> install : false, >>> include_directories : >>> spice_common_include, >>> diff --git a/common/recorder b/common/recorder >>> new file mode 160000 >>> index 0000000..8f450dc >>> --- /dev/null >>> +++ b/common/recorder >>> @@ -0,0 +1 @@ >>> +Subproject commit 8f450dcf0ec725a41b80c495ecdd6110dd1fcdb8 >>> diff --git a/common/recorder.h b/common/recorder.h >>> new file mode 100644 >>> index 0000000..4496de9 >>> --- /dev/null >>> +++ b/common/recorder.h >>> @@ -0,0 +1,75 @@ >>> +/* >>> + Copyright (C) 2018 Red Hat, Inc. >>> + >>> + This library is free software; you can redistribute it and/or >>> + modify it under the terms of the GNU Lesser General Public >>> + License as published by the Free Software Foundation; either >>> + version 2.1 of the License, or (at your option) any later version. >>> + >>> + This library is distributed in the hope that it will be useful, >>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + Lesser General Public License for more details. >>> + >>> + You should have received a copy of the GNU Lesser General Public >>> + License along with this library; if not, see >>> <http://www.gnu.org/licenses/>. >>> +*/ >>> +/* This file include recorder library headers or if disabled provide >>> + * replacement declarations */ >>> +#ifndef ENABLE_RECORDER >>> + >>> +#include <stdio.h> >>> +#include <stdint.h> >>> + >>> +/* Replacement declarations. >>> + * There declarations should generate no code (beside when no optimization >>> are >>> + * selected) but catch some possible programming warnings/errors at >>> + * compile/link time like: >>> + * - usage of undeclared recorders; >>> + * - recording formats and arguments; >>> + * - matching RECORD_TIMING_BEGIN and RECORD_TIMING_END. >>> + * The only exceptions are tweaks which just generate a variable to hold >>> the >>> + * value. >>> + */ >>> + >>> +typedef struct SpiceEmptyStruct { >>> + char dummy[0]; >>> +} SpiceEmptyStruct; >>> + >>> +typedef struct SpiceDummyTweak { >>> + intptr_t tweak_value; >>> +} SpiceDummyTweak; >>> + >>> +#define RECORDER_DECLARE(rec) \ >>> + extern const SpiceEmptyStruct spice_recorder_ ## rec >>> +#define RECORDER(rec, num_rings, comment) \ >>> + RECORDER_DEFINE(rec, num_rings, comment) >>> +#define RECORDER_DEFINE(rec, num_rings, comment) \ >>> + const SpiceEmptyStruct spice_recorder_ ## rec = {} >>> +#define RECORDER_TRACE(rec) \ >>> + (sizeof(spice_recorder_ ## rec) != sizeof(SpiceEmptyStruct)) >>> +#define RECORDER_TWEAK_DECLARE(rec) \ >>> + extern const SpiceDummyTweak spice_recorder_tweak_ ## rec >>> +#define RECORDER_TWEAK_DEFINE(rec, value, comment) \ >>> + const SpiceDummyTweak spice_recorder_tweak_ ## rec = { (value) } >>> +#define RECORDER_TWEAK(rec) \ >>> + ((spice_recorder_tweak_ ## rec).tweak_value) >>> +#define RECORD(rec, format, ...) do { \ >>> + if (sizeof((spice_recorder_ ## rec).dummy)) printf(format, >>> ##__VA_ARGS__); \ >>> + } while(0) >>> +#define RECORD_TIMING_BEGIN(rec) \ >>> + do { RECORD(rec, "begin"); >>> +#define RECORD_TIMING_END(rec, op, name, value) \ >>> + RECORD(rec, "end" op name); \ >>> + } while(0) >>> +#define record(...) \ >>> + RECORD(__VA_ARGS__) >>> +static inline void >>> +recorder_dump_on_common_signals(unsigned add, unsigned remove) >>> +{ >>> +} >>> + >>> +#else >>> +#include <common/recorder/recorder.h> >>> +#include <common/recorder/recorder_ring.h> >> >> The second include is unnecessary. Mostly harmless. >> > > I'll remove it. > >>> +#endif >>> diff --git a/configure.ac b/configure.ac >>> index 6e1f5a8..923055b 100644 >>> --- a/configure.ac >>> +++ b/configure.ac >>> @@ -13,7 +13,7 @@ AC_CONFIG_AUX_DIR([build-aux]) >>> m4_ifdef([AM_PROG_AR], [AM_PROG_AR]) >>> >>> # Checks for programs >>> -AM_INIT_AUTOMAKE([1.11 dist-xz no-dist-gzip tar-ustar foreign -Wall >>> -Werror]) >>> +AM_INIT_AUTOMAKE([1.11 dist-xz no-dist-gzip tar-ustar foreign >>> subdir-objects -Wall -Werror]) >>> AM_MAINTAINER_MODE >>> AM_SILENT_RULES([yes]) >>> LT_INIT >>> @@ -26,6 +26,10 @@ if test "x$ac_cv_prog_cc_c99" = xno; then >>> fi >>> AM_PROG_CC_C_O >>> >>> +AC_CHECK_HEADERS([sys/mman.h regex.h]) >>> +AC_CHECK_FUNCS([sigaction drand48]) >>> +AC_SEARCH_LIBS(regcomp, [regex rx]) >>> + >>> SPICE_CHECK_SYSDEPS >>> SPICE_EXTRA_CHECKS >>> >>> @@ -37,6 +41,8 @@ AC_ARG_ENABLE([alignment-checks], >>> AS_IF([test "x$enable_alignment_checks" = "xyes"], >>> [AC_DEFINE([SPICE_DEBUG_ALIGNMENT], 1, [Enable runtime checks for >>> cast alignment])]) >>> >>> +SPICE_CHECK_RECORDER >>> + >>> # Checks for libraries >>> PKG_CHECK_MODULES([PROTOCOL], [spice-protocol >= 0.12.12]) >>> >>> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4 >>> index 1721d34..4bf4eb5 100644 >>> --- a/m4/spice-deps.m4 >>> +++ b/m4/spice-deps.m4 >>> @@ -363,3 +363,18 @@ AC_DEFUN([SPICE_CHECK_SASL], [ >>> AC_DEFUN([SPICE_CHECK_OPENSSL], [ >>> PKG_CHECK_MODULES(OPENSSL, openssl) >>> ]) >>> + >>> +# SPICE_CHECK_RECORDER >>> +# ----------------- >>> +# Check for the availability of recorder library. >>> +#------------------ >>> +AC_DEFUN([SPICE_CHECK_RECORDER], [ >>> + AC_ARG_ENABLE([recorder], >>> + AS_HELP_STRING([--enable-recorder], >>> + [Enable recorder instrumentation >>> @<:@default=no@:>@]), >>> + [], >>> + enable_recorder="no") >>> + AS_IF([test "$enable_recorder" = "yes"], >>> + AC_DEFINE([ENABLE_RECORDER], [1], [Define if recorder >>> instrumentation is enabled])) >>> + AM_CONDITIONAL([ENABLE_RECORDER],[test "$enable_recorder" = "yes"]) >>> +]) >>> diff --git a/meson.build b/meson.build >>> index 049409b..f6ae0f2 100644 >>> --- a/meson.build >>> +++ b/meson.build >>> @@ -34,6 +34,10 @@ if get_option('extra-checks') >>> spice_common_config_data.set('ENABLE_EXTRA_CHECKS', '1') >>> endif >>> >>> +if get_option('recorder') >>> + spice_common_config_data.set('ENABLE_RECORDER', '1') >>> +endif >>> + >>> spice_common_generate_code = get_option('generate-code') >>> spice_common_generate_client_code = spice_common_generate_code == 'all' or >>> spice_common_generate_code == 'client' >>> spice_common_generate_server_code = spice_common_generate_code == 'all' or >>> spice_common_generate_code == 'server' >>> @@ -56,7 +60,9 @@ headers = ['alloca.h', >>> 'sys/socket.h', >>> 'sys/stat.h', >>> 'sys/types.h', >>> - 'unistd.h'] >>> + 'unistd.h', >>> + 'regex.h', >>> + 'sys/mman.h'] >>> >>> foreach header : headers >>> if compiler.has_header(header) >>> @@ -72,7 +78,9 @@ functions = ['alloca', >>> 'floor', >>> 'fork', >>> 'memmove', >>> - 'memset'] >>> + 'memset', >>> + 'sigaction', >>> + 'drand48’] >> >> The drand48 function is only used for the testing code, which you do not >> compile. >> Also mostly harmless. >> >>> >>> foreach func : functions >>> if compiler.has_function(func) >>> diff --git a/meson_options.txt b/meson_options.txt >>> index 1b80257..a90726a 100644 >>> --- a/meson_options.txt >>> +++ b/meson_options.txt >>> @@ -20,6 +20,12 @@ option('opus', >>> yield : true, >>> description: 'Enable Opus audio codec') >>> >>> +option('recorder', >>> + type : 'boolean', >>> + value : false, >>> + yield : true, >>> + description: 'Enable recorder instrumentation') >>> + >>> option('smartcard', >>> type : 'boolean', >>> value : true, >>> diff --git a/tests/Makefile.am b/tests/Makefile.am >>> index 926ac99..690972f 100644 >>> --- a/tests/Makefile.am >>> +++ b/tests/Makefile.am >>> @@ -65,6 +65,18 @@ test_region_LDADD = \ >>> $(top_builddir)/common/libspice-common.la \ >>> $(NULL) >>> >>> +TESTS += test_dummy_recorder >>> + >>> +test_dummy_recorder_SOURCES = \ >>> + test-dummy-recorder.c \ >>> + $(NULL) >>> +test_dummy_recorder_CFLAGS = \ >>> + -I$(top_srcdir) \ >>> + $(NULL) >>> +test_dummy_recorder_LDADD = \ >>> + $(top_builddir)/common/libspice-common.la \ >>> + $(NULL) >>> + >>> # Avoid need for python(pyparsing) by end users >>> TEST_MARSHALLERS = \ >>> generated_test_marshallers.c \ >>> diff --git a/tests/test-dummy-recorder.c b/tests/test-dummy-recorder.c >>> new file mode 100644 >>> index 0000000..4e674a9 >>> --- /dev/null >>> +++ b/tests/test-dummy-recorder.c >>> @@ -0,0 +1,53 @@ >>> +/* >>> + Copyright (C) 2018 Red Hat, Inc. >>> + >>> + This library is free software; you can redistribute it and/or >>> + modify it under the terms of the GNU Lesser General Public >>> + License as published by the Free Software Foundation; either >>> + version 2.1 of the License, or (at your option) any later version. >>> + >>> + This library is distributed in the hope that it will be useful, >>> + but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + Lesser General Public License for more details. >>> + >>> + You should have received a copy of the GNU Lesser General Public >>> + License along with this library; if not, see >>> <http://www.gnu.org/licenses/>. >>> +*/ >>> +/* Test that the macros to replace recorder works correctly */ >>> +#undef NDEBUG >>> +#include <config.h> >>> +#include <assert.h> >>> + >>> +#undef ENABLE_RECORDER >>> + >>> +#include <common/recorder.h> >>> + >>> +RECORDER(rec1, 32, "Desc rec1"); >>> +RECORDER_DECLARE(rec2); >>> + >>> +RECORDER_TWEAK_DECLARE(tweak); >>> + >>> +int main(void) >>> +{ >>> + // this should compile and link >>> + recorder_dump_on_common_signals(0, 0); >>> + >>> + // dummy traces should be always disabled >>> + if (RECORDER_TRACE(rec1)) { >>> + return 1; >>> + } >>> + >>> + // check tweak value >>> + assert(RECORDER_TWEAK(tweak) == 123); >>> + >>> + RECORD(rec2, "aaa %d", 1); >>> + >>> + RECORD_TIMING_BEGIN(rec2); >>> + RECORD_TIMING_END(rec2, "op", "name", 123); >> >> The features provided by RECORD_TIMING_BEGIN / RECORD_TIMING_END >> are now provided by the recorder scope. >> > > Interesting. How? While the scope is running, use the “t” key to show timing info, the “a” key to show a moving average and the “m” to show shrinking min/max boundaries. There is also a “s” key to save to CSV and “i” to save the graph as a picture. There are also command-line options to start with timing on, etc. It’s not exactly the same thing as the above macros, but it reduces the need for them. That being said, I have no plan to remove or change the macros any time soon, I just find them ad-hoc and hard to read. Thanks Christophe >>> + >>> + return 0; >>> +} >>> + >>> +RECORDER_DEFINE(rec2, 64, "Desc rec2"); >>> +RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak"); > > Frediano > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel