On Wed, 2015-12-09 at 15:58 +0000, Frediano Ziglio wrote:
> make sure code compile with statistics enabled or disabled.
> Dummy (empty) structures and functions are used instead of preprocessor.
> Also fix a problem as stat_compress_init did not initialize clock
> field.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> server/dcc.c | 31 ++++++++--------------
> server/display-channel.c | 47 ++++++++++++---------------------
> server/display-channel.h | 4 +--
> server/stat.h | 67 ++++++++++++++++++++++++++++++++---------------
> -
> 4 files changed, 74 insertions(+), 75 deletions(-)
>
> diff --git a/server/dcc.c b/server/dcc.c
> index 78452f4..5d55e5e 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -651,9 +651,8 @@ int dcc_compress_image_glz(DisplayChannelClient *dcc,
> compress_send_data_t* o_comp_data)
> {
> DisplayChannel *display_channel = DCC_TO_DC(dcc);
> -#ifdef COMPRESS_STAT
> - stat_time_t start_time = stat_now(display_channel->glz_stat.clock);
> -#endif
> + stat_start_time_t start_time;
> + stat_start_time_init(&start_time, &display_channel->zlib_glz_stat);
> spice_assert(bitmap_fmt_is_rgb(src->format));
> GlzData *glz_data = &dcc->glz_data;
> ZlibData *zlib_data;
> @@ -687,9 +686,7 @@ int dcc_compress_image_glz(DisplayChannelClient *dcc,
> if (!display_channel->enable_zlib_glz_wrap || (glz_size <
> MIN_GLZ_SIZE_FOR_ZLIB)) {
> goto glz;
> }
> -#ifdef COMPRESS_STAT
> - start_time = stat_now(display_channel->zlib_glz_stat.clock);
> -#endif
> + stat_start_time_init(&start_time, &display_channel->zlib_glz_stat);
> zlib_data = &dcc->zlib_data;
>
> zlib_data->data.bufs_tail = compress_buf_new();
> @@ -741,9 +738,8 @@ int dcc_compress_image_lz(DisplayChannelClient *dcc,
> LzImageType type = bitmap_fmt_to_lz_image_type[src->format];
> int size; // size of the compressed data
>
> -#ifdef COMPRESS_STAT
> - stat_time_t start_time = stat_now(DCC_TO_DC(dcc)->lz_stat.clock);
> -#endif
> + stat_start_time_t start_time;
> + stat_start_time_init(&start_time, &DCC_TO_DC(dcc)->lz_stat);
>
> lz_data->data.bufs_tail = compress_buf_new();
> lz_data->data.bufs_head = lz_data->data.bufs_tail;
> @@ -817,10 +813,9 @@ int dcc_compress_image_jpeg(DisplayChannelClient *dcc,
> SpiceImage *dest,
> int comp_head_left;
> int stride;
> uint8_t *lz_out_start_byte;
> + stat_start_time_t start_time;
> + stat_start_time_init(&start_time, &DCC_TO_DC(dcc)->jpeg_alpha_stat);
>
> -#ifdef COMPRESS_STAT
> - stat_time_t start_time = stat_now(DCC_TO_DC(dcc)->jpeg_stat.clock);
> -#endif
> switch (src->format) {
> case SPICE_BITMAP_FMT_16BIT:
> jpeg_in_type = JPEG_IMAGE_TYPE_RGB16;
> @@ -940,10 +935,8 @@ int dcc_compress_image_lz4(DisplayChannelClient *dcc,
> SpiceImage *dest,
> Lz4Data *lz4_data = &dcc->lz4_data;
> Lz4EncoderContext *lz4 = dcc->lz4;
> int lz4_size = 0;
> -
> -#ifdef COMPRESS_STAT
> - stat_time_t start_time = stat_now(DCC_TO_DC(dcc)->lz4_stat.clock);
> -#endif
> + stat_start_time_t start_time;
> + stat_start_time_init(&start_time, &DCC_TO_DC(dcc)->lz4_stat);
>
> lz4_data->data.bufs_tail = compress_buf_new();
> lz4_data->data.bufs_head = lz4_data->data.bufs_tail;
> @@ -1003,10 +996,8 @@ int dcc_compress_image_quic(DisplayChannelClient *dcc,
> SpiceImage *dest,
> QuicContext *quic = dcc->quic;
> volatile QuicImageType type;
> int size, stride;
> -
> -#ifdef COMPRESS_STAT
> - stat_time_t start_time = stat_now(DCC_TO_DC(dcc)->quic_stat.clock);
> -#endif
> + stat_start_time_t start_time;
> + stat_start_time_init(&start_time, &DCC_TO_DC(dcc)->quic_stat);
>
> switch (src->format) {
> case SPICE_BITMAP_FMT_32BIT:
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 32d87af..1f9a0ed 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -29,26 +29,13 @@ uint32_t display_channel_generate_uid(DisplayChannel
> *display)
> return ++display->bits_unique;
> }
>
> -static stat_time_t display_channel_stat_now(DisplayChannel *display)
> -{
> -#ifdef RED_WORKER_STAT
> - RedWorker *worker = COMMON_CHANNEL(display)->worker;
> -
> - return stat_now(red_worker_get_clockid(worker));
> -
> -#else
> - return 0;
> -#endif
> -}
> -
> -#define stat_start(display, var) \
> - G_GNUC_UNUSED stat_time_t var = display_channel_stat_now((display));
> +#define stat_start(stat, var) \
> + stat_start_time_t var; stat_start_time_init(&var, stat);
>
> void display_channel_compress_stats_reset(DisplayChannel *display)
> {
> spice_return_if_fail(display);
>
> -#ifdef COMPRESS_STAT
> stat_reset(&display->quic_stat);
> stat_reset(&display->lz_stat);
> stat_reset(&display->glz_stat);
> @@ -56,7 +43,6 @@ void display_channel_compress_stats_reset(DisplayChannel
> *display)
> stat_reset(&display->zlib_glz_stat);
> stat_reset(&display->jpeg_alpha_stat);
> stat_reset(&display->lz4_stat);
> -#endif
> }
>
> void display_channel_compress_stats_print(const DisplayChannel
> *display_channel)
> @@ -568,7 +554,7 @@ static void __exclude_region(DisplayChannel *display, Ring
> *ring, TreeItem *item
> Ring **top_ring, Drawable *frame_candidate)
> {
> QRegion and_rgn;
> - stat_start(display, start_time);
> + stat_start(&display->__exclude_stat, start_time);
>
> region_clone(&and_rgn, rgn);
> region_and(&and_rgn, &item->rgn);
> @@ -637,7 +623,7 @@ static void exclude_region(DisplayChannel *display, Ring
> *ring, RingItem *ring_i
> QRegion *rgn, TreeItem **last, Drawable
> *frame_candidate)
> {
> Ring *top_ring;
> - stat_start(display, start_time);
> + stat_start(&display->exclude_stat, start_time);
>
> if (!ring_item) {
> return;
> @@ -692,7 +678,7 @@ static void exclude_region(DisplayChannel *display, Ring
> *ring, RingItem *ring_i
>
> static int current_add_with_shadow(DisplayChannel *display, Ring *ring,
> Drawable *item)
> {
> - stat_start(display, start_time);
> + stat_start(&display->add_stat, start_time);
> #ifdef RED_WORKER_STAT
> ++display->add_with_shadow_count;
> #endif
> @@ -739,7 +725,7 @@ static int current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
> RingItem *now;
> QRegion exclude_rgn;
> RingItem *exclude_base = NULL;
> - stat_start(display, start_time);
> + stat_start(&display->add_stat, start_time);
>
> spice_assert(!region_is_empty(&item->base.rgn));
> region_init(&exclude_rgn);
> @@ -2051,9 +2037,10 @@ DisplayChannel* display_channel_new(RedWorker *worker,
> int migrate, int stream_v
> &cbs, dcc_handle_message);
> spice_return_val_if_fail(display, NULL);
>
> - stat_init(&display->add_stat, "add", red_worker_get_clockid(worker));
> - stat_init(&display->exclude_stat, "exclude",
> red_worker_get_clockid(worker));
> - stat_init(&display->__exclude_stat, "__exclude",
> red_worker_get_clockid(worker));
> + clockid_t stat_clock = red_worker_get_clockid(worker);
> + stat_init(&display->add_stat, "add", stat_clock);
> + stat_init(&display->exclude_stat, "exclude", stat_clock);
> + stat_init(&display->__exclude_stat, "__exclude", stat_clock);
> #ifdef RED_STATISTICS
> RedChannel *channel = RED_CHANNEL(display);
> display->cache_hits_counter = stat_add_counter(channel->stat,
> @@ -2063,13 +2050,13 @@ DisplayChannel* display_channel_new(RedWorker *worker,
> int migrate, int stream_v
> display->non_cache_counter = stat_add_counter(channel->stat,
> "non_cache", TRUE);
> #endif
> - stat_compress_init(&display->lz_stat, "lz",
> red_worker_get_clockid(worker));
> - stat_compress_init(&display->glz_stat, "glz",
> red_worker_get_clockid(worker));
> - stat_compress_init(&display->quic_stat, "quic",
> red_worker_get_clockid(worker));
> - stat_compress_init(&display->jpeg_stat, "jpeg",
> red_worker_get_clockid(worker));
> - stat_compress_init(&display->zlib_glz_stat, "zlib",
> red_worker_get_clockid(worker));
> - stat_compress_init(&display->jpeg_alpha_stat, "jpeg_alpha",
> red_worker_get_clockid(worker));
> - stat_compress_init(&display->lz4_stat, "lz4",
> red_worker_get_clockid(worker));
> + stat_compress_init(&display->lz_stat, "lz", stat_clock);
> + stat_compress_init(&display->glz_stat, "glz", stat_clock);
> + stat_compress_init(&display->quic_stat, "quic", stat_clock);
> + stat_compress_init(&display->jpeg_stat, "jpeg", stat_clock);
> + stat_compress_init(&display->zlib_glz_stat, "zlib", stat_clock);
> + stat_compress_init(&display->jpeg_alpha_stat, "jpeg_alpha", stat_clock);
> + stat_compress_init(&display->lz4_stat, "lz4", stat_clock);
>
> display->n_surfaces = n_surfaces;
> display->num_renderers = num_renderers;
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 7cbc58d..e698788 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -201,10 +201,10 @@ struct DisplayChannel {
> RedCompressBuf *free_compress_bufs;
>
> /* TODO: some day unify this, make it more runtime.. */
> -#ifdef RED_WORKER_STAT
> stat_info_t add_stat;
> stat_info_t exclude_stat;
> stat_info_t __exclude_stat;
> +#ifdef RED_WORKER_STAT
> uint32_t add_count;
> uint32_t add_with_shadow_count;
> #endif
> @@ -213,7 +213,6 @@ struct DisplayChannel {
> uint64_t *add_to_cache_counter;
> uint64_t *non_cache_counter;
> #endif
> -#ifdef COMPRESS_STAT
> stat_info_t lz_stat;
> stat_info_t glz_stat;
> stat_info_t quic_stat;
> @@ -221,7 +220,6 @@ struct DisplayChannel {
> stat_info_t zlib_glz_stat;
> stat_info_t jpeg_alpha_stat;
> stat_info_t lz4_stat;
> -#endif
> };
>
> #define LINK_TO_DCC(ptr) SPICE_CONTAINEROF(ptr, DisplayChannelClient, \
> diff --git a/server/stat.h b/server/stat.h
> index 5e8fc32..adbf46b 100644
> --- a/server/stat.h
> +++ b/server/stat.h
> @@ -19,6 +19,7 @@
> #define _H_STAT
>
> #include <stdint.h>
> +#include <glib.h>
>
> typedef uint32_t StatNodeRef;
> #define INVALID_STAT_REF (~(StatNodeRef)0)
> @@ -53,12 +54,21 @@ static inline stat_time_t stat_now(clockid_t clock_id)
> return ts.tv_nsec + (uint64_t) ts.tv_sec * (1000 * 1000 * 1000);
> }
>
> +typedef struct {
> +#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> + stat_time_t time;
> +#endif
> +} stat_start_time_t;
> +
> +#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> static inline double stat_cpu_time_to_sec(stat_time_t time)
> {
> return (double)time / (1000 * 1000 * 1000);
> }
> +#endif
>
> -typedef struct stat_info_s {
> +typedef struct {
> +#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> const char *name;
> clockid_t clock;
> uint32_t count;
> @@ -69,37 +79,54 @@ typedef struct stat_info_s {
> uint64_t orig_size;
> uint64_t comp_size;
> #endif
> +#endif
> } stat_info_t;
>
> -static inline void stat_reset(stat_info_t *info)
> +static inline void stat_start_time_init(G_GNUC_UNUSED stat_start_time_t *tm,
> + G_GNUC_UNUSED const stat_info_t
> *info)
> {
> +#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> + tm->time = stat_now(info->clock);
> +#endif
> +}
> +
> +static inline void stat_reset(G_GNUC_UNUSED stat_info_t *info)
> +{
> +#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> info->count = info->max = info->total = 0;
> info->min = ~(stat_time_t)0;
> #ifdef COMPRESS_STAT
> info->orig_size = info->comp_size = 0;
> #endif
> +#endif
> }
>
> -#ifdef COMPRESS_STAT
> -static inline void stat_compress_init(stat_info_t *info, const char *name,
> clockid_t clock)
> +static inline void stat_compress_init(G_GNUC_UNUSED stat_info_t *info,
> + G_GNUC_UNUSED const char *name,
> + G_GNUC_UNUSED clockid_t clock)
> {
> +#ifdef COMPRESS_STAT
> info->name = name;
> info->clock = clock;
> stat_reset(info);
> +#endif
> }
>
> -static inline void stat_compress_add(stat_info_t *info,
> - stat_time_t start, int orig_size,
> - int comp_size)
> +static inline void stat_compress_add(G_GNUC_UNUSED stat_info_t *info,
> + G_GNUC_UNUSED stat_start_time_t start,
> + G_GNUC_UNUSED int orig_size,
> + G_GNUC_UNUSED int comp_size)
> {
> +#ifdef COMPRESS_STAT
> stat_time_t time;
> ++info->count;
> - time = stat_now(info->clock) - start;
> + time = stat_now(info->clock) - start.time;
> info->total += time;
> info->max = MAX(info->max, time);
> info->min = MIN(info->min, time);
> info->orig_size += orig_size;
> info->comp_size += comp_size;
> +#endif
> }
>
> static inline double stat_byte_to_mega(uint64_t size)
> @@ -107,32 +134,28 @@ static inline double stat_byte_to_mega(uint64_t size)
> return (double)size / (1000 * 1000);
> }
>
> -#else
> -#define stat_compress_init(a, b, c)
> -#define stat_compress_add(a, b, c, d)
> -#endif
> -
> -#ifdef RED_WORKER_STAT
> -static inline void stat_init(stat_info_t *info, const char *name, clockid_t
> clock)
> +static inline void stat_init(G_GNUC_UNUSED stat_info_t *info,
> + G_GNUC_UNUSED const char *name,
> + G_GNUC_UNUSED clockid_t clock)
> {
> +#ifdef RED_WORKER_STAT
> info->name = name;
> info->clock = clock;
> stat_reset(info);
> +#endif
> }
>
> -static inline void stat_add(stat_info_t *info, stat_time_t start)
> +static inline void stat_add(G_GNUC_UNUSED stat_info_t *info,
> + G_GNUC_UNUSED stat_start_time_t start)
> {
> +#ifdef RED_WORKER_STAT
> stat_time_t time;
> ++info->count;
> - time = stat_now(info->clock) - start;
> + time = stat_now(info->clock) - start.time;
> info->total += time;
> info->max = MAX(info->max, time);
> info->min = MIN(info->min, time);
> +#endif
> }
>
> -#else
> -#define stat_add(a, b)
> -#define stat_init(a, b, c)
> -#endif /* RED_WORKER_STAT */
> -
> #endif /* _H_STAT */
Looks good.
Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel