[Spice-devel] [PATCH spice-common] Integrate recorder library
Christophe de Dinechin
dinechin at redhat.com
Tue Nov 20 13:12:00 UTC 2018
> On 20 Nov 2018, at 13:49, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>
>> Thanks Frediano,
>>
>>
>> Reviewed-by: Christophe de Dinechin <dinechin at redhat.com>
>>
>>
>>> On 19 Nov 2018, at 21:46, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>
>>> From: Christophe de Dinechin <dinechin at redhat.com>
>>
>> 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 at redhat.com>
>>> ---
>>> .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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list