Use prefix instead of postfix increment/decrement for non-trivial typ… #11149

danra commented at 10:25 PM on August 25, 2017: contributor

…es where the result isn't used (and for trivial types as well in the changed files, for consistency).

Follow Scott Meyers' advice in his More Effective C++ book, as well as cppcheck's performance messages, and use prefix instead of postfix increment/decrement for objects of non-trivial type, whenever the result of the increment isn't used, to avoid possible extra copies and maximize performance.

All the modified files fix this for some objects of non-trivial type. Increments/decrements of trivial types in these files were also converted to prefix instead of postfix (wherever the result isn't used) for consistency.

Use prefix instead of postfix increment/decrement for non-trivial types where the result isn't used.

Follow Scott Meyers' advice in his More Effective C++ book, as well as cppcheck's performance messages, and use prefix instead of postfix increment/decrement for objects of non-trivial type, whenever the result of the increment isn't used, to avoid possible extra copies and maximize performance.

All the modified files fix this for some objects of non-trivial type. Increments/decrements of trivial types in these files were also converted to prefix instead of postfix (wherever the result isn't used) for consistency.

e6d7e4ee09

in src/arith_uint256.h:36 in e6d7e4ee09

  32 | @@ -33,21 +33,21 @@ class base_uint
  33 |      {
  34 |          static_assert(BITS/32 > 0 && BITS%32 == 0, "Template parameter BITS must be a positive multiple of 32.");
  35 |  
  36 | -        for (int i = 0; i < WIDTH; i++)

luke-jr commented at 10:32 PM on August 25, 2017:

While you're touching these, can you add braces too (in a separate commit in the same PR)?

in src/net.cpp:2387 in e6d7e4ee09

2383 | @@ -2384,13 +2384,13 @@ void CConnman::Interrupt()
2384 |      InterruptSocks5(true);
2385 |  
2386 |      if (semOutbound) {
2387 | -        for (int i=0; i<(nMaxOutbound + nMaxFeeler); i++) {

luke-jr commented at 10:34 PM on August 25, 2017:

Add some spaces here.

luke-jr approved

luke-jr commented at 10:40 PM on August 25, 2017: member

utACK. Was there a script to automate this?

Maybe update the code format guidelines too...

promag commented at 10:43 PM on August 25, 2017: member

IMO this can turn to be one of the never merge PR. I agree with the change but I'm afraid this may force lots of rebases and there isn't that much value on the change.

danra commented at 10:54 PM on August 25, 2017: contributor

@promag Would this be more manageable if this was split into single-file commits? I wouldn't mind doing that, but I'm just started out with pull requests to this repo and so far I got the impression that multiple small pull requests would be frowned upon.

danra commented at 10:56 PM on August 25, 2017: contributor

@luke-jr cppcheck pointed to all the non-trivial object increments which should be optimized. Beyond that, there was no automation. Automation of this would probably require building a clang module or similar, since one can't blindly change all postfixes to prefixes, only those where the result is not used.

promag commented at 11:02 PM on August 25, 2017: member

I think everyone agrees if you add a developer note with the advice, and keep an eye on PR's that add or touch code with postfix inc/dec.

As for this PR, wait for more feedback. If you could make this scripted then I wouldn't mind if it doesn't fix the code style as @luke-jr suggests.

danra commented at 11:19 PM on August 25, 2017: contributor

@promag There is actually already such a note in the developer notes:

++i is preferred over i++.

Should I expand on the reasoning for this in the developer notes?

in src/addrman.cpp:280 in e6d7e4ee09

 276 | @@ -277,14 +277,14 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP
 277 |  
 278 |          // stochastic test: previous nRefCount == N: 2^N times harder to increase it
 279 |          int nFactor = 1;
 280 | -        for (int n = 0; n < pinfo->nRefCount; n++)
 281 | +        for (int n = 0; n < pinfo->nRefCount; ++n)

MarcoFalke commented at 11:20 PM on August 25, 2017:

In the subject you mention "for non-trivial types". I always thought that int was a trivial type.

danra commented at 11:28 PM on August 25, 2017:

I've explained this in the commit message body:

All the modified files fix this for some objects of non-trivial type. Increments/decrements of trivial types in these files were also converted to prefix instead of postfix (wherever the result isn't used) for consistency.

But, I'll change the subject to reflect that as well.

achow101 commented at 3:57 AM on August 26, 2017: member

I tend toward NACKing this. It is a lot of code churn and will likely require a lot of other PRs to be rebased and work through a lot of merge conflict stuff. Also, shouldn't the compiler be optimizing out all of those anyways so it doesn't really matter?

laanwj added the label Refactoring on Aug 26, 2017

danra commented at 8:59 AM on August 26, 2017: contributor

@achow101

Also, shouldn't the compiler be optimizing out all of those anyways so it doesn't really matter?

The experts say no. Isn't Scott Meyers' advice authoritative enough? :)

Also, see https://stackoverflow.com/questions/30036749/is-it-still-better-to-prefer-pre-increment-over-post-increment

My favorite quote from there:

The point is, the compiler is free to change your i++ to ++i if it can prove that such transformation wont change the behavior of the program. For built-in types, proofs are easy. For class types, it could be really difficult (or practically expensive)!

promag commented at 9:14 AM on August 26, 2017: member

NACK, although it's a better code and all, unless you show numbers of performance increase.

laanwj commented at 10:51 AM on August 26, 2017: member

This should certainly be done in new code, but not sure about changing this for the entire existing code at once. Seems too much. Usually we improve minor things like this as time goes on and code needs to be changed anyway.

Also this does have a risk attached, as the compiled code will not always stay the same (which is the point).

Do you see any differences in benchmarks/profiles to warrant this?

danra commented at 11:32 AM on August 26, 2017: contributor

@laanwj I don't. Just thought that fixing some cppcheck performance messages as well as following the developer guidelines would be reason enough to introduce this fix.

meshcollider commented at 12:12 PM on August 26, 2017: contributor

Yeah because there are no real benefits to the change other than code style changes, its not worth the merge issues and even small risks imo, so I'd have to give this a NACK. Thanks though, nice in theory :)

MarcoFalke commented at 12:04 AM on August 27, 2017: member

The last time someone tried those changes, we compared the binaries before and after and nothing changed. Implying that the compiler can handle this.

On Sat, Aug 26, 2017 at 8:13 AM, MeshCollider notifications@github.com wrote:

Yeah because there are no real benefits to the change other than code style changes, its not worth the merge issues and even small risks imo, so I'd have to give this a NACK. Thanks though, nice in theory :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11149#issuecomment-325121143, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv-otTBsJPnSJ2FkpvuiwpRJr9FmRks5scAvZgaJpZM4PDOOK .

laanwj commented at 8:23 AM on August 28, 2017: member

Ok, closing this then. Sorry.

laanwj closed this on Aug 28, 2017

DrahtBot locked this on Sep 8, 2021