[Spice-devel] [PATCH 3/4] stat: use a better design for statistic functions
Frediano Ziglio
fziglio at redhat.com
Wed Dec 2 09:57:28 PST 2015
>
> On Wed, 2015-12-02 at 12:04 -0500, Frediano Ziglio wrote:
> > >
> > > 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?
> > >
> >
> > No, arguments have no such requirement in C.
> > This to support callbacks that does not use parameters.
> > However it's not of any harm to add them.
>
>
> Yes, but we often compile with -Wunused-variable. In fact I think that's
> added
> by default in this project?
>
Yes, and don't produce any warning. Arguments in this case do not produce
warnings. Could be that there is another -Wno-unused-parameter but
-Wno-unused-variable does not enable warnings on arguments.
>
> >
> > > > +{
> > > > +#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>
> > >
> >
> > Main idea is to avoid preprocessor stuff. Code is always compiled, just
> > compiler
> > produce no code if disabled.
> > This avoid problems for instance of not used variable with processor code
> > turned on/off.
> > The test instead is written to exercise all settings combinations.
> >
> > Frediano
>
>
>
> I just noticed something else. Can we have stat_compress_add() simply call
> stat_add() and then doing its additional work instead of duplicating this
> code?
>
> Jonathon
>
Actually the two functions are controlled by two preprocessor macros.
Don't know if this make a lot of sense at the end and perhaps we should
just use one macro. This would make things easier.
Frediano
More information about the Spice-devel
mailing list