basepi / libgit2

The Library
http://libgit2.github.com
Other
0 stars 0 forks source link

Decide on bitshift vs division/mutiplication by 2 #17

Closed hausdorf closed 13 years ago

hausdorf commented 13 years ago

Recently we changed some of the places where we divide or multiply by a power of 2 to use bitshifts. My impression was that this was implemented automatically by the compiler.

We need to decide which to use.

hausdorf commented 13 years ago
int a = 1;
int main(void)
{
       int x = a * 8;
}

compiled with gcc -S gives:

movl        a, %eax
sall        $3, %eax

So I guess that issue should more or less be settled. We should change those bitshifts back.

trane commented 13 years ago

Interesting, when you add -O[23] it just makes it into a number, no shifts. Though, it might be different when you are using it in a conditional.

trane commented 13 years ago

Looks like if the value of 'a' is unknown at compile time... bitshifts are fewer operations. In my test, by 5 operations.

trane commented 13 years ago
#include <stdlib.h>

int main(void)
{
    int a = rand() % 10 + 5;
    int x = blah(a);
    int y = blah(a);
}

int blah(int num)
{
    return num * 8;
}

int blah2(int num)
{
    return num << 3;
}
LASFDE1:
    .long   LASFDE1-EH_frame1
    .quad   LFB5-.
    .set L$set$2,LFE5-LFB5
    .quad L$set$2
    .byte   0x0
    .byte   0x4
    .set L$set$3,LCFI0-LFB5
    .long L$set$3
    .byte   0xe
    .byte   0x10
    .byte   0x86
    .byte   0x2
    .byte   0x4
    .set L$set$4,LCFI1-LCFI0
    .long L$set$4
    .byte   0xd
    .byte   0x6
    .byte   0x4
    .set L$set$5,LCFI3-LCFI1
    .long L$set$5
    .byte   0x83
    .byte   0x3

LASFDE3:
    .long   LASFDE3-EH_frame1
    .quad   LFB6-.
    .set L$set$7,LFE6-LFB6
    .quad L$set$7
    .byte   0x0
    .byte   0x4
    .set L$set$8,LCFI4-LFB6
    .long L$set$8
    .byte   0xe
    .byte   0x10
    .byte   0x86
    .byte   0x2
    .byte   0x4
    .set L$set$9,LCFI5-LCFI4
    .long L$set$9
    .byte   0xd
    .byte   0x6
    .align 3
hausdorf commented 13 years ago

It's almost always a bad idea to try to outsmart the compiler. IMHO, we should just change it to multiplication and concentrate on making the algorithm faster, which is historically much more effective at speeding up a program. Imagine if, instead of inventing MapReduce, Google had decided to micro-optimize existing algorithms with bitshifts. The progress in algorithms regularly bests Moore's law by a factor of 1000 (in chess, for example). I could quote Don Knuth ("Premature optimization is of the devil") and stuff, but I think it's a lot more effective to remember that people will look at this code and think we're amateurs. It doesn't usually make the code faster, and it doesn't make us look good in most circles.

In fact, I will go so far as to say that unless we run a detailed profile of the program demonstrates that this will results in significant performance disadvantage, we should just let the compiler do its job. That is, after all, the point of C. If we wanted to program in ASM (sans concurrency, sans type, sans stack), we would do that. C is not ASM, and unless there's a very good reason for using C as ASM, you shouldn't do it, and (IMHO) this is not a good enough reason.

trane commented 13 years ago

If that was your point from the beginning, you should have made it then. Focusing the discussion on the compiler performing the same action and then changing your argument does nothing for the discussion. In fact, you not only change your argument, but go on to talk about how my "premature optimizations" are embarrassing and noobish. everyone knows the Knuth quote and I don't need to be reminded about how bitshift make things less readable. My point was based on our goal, "Make it as fast or faster than git".

I honestly don't care if we use bitshifts or multiplication, but your initial argument for the compiler optimizing it anyway and therefore needed no more discussion was what my comments were about. My comments were meant to be constructive and give evidence to the contrary of your initial assembly output. This last comment was just for you to get on your soap box and be a dick. I don't appreciate it.

hausdorf commented 13 years ago

I get that you're frustrated with stuff. Me, the code, the project, whatever. But I'm not responding to that. My only goal here is to write good software. Believe that or don't, your choice. I personally don't believe that I say things to get on a soapbox and "be a dick".

The issue at hand is manual bit shifting. Yes, it makes code faster, but it doesn't really make code faster. Other people who want to opine on this should pipe in about that. I'm not responding to comments about my character here.

trane commented 13 years ago

I am frustrated that by me giving the assembly output countering your argument it prompted you to respond how you did. I wanted to make it clear that I don't appreciate the slight of hand. Your character is not in question and I agree with the overall idea of making code readable. I don't agree that it warranted 2 paragraphs about how everyone else will think us fools, that Knuth will swoop in from Stanford and kill our future children and that I want to give up C to program in ASM. That was unnecessary and dismissive. I'm sure that you wouldn't appreciate someone talking down to you in that way, too.

hausdorf commented 13 years ago

You can let me spend my time working with this project, or you can force me to spend time and energy working on the social calculus required to make tomorrow's class not awkward. If it seems to you plausible that I wouldn't say that just to dismiss your argument, get on a soapbox, and "be a dick", then I propose that you join me in leaving this problem behind.

trane commented 13 years ago

Fair enough.

hausdorf commented 13 years ago

That would never have worked on a girl. I should switch to dating men.

trane commented 13 years ago

lol