Re: [Qemu-devel] [PATCH] [qemu-img] CPU consuming optimization

[Kevin Wolf <kwolf@redhat.com>]

Am 18.05.2011 11:18, schrieb Dmitry Konishchev:
> On 18.05.2011 11:57, Stefan Hajnoczi wrote:
>> Yes, optimizing is_not_zero() is good.  The only additional thing I
>> suggest is adding a comment before the function to document the length
>> constraint.
> 
> OK, fixed.
> 
> 
> On 18.05.2011 12:05, Kevin Wolf wrote:
>> A future bdrv_is_allocated() patch must make sure that the conversion
>> falls back to a simple is_not_zero() when a backing file is used.
> 
> Thanks, I'll take this into account.
> 
> 
> Signed-off-by: Dmitry Konishchev <konishchev@gmail.com>
> ---
>   qemu-img.c |   30 +++++++++++++++++++++++++++---
>   1 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e825123..7665c2f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -496,14 +496,38 @@ static int img_commit(int argc, char **argv)
>       return 0;
>   }
> 
> +/*
> + * Checks whether the sector is not a zero sector.
> + *
> + * Attention! The len must be a multiple of 4 * sizeof(long) due to
> + * restriction of optimizations in this function.
> + */
>   static int is_not_zero(const uint8_t *sector, int len)
>   {
> +    /*
> +     * Use long as the biggest available internal data type that fits 
> into the
> +     * CPU register and unroll the loop to smooth out the effect of memory
> +     * latency.
> +     */
> +
>       int i;
> -    len >>= 2;
> -    for(i = 0;i < len; i++) {
> -        if (((uint32_t *)sector)[i] != 0)
> +    len /= sizeof(long);
> +
> +    long d0;
> +    long d1;
> +    long d2;
> +    long d3;

Please move the declarations to the start of the function.

I also would use a single line like "long d0, d1, d2, d3;", but that's
up to you.

> +
> +    for(i = 0; i < len; i += 4) {
> +        d0 = ((const long*) sector)[i + 0];
> +        d1 = ((const long*) sector)[i + 1];
> +        d2 = ((const long*) sector)[i + 2];
> +        d3 = ((const long*) sector)[i + 3];

I would suggest to declare a const long* variable so that you don't have
to cast each time you use, but that's probably a matter of taste.

> +
> +        if (d0 || d1 || d2 || d3)
>               return 1;

Coding style requires braces here.

>       }
> +
>       return 0;
>   }

Please make sure that your patch isn't line-wrapped when you send it for
inclusion. git send-email will do the right thing.

Kevin