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

[Eric Blake]

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?  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.

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

signature.asc
Description: OpenPGP digital signature