[Spice-devel] [PATCH 3/4] stat: use a better design for statistic functions
Jonathon Jongsma
jjongsma at redhat.com
Wed Dec 2 08:43:23 PST 2015
On Wed, 2015-12-02 at 11:43 +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 at redhat.com>
> ---
> server/dcc.c | 31 +++++++++++-------------------
> server/display-channel.c | 47 +++++++++++++++++-----------------------------
> server/display-channel.h | 4 +---
> server/stat.h | 49 +++++++++++++++++++++++++++++++----------------
> -
> 4 files changed, 61 insertions(+), 70 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 4ef5524..f37cb2b 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);
> @@ -1782,9 +1768,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,
> @@ -1794,13 +1781,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 69d287a..733b4b7 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..aa21f2b 100644
> --- a/server/stat.h
> +++ b/server/stat.h
> @@ -53,12 +53,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 +78,50 @@ typedef struct stat_info_s {
> uint64_t orig_size;
> uint64_t comp_size;
> #endif
> +#endif
> } stat_info_t;
>
> +static inline void stat_start_time_init(stat_start_time_t *tm, const
> stat_info_t *info)
do we need to use G_GNUC_UNUSED here to avoid unused variable warnings when
stats are disabled?
> +{
> +#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> + tm->time = stat_now(info->clock);
> +#endif
> +}
> +
> static inline void stat_reset(stat_info_t *info)
and here?
> {
> +#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)
and here?
> {
> +#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,
> + stat_start_time_t start, int orig_size,
> int comp_size)
and here?
> {
> +#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 +129,25 @@ 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
and here?
> 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(stat_info_t *info, stat_start_time_t start)
again?
> {
> +#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 */
I don't really feel strongly about whether this way is better than the previous
way, to be honest. But I'm fine with it as long as we don't add warnings as
mentioned above.
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
More information about the Spice-devel
mailing list