change '%' to condition

change '%' to condition

[Mikulas Patocka]

Hi

I think you should revert the patch 
4c0ca26bd260dddf3b9781758cb5e2df3f74d4a3 that did this change:

                for (f = 1; f < geo->far_copies; f++) {
                        d += geo->near_copies;
-                       if (d >= geo->raid_disks)
-                               d -= geo->raid_disks;
+                       d %= geo->raid_disks;
                        s += geo->stride;
                        r10bio->devs[slot].devnum = d;
                        r10bio->devs[slot].addr = s;


On most processors, the divide and modulo operations are slower than a 
possibly misprediteced branch or conditional move instruction.

So, replacing a condition with modulo doesn't make sense.

A benchmark on AMD K10 shows that mispredicted branch is 8.7 times faster 
than a divide operation:

        for (i = 0; i < 10000000; i++) {
                q++;
                if (q >= 8)
                        q -= 8;
        }
- 11607us (it compiles to cmov and runs at a rate of 3 ticks per 
iteration)

        for (i = 0; i < 10000000; i++) {
                q++;
                q %= max;
        }
- 101241us (26 ticks per iteration)


Mikulas

Re: change '%' to condition

[Mike Snitzer]

On Wed, Sep 11 2013 at  7:18pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Hi
> 
> I think you should revert the patch 
> 4c0ca26bd260dddf3b9781758cb5e2df3f74d4a3 that did this change:
> 
>                 for (f = 1; f < geo->far_copies; f++) {
>                         d += geo->near_copies;
> -                       if (d >= geo->raid_disks)
> -                               d -= geo->raid_disks;
> +                       d %= geo->raid_disks;
>                         s += geo->stride;
>                         r10bio->devs[slot].devnum = d;
>                         r10bio->devs[slot].addr = s;
> 
> 
> On most processors, the divide and modulo operations are slower than a 
> possibly misprediteced branch or conditional move instruction.
> 
> So, replacing a condition with modulo doesn't make sense.
> 
> A benchmark on AMD K10 shows that mispredicted branch is 8.7 times faster 
> than a divide operation:
> 
>         for (i = 0; i < 10000000; i++) {
>                 q++;
>                 if (q >= 8)
>                         q -= 8;
>         }
> - 11607us (it compiles to cmov and runs at a rate of 3 ticks per 
> iteration)
> 
>         for (i = 0; i < 10000000; i++) {
>                 q++;
>                 q %= max;
>         }
> - 101241us (26 ticks per iteration)

Patch 68 in this series goes on to introduce more use of %= (upstream
commit 9a3152ab024867100f2f50d124b998d05fb1c3f6).

But regardless Jes' original point still stands: if this should be
changed it needs to be done upstream first.

Mike

Re: change '%' to condition

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On Wed, 11 Sep 2013 19:18:57 -0400 (EDT) Mikulas Patocka
<mpatocka@redhat.com> wrote:

> Hi
> 
> I think you should revert the patch 
> 4c0ca26bd260dddf3b9781758cb5e2df3f74d4a3 that did this change:
> 
>                 for (f = 1; f < geo->far_copies; f++) {
>                         d += geo->near_copies;
> -                       if (d >= geo->raid_disks)
> -                               d -= geo->raid_disks;
> +                       d %= geo->raid_disks;
>                         s += geo->stride;
>                         r10bio->devs[slot].devnum = d;
>                         r10bio->devs[slot].addr = s;
> 
> 
> On most processors, the divide and modulo operations are slower than a 
> possibly misprediteced branch or conditional move instruction.
> 
> So, replacing a condition with modulo doesn't make sense.
> 
> A benchmark on AMD K10 shows that mispredicted branch is 8.7 times faster 
> than a divide operation:
> 
>         for (i = 0; i < 10000000; i++) {
>                 q++;
>                 if (q >= 8)
>                         q -= 8;
>         }
> - 11607us (it compiles to cmov and runs at a rate of 3 ticks per 
> iteration)
> 
>         for (i = 0; i < 10000000; i++) {
>                 q++;
>                 q %= max;
>         }
> - 101241us (26 ticks per iteration)
> 
> 
> Mikulas

I can't seem to get excited about this sort of micro optimisation.

If someone were to send me a nice patch with a good description and a couple
of acked-by:s from relevant people I would probably apply it.  But otherwise
I just wouldn't care.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]