Re: [Qemu-devel] [PATCH 1/2] Rolling statistics utilities

[Dr. David Alan Gilbert]

* Eric Blake (address@hidden) wrote:
> On 02/27/2015 12:06 PM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > There are various places where it's useful to hold a series
> > of values that change over time and get summaries about them.
> > 
> > This provides:
> > 
> >    - a count of the number of items
> >    - min/max
> >    - mean
> >    - a weighted mean (where you can set the weight to determine
> >                       whether it changes quickly or slowly)
> >    - the last 'n' values
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  include/qemu/rolling-stats.h | 101 +++++++++++
> 
> > +
> > +/**
> > + * Return a string representing the RStats data, intended for JSON parsing
> > + *
> > + * Returns: An allocated string the caller must free
> > + *          or NULL on error
> > + *
> > + * e.g.
> > + *    { "min": -3.57, "max": 126.3, "mean": 7.83, "weighted_mean": 8.56,
> > + *      "count": 5678,
> > + *      "values": [ 4.3, 5.8, 1.2, 7.9, 10.3 ],
> > + *      "tags": [ 458, 783, 950, 951, 952 ] }
> 
> Looks useful at first glance.  Maybe s/weighted_mean/weighted-mean/
> since we favor - in new QMP.
> 
> 
> > +
> > +    qemu_mutex_lock(&r->mutex);
> > +    space  = 60 /* for text */ +
> > +             /* 4 double values (min/max/mean/weighted) + the stored
> > +              * values, plus a normal guess for the size of them printed
> > +              * with %g and some padding.  I'm not sure of the worst case.
> > +              */
> > +             (4 + r->allocated) * 13 +
> > +             /* and the count and tags as 64bit ints and some padding */
> > +             (1 + r->allocated) * 23;
> > +    space_left = space - 1;
> > +
> > +    result = g_try_malloc(space);
> > +
> > +    if (!result) {
> > +        qemu_mutex_unlock(&r->mutex);
> > +        return NULL;
> > +    }
> > +
> > +    current = result;
> > +    tmp = snprintf(current, space_left, "Min/Max: %.8g, %.8g Mean: %.8g "
> > +                                        "(Weighted: %.8g) Count: %" PRIu64
> > +                                        " Values: ",
> 
> Eww. Why pre-compute things for a possibly not-wide-enough snprintf,
> when you can instead use glib's g_string_printf that allocates the
> perfect size as it goes?

Ah, because I didn't know about that; useful.

>  For that matter, your cover letter comment
> about putting the struct in QAPI and letting the generated visitor
> automatically produce the JSON might make this simpler than building it
> by hand.

Right; I'd got this far, tried to glue it into the 'info' commands
and realised it needed to return soemthing more QAPI friendly;
but thought it best to see if people liked the general idea before attacking
that.

I'm not sure; but I think the approach would be to have a QAPI
type to hold the same data (Lets say a RollingStats type)
and then have a rstats_copy_to_rolling_stats function
that, under lock, copied the data out, that way the QAPI
type doesn't need to worry about the lock and neither does
anything outside this code.

Dave

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK