Apply More Effective C++ item 6 advice #3054

Basically that means to prefer prefix increment over postfix ones As the latter produce temporary objects.

Signed-off-by: Olivier Langlois olivier@olivierlanglois.net

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c2b705f0ce0a1c51f71db62b757d43f752499e8a for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

NACK from me. Compilers are very good at optimizing away creation of temporary objects, and I'm against this kind of "worry about micro-optimizations that used to matter in some compiler somewhere but aren't relevant any more" change.

g++ 4.8.1 isn't that good then as I have verified that the generated binaries after the patch were smaller.

I have seen computation programs execution time reduced from 2 hours to 25 mins by doing just that.

Granted bitcoin is probably more io bounded than it is doing heavy STL iterations but why doing gratuitous pessimization when a better form that does not affect readability or code complexity exist is offered?

I want to contribute to your project and before taking on more ambitious tasks, I want to slowly gain your trust by doing small improvements.

Greetings,

Our bottleneck is testing and review, so the best way to slowly gain our trust is to help test and review existing pull requests, before adding to the general workload by submitting small improvements that must still be tested and reviewed.

RE: generated binaries are smaller: how much smaller for release builds? Eleven bytes or eleven-hundred K? Significantly smaller binaries would be a good reason to accept this patch (and should have been mentioned at the start as a reason to accept).

I have seen computation programs execution time reduced from 2 hours to 25 mins by doing just that.

And the improvement here was?

Gavin,

We are talking about 10KB on the 64-bits build but I think that you were right and I must say that this is before stripping the exec. After stripping they have the same size.

I have explored the question further with these small test program

lano1106@Wailaba2 ~/dev/test :( $ cat pre.cpp #include <iostream> #include <map>

int main( int argc, char *argv[] ) { std::map<int,int> m;

for( int i = 0; i < 10; ++i )
    m[i] = i;

for( auto it = m.begin(); it != m.end(); ++it )
{
    std::cout << it->first << ':' << it->second << '\n';
}

return 0;

}

lano1106@Wailaba2 ~/dev/test $ cat post.cpp #include <iostream> #include <map>

int main( int argc, char *argv[] ) { std::map<int,int> m;

for( int i = 0; i < 10; ++i )
    m[i] = i;

for( auto it = m.begin(); it != m.end(); it++ )
{
    std::cout << it->first << ':' << it->second << '\n';
}

return 0;

}

lano1106@Wailaba2 ~/dev/test $ g++ -S -std=c++11 pre.cpp lano1106@Wailaba2 ~/dev/test $ g++ -S -std=c++11 post.cpp lano1106@Wailaba2 ~/dev/test $ diff pre.s post.s

The output is significantly different but as soon as you put a -O switch the difference goes away.

I have learn something from this discussion.

Thanks

We're not a big fan of commits that make changes all over the place. If you want to optimize, it's better to pick a function that's used a lot, benchmark it, then re-benchmark it after the changes.

@gavinandresen I'm pretty sure it would violate the C++ spec to optimise away postfix copying. @lano1106 Gaining trust should not be a goal here. Every change must be carefully reviewed, tested, and audited, regardless of how much "trust" anyone has for the author.