[Spice-devel] [PATCH spice-server 1/3] Improve statistic code interface
Jonathon Jongsma
jjongsma at redhat.com
Tue Feb 14 21:02:36 UTC 2017
On Tue, 2017-02-14 at 15:51 +0000, Frediano Ziglio wrote:
> Use new structures and functions to implement statistic code.
> Instead of using macro use inline functions.
> Inline functions are more type safe.
> If statistics are disabled structure and functions became empty;
> this allow the code to work and compile with either statistic
> disabled or enabled not requiring extra compilation to understand
> if the code will still work if these options are enabled.
> Also this way reduce the preprocessor usage.
> The reds option were removed from stat_inc_counter as not used
> (if parameter were used code wouldn't compile).
It doesn't seem like a huge improvement to me, but it seems fine. I'd
like to suggest a bit of a re-wording of the commit log though. For
example:
"Use new structures and functions to implement the statistics code.
Use inline functions instead of macros for increased type-safety.
If statistics are disabled, the structures and functions become
empty. This confines the configuration-specific #defines to the
statistics implementation itself and avoids the need for #defines in
the calling functions. This greatly reduces the chance of accidentally
breaking the build for one configuration or the other. The reds option
was removed from stat_inc_counter() as it was not used."
> (if parameter were used code wouldn't compile).
I don't quite understand this part...
Acked-by: Jonathon Jongsma <jjongsma at redhat.com
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/dcc-send.c | 6 ++--
> server/display-channel-private.h | 8 ++---
> server/display-channel.c | 18 +++++------
> server/red-channel.c | 27 +++++++----------
> server/red-channel.h | 4 +--
> server/red-worker.c | 24 +++++++--------
> server/reds.c | 28 ++++++++++++-----
> server/stat.h | 65
> ++++++++++++++++++++++++++++++----------
> 8 files changed, 105 insertions(+), 75 deletions(-)
>
> Ping.
>
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 510dfe0..b9244a6 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -197,13 +197,13 @@ static void
> red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> io_image->descriptor.flags |=
> SPICE_IMAGE_FLAGS_CACHE_ME;
> dcc->priv->send_data.pixmap_cache_items[dcc->priv-
> >send_data.num_pixmap_cache_items++] =
>
> image->descriptor.id;
> - stat_inc_counter(reds, display_channel->priv-
> >add_to_cache_counter, 1);
> + stat_inc_counter(display_channel->priv-
> >add_to_cache_counter, 1);
> }
> }
> }
>
> if (!(io_image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME))
> {
> - stat_inc_counter(reds, display_channel->priv-
> >non_cache_counter, 1);
> + stat_inc_counter(display_channel->priv->non_cache_counter,
> 1);
> }
> }
>
> @@ -393,7 +393,7 @@ static FillBitsType
> fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
> &bitmap_palette_out,
> &lzplt_palette_out);
> spice_assert(bitmap_palette_out == NULL);
> spice_assert(lzplt_palette_out == NULL);
> - stat_inc_counter(reds, display->priv-
> >cache_hits_counter, 1);
> + stat_inc_counter(display->priv->cache_hits_counter,
> 1);
> pthread_mutex_unlock(&dcc->priv->pixmap_cache-
> >lock);
> return FILL_BITS_TYPE_CACHE;
> } else {
> diff --git a/server/display-channel-private.h b/server/display-
> channel-private.h
> index a727ddb..da807d1 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -77,11 +77,9 @@ struct DisplayChannelPrivate
> uint32_t add_count;
> uint32_t add_with_shadow_count;
> #endif
> -#ifdef RED_STATISTICS
> - uint64_t *cache_hits_counter;
> - uint64_t *add_to_cache_counter;
> - uint64_t *non_cache_counter;
> -#endif
> + RedStatCounter cache_hits_counter;
> + RedStatCounter add_to_cache_counter;
> + RedStatCounter non_cache_counter;
> ImageEncoderSharedData encoder_shared_data;
> };
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 7d3c6e4..c455462 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2043,18 +2043,14 @@ display_channel_constructed(GObject *object)
> stat_init(&self->priv->add_stat, "add",
> CLOCK_THREAD_CPUTIME_ID);
> stat_init(&self->priv->exclude_stat, "exclude",
> CLOCK_THREAD_CPUTIME_ID);
> stat_init(&self->priv->__exclude_stat, "__exclude",
> CLOCK_THREAD_CPUTIME_ID);
> -#ifdef RED_STATISTICS
> RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> - self->priv->cache_hits_counter =
> - stat_add_counter(reds, red_channel_get_stat_node(channel),
> - "cache_hits", TRUE);
> - self->priv->add_to_cache_counter =
> - stat_add_counter(reds, red_channel_get_stat_node(channel),
> - "add_to_cache", TRUE);
> - self->priv->non_cache_counter =
> - stat_add_counter(reds, red_channel_get_stat_node(channel),
> - "non_cache", TRUE);
> -#endif
> + const RedStatNode *stat = red_channel_get_stat_node(channel);
> + stat_init_counter(&self->priv->cache_hits_counter, reds, stat,
> + "cache_hits", TRUE);
> + stat_init_counter(&self->priv->add_to_cache_counter, reds, stat,
> + "add_to_cache", TRUE);
> + stat_init_counter(&self->priv->non_cache_counter, reds, stat,
> + "non_cache", TRUE);
> image_cache_init(&self->priv->image_cache);
> self->priv->stream_video = SPICE_STREAM_VIDEO_OFF;
> display_channel_init_streams(self);
> diff --git a/server/red-channel.c b/server/red-channel.c
> index f2a35f3..e70c46b 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -101,10 +101,8 @@ struct RedChannelPrivate
> // from Channel -> need to protect!
> pthread_t thread_id;
> RedsState *reds;
> -#ifdef RED_STATISTICS
> - StatNodeRef stat;
> - uint64_t *out_bytes_counter;
> -#endif
> + RedStatNode stat;
> + RedStatCounter out_bytes_counter;
> };
>
> enum {
> @@ -212,7 +210,7 @@ static void red_channel_on_output(void *opaque,
> int n)
> red_channel_client_on_output(opaque, n);
> #ifdef RED_STATISTICS
> self = red_channel_client_get_channel((RedChannelClient
> *)opaque);
> - stat_inc_counter(self->priv->reds, self->priv-
> >out_bytes_counter, n);
> + stat_inc_counter(self->priv->out_bytes_counter, n);
> #endif
> }
>
> @@ -408,24 +406,19 @@ int
> red_channel_is_waiting_for_migrate_data(RedChannel *channel)
> return red_channel_client_is_waiting_for_migrate_data(rcc);
> }
>
> -void red_channel_set_stat_node(RedChannel *channel, StatNodeRef
> stat)
> +void red_channel_init_stat_node(RedChannel *channel, const
> RedStatNode *parent, const char *name)
> {
> spice_return_if_fail(channel != NULL);
> -#ifdef RED_STATISTICS
> - spice_return_if_fail(channel->priv->stat == 0);
>
> - channel->priv->stat = stat;
> - channel->priv->out_bytes_counter =
> - stat_add_counter(channel->priv->reds, stat, "out_bytes",
> TRUE);
> -#endif
> + // TODO check not already initialized
> + stat_init_node(&channel->priv->stat, channel->priv->reds,
> parent, name, TRUE);
> + stat_init_counter(&channel->priv->out_bytes_counter,
> + channel->priv->reds, &channel->priv->stat,
> "out_bytes", TRUE);
> }
>
> -StatNodeRef red_channel_get_stat_node(RedChannel *channel)
> +const RedStatNode *red_channel_get_stat_node(RedChannel *channel)
> {
> -#ifdef RED_STATISTICS
> - return channel->priv->stat;
> -#endif
> - return 0;
> + return &channel->priv->stat;
> }
>
> void red_channel_register_client_cbs(RedChannel *channel, const
> ClientCbs *client_cbs,
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 4430d0b..896a7c5 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -205,7 +205,7 @@ GType red_channel_get_type(void) G_GNUC_CONST;
> void red_channel_add_client(RedChannel *channel, RedChannelClient
> *rcc);
> void red_channel_remove_client(RedChannel *channel, RedChannelClient
> *rcc);
>
> -void red_channel_set_stat_node(RedChannel *channel, StatNodeRef
> stat);
> +void red_channel_init_stat_node(RedChannel *channel, const
> RedStatNode *parent, const char *name);
>
> void red_channel_register_client_cbs(RedChannel *channel, const
> ClientCbs *client_cbs, gpointer cbs_data);
> // caps are freed when the channel is destroyed
> @@ -299,7 +299,7 @@ int red_channel_config_socket(RedChannel *self,
> RedChannelClient *rcc);
> void red_channel_on_disconnect(RedChannel *self, RedChannelClient
> *rcc);
> void red_channel_send_item(RedChannel *self, RedChannelClient *rcc,
> RedPipeItem *item);
> void red_channel_reset_thread_id(RedChannel *self);
> -StatNodeRef red_channel_get_stat_node(RedChannel *channel);
> +const RedStatNode *red_channel_get_stat_node(RedChannel *channel);
>
> /* FIXME: do these even need to be in RedChannel? It's really only
> used in
> * RedChannelClient. Needs refactoring */
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 475acc4..e5adbaa 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -76,11 +76,9 @@ struct RedWorker {
> spice_wan_compression_t zlib_glz_state;
>
> uint32_t process_display_generation;
> -#ifdef RED_STATISTICS
> - StatNodeRef stat;
> - uint64_t *wakeup_counter;
> - uint64_t *command_counter;
> -#endif
> + RedStatNode stat;
> + RedStatCounter wakeup_counter;
> + RedStatCounter command_counter;
>
> int driver_cap_monitors_config;
>
> @@ -213,7 +211,7 @@ static int red_process_display(RedWorker *worker,
> int *ring_is_empty)
> red_record_qxl_command(worker->record, &worker-
> >mem_slots, ext_cmd);
> }
>
> - stat_inc_counter(reds, worker->command_counter, 1);
> + stat_inc_counter(worker->command_counter, 1);
> worker->display_poll_tries = 0;
> switch (ext_cmd.cmd.type) {
> case QXL_CMD_DRAW: {
> @@ -662,7 +660,7 @@ static void handle_dev_wakeup(void *opaque, void
> *payload)
> {
> RedWorker *worker = opaque;
>
> - stat_inc_counter(reds, worker->wakeup_counter, 1);
> + stat_inc_counter(worker->wakeup_counter, 1);
> red_qxl_clear_pending(worker->qxl->st,
> RED_DISPATCHER_PENDING_WAKEUP);
> }
>
> @@ -1341,13 +1339,11 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> worker->jpeg_state = reds_get_jpeg_state(reds);
> worker->zlib_glz_state = reds_get_zlib_glz_state(reds);
> worker->driver_cap_monitors_config = 0;
> -#ifdef RED_STATISTICS
> char worker_str[20];
> sprintf(worker_str, "display[%d]", worker->qxl->id);
> - worker->stat = stat_add_node(reds, INVALID_STAT_REF, worker_str,
> TRUE);
> - worker->wakeup_counter = stat_add_counter(reds, worker->stat,
> "wakeups", TRUE);
> - worker->command_counter = stat_add_counter(reds, worker->stat,
> "commands", TRUE);
> -#endif
> + stat_init_node(&worker->stat, reds, NULL, worker_str, TRUE);
> + stat_init_counter(&worker->wakeup_counter, reds, &worker->stat,
> "wakeups", TRUE);
> + stat_init_counter(&worker->command_counter, reds, &worker->stat,
> "commands", TRUE);
>
> worker->dispatch_watch =
> worker->core.watch_add(&worker->core,
> dispatcher_get_recv_fd(dispatcher),
> @@ -1371,7 +1367,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> worker->cursor_channel = cursor_channel_new(reds, qxl,
> &worker->core);
> channel = RED_CHANNEL(worker->cursor_channel);
> - red_channel_set_stat_node(channel, stat_add_node(reds, worker-
> >stat, "cursor_channel", TRUE));
> + red_channel_init_stat_node(channel, &worker->stat,
> "cursor_channel");
> red_channel_register_client_cbs(channel, client_cursor_cbs,
> dispatcher);
> g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
> reds_register_channel(reds, channel);
> @@ -1382,7 +1378,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> reds_get_video_cod
> ecs(reds),
> init_info.n_surfac
> es);
> channel = RED_CHANNEL(worker->display_channel);
> - red_channel_set_stat_node(channel, stat_add_node(reds, worker-
> >stat, "display_channel", TRUE));
> + red_channel_init_stat_node(channel, &worker->stat,
> "display_channel");
> red_channel_register_client_cbs(channel, client_display_cbs,
> dispatcher);
> g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
> reds_register_channel(reds, channel);
> diff --git a/server/reds.c b/server/reds.c
> index 953a95a..39a7a31 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -352,25 +352,37 @@ static void reds_link_free(RedLinkInfo *link)
>
> #ifdef RED_STATISTICS
>
> -StatNodeRef stat_add_node(RedsState *reds, StatNodeRef parent, const
> char *name, int visible)
> +void stat_init_node(RedStatNode *node, SpiceServer *reds, const
> RedStatNode *parent,
> + const char *name, int visible)
> {
> - return stat_file_add_node(reds->stat_file, parent, name,
> visible);
> + StatNodeRef parent_ref = parent ? parent->ref :
> INVALID_STAT_REF;
> + node->ref = stat_file_add_node(reds->stat_file, parent_ref,
> name, visible);
> }
>
> -void stat_remove_node(RedsState *reds, StatNodeRef ref)
> +void stat_remove_node(SpiceServer *reds, RedStatNode *node)
> {
> - stat_file_remove_node(reds->stat_file, ref);
> + if (node->ref != INVALID_STAT_REF) {
> + stat_file_remove_node(reds->stat_file, node->ref);
> + node->ref = INVALID_STAT_REF;
> + }
> }
>
> -uint64_t *stat_add_counter(RedsState *reds, StatNodeRef parent,
> const char *name, int visible)
> +void stat_init_counter(RedStatCounter *counter, SpiceServer *reds,
> + const RedStatNode *parent, const char *name,
> int visible)
> {
> - return stat_file_add_counter(reds->stat_file, parent, name,
> visible);
> + StatNodeRef parent_ref = parent ? parent->ref :
> INVALID_STAT_REF;
> + counter->counter =
> + stat_file_add_counter(reds->stat_file, parent_ref, name,
> visible);
> }
>
> -void stat_remove_counter(RedsState *reds, uint64_t *counter)
> +void stat_remove_counter(SpiceServer *reds, RedStatCounter *counter)
> {
> - stat_file_remove_counter(reds->stat_file, counter);
> + if (counter->counter) {
> + stat_file_remove_counter(reds->stat_file, counter->counter);
> + counter->counter = NULL;
> + }
> }
> +
> #endif
>
> void reds_register_channel(RedsState *reds, RedChannel *channel)
> diff --git a/server/stat.h b/server/stat.h
> index a5df7ea..5255efa 100644
> --- a/server/stat.h
> +++ b/server/stat.h
> @@ -24,26 +24,61 @@
> #include "spice.h"
> #include "stat-file.h"
>
> +typedef struct {
> #ifdef RED_STATISTICS
> -StatNodeRef stat_add_node(SpiceServer *reds, StatNodeRef parent,
> const char *name, int visible);
> -void stat_remove_node(SpiceServer *reds, StatNodeRef node);
> -uint64_t *stat_add_counter(SpiceServer *reds, StatNodeRef parent,
> const char *name, int visible);
> -void stat_remove_counter(SpiceServer *reds, uint64_t *counter);
> -
> -#define stat_inc_counter(reds, counter, value) { \
> - if (counter) { \
> - *(counter) += (value); \
> - } \
> -}
> + uint64_t *counter;
> +#endif
> +} RedStatCounter;
> +
> +typedef struct {
> +#ifdef RED_STATISTICS
> + uint32_t ref;
> +#endif
> +} RedStatNode;
> +
> +#ifdef RED_STATISTICS
> +void stat_init_node(RedStatNode *node, SpiceServer *reds,
> + const RedStatNode *parent, const char *name, int
> visible);
> +void stat_remove_node(SpiceServer *reds, RedStatNode *node);
> +void stat_init_counter(RedStatCounter *counter, SpiceServer *reds,
> + const RedStatNode *parent, const char *name,
> int visible);
> +void stat_remove_counter(SpiceServer *reds, RedStatCounter
> *counter);
>
> #else
> -#define stat_add_node(r, p, n, v) INVALID_STAT_REF
> -#define stat_remove_node(r, n)
> -#define stat_add_counter(r, p, n, v) NULL
> -#define stat_remove_counter(r, c)
> -#define stat_inc_counter(r, c, v)
> +
> +static inline void
> +stat_init_node(RedStatNode *node, SpiceServer *reds,
> + const RedStatNode *parent, const char *name, int
> visible)
> +{
> +}
> +
> +static inline void
> +stat_remove_node(SpiceServer *reds, RedStatNode *node)
> +{
> +}
> +
> +static inline void
> +stat_init_counter(RedStatCounter *counter, SpiceServer *reds,
> + const RedStatNode *parent, const char *name, int
> visible)
> +{
> +}
> +
> +static inline void
> +stat_remove_counter(SpiceServer *reds, RedStatCounter *counter)
> +{
> +}
> #endif /* RED_STATISTICS */
>
> +static inline void
> +stat_inc_counter(RedStatCounter counter, uint64_t value)
> +{
> +#ifdef RED_STATISTICS
> + if (counter.counter) {
> + *(counter.counter) += value;
> + }
> +#endif
> +}
> +
> typedef uint64_t stat_time_t;
>
> static inline stat_time_t stat_now(clockid_t clock_id)
More information about the Spice-devel
mailing list