Improved efficiency in COutPoint constructors

mm-s commented at 8:06 am on April 28, 2017: contributor

memset(0) executed twice, first in uint256_t default constructor and then in SetNull.

Remake of https://github.com/bitcoin/bitcoin/pull/10277

in src/primitives/transaction.h:26 in 90703eece7 outdated

21@@ -22,8 +22,8 @@ class COutPoint
22     uint256 hash;
23     uint32_t n;
24 
25-    COutPoint() { SetNull(); }
26-    COutPoint(uint256 hashIn, uint32_t nIn) { hash = hashIn; n = nIn; }
27+    COutPoint(): n( (uint32_t) -1 ) { }
28+    COutPoint(const uint256& hash, uint32_t n): hash(hash), n(n) { }

kallewoof commented at 8:10 am on April 28, 2017:

Can you leave the names as hashIn and nIn so we don’t get hash(hash), n(n)?

dcousens approved

dcousens commented at 8:23 am on April 28, 2017: contributor

utACK with @kallewoof ’s suggestion

Improved efficiency in COutPoint constructors 4fbae77929

in src/primitives/transaction.h:25 in 90703eece7 outdated

21@@ -22,8 +22,8 @@ class COutPoint
22     uint256 hash;
23     uint32_t n;
24 
25-    COutPoint() { SetNull(); }
26-    COutPoint(uint256 hashIn, uint32_t nIn) { hash = hashIn; n = nIn; }
27+    COutPoint(): n( (uint32_t) -1 ) { }

dcousens commented at 8:24 am on April 28, 2017:

Inconsistent spacing, n((uint32_t) -1) is fine

mm-s force-pushed on Apr 28, 2017

mm-s commented at 8:44 am on April 28, 2017: contributor

Fixed both suggestions. @dcousens & @kallewoof, although I’d like to state my opinion on these reviews: 1.- “In” suffix does not add any value and makes the sentence more verbose and, in my opinion, less readable. 2.- Consistency rules often carry problems related to lack of diversity.

Thanks

dcousens approved

dcousens commented at 9:20 am on April 28, 2017: contributor

utACK 4fbae77

kallewoof commented at 3:02 pm on April 28, 2017: member

I would argue that foo(foo) is less ambiguous despite semantic equivalence. It’s also the standard that’s been adhered to in the code thus far so it’d require some amount of merit to break.

mm-s commented at 3:29 pm on April 28, 2017: contributor

it is just that c(p):p(p) is a better abstraction than c(pIn):p(pIn) since In does not add any meaning to the code, apart from remarking that it is an input parameter which is obvious.

kallewoof commented at 3:41 pm on April 28, 2017: member

That’s all fine and dandy until you start doing things like

0class foo {
1    int x;
2    foo(int x) { x = x * 48; } 
3};

which, crazy as it may seem, is not very far away from your suggestion.

JeremyRubin commented at 8:43 pm on April 28, 2017: contributor

Weak Concept Ack.

Most likely this gets inlined and optimized out so this shouldn’t impact the generated code?

If that isn’t the case, I have a slight preference to make this happen by exposing an uninitialized constructor for uin256_t and then calling SetNull.

fanquake added the label Resource usage on Apr 29, 2017

mm-s commented at 6:40 pm on April 30, 2017: contributor

@JeremyRubin Not easy decision: The default constructor is reseting to 0, adding a constructor that leaves the object uninitalized looks like having an ugly prototype; on the other hand changing the behavior by leaving the object uninitalized in the default constructor and adding another constructor accepting a uint8[] looks like it is the right design, but at this point it may confuse to whom is already assuming that the default constructor is reseting the memory, apart from checking every single call done in the codebase.

JeremyRubin commented at 5:40 am on May 1, 2017: contributor

I’m not suggesting that it has to/should be the default constructor.

Also the main part is that this likely does not impact the actual code generated; you should try outputting optimized assembly for

0#include <memory>
1int main() {
2    char x[32];
3    memset((void*)*x, 0, 32);
4}

and

0#include <memory>
1int main() {
2    char x[32];
3    memset((void*)*x, 0, 32);
4    memset((void*)*x, 0, 32);
5}

I didn’t see a difference even at O1.

sipa commented at 5:46 am on May 1, 2017: member

@JeremyRubin Also when the code is split over different modules?

mm-s commented at 12:09 pm on May 1, 2017: contributor

@JeremyRubin The compiler can resolve programmer’s errors automatically, but it is preferably to have the high level code well designed before the optimizer checks ; with so many compiler vendors , so many flags and so many archs out there you can never say for sure the compiler will end up optimizing. Also i commented on the possibility of not using the default constructor, and seemed to me ugly, which prototype would you use for a constructor that does not initialize?

JeremyRubin commented at 4:49 pm on May 1, 2017: contributor

@sipa I think the memset occurs in the header so it’s my understanding it would be inlined.

edit: notice that memset is defined in a different header in my example. @mm-s while this is true, your PR is not fixing an error it is an optimization, which introduces a dependency on the 0 initialization of uint256 that might introduce bugs later. Perhaps one easy way is:

0class X {
1    public:
2        X(bool initialized=true) {
3        }
4};

sipa commented at 11:31 pm on May 1, 2017: member

utACK 4fbae77929e6344bc49ab60af10a9c5ff21d2cdf

I think this change is perfectly reasonable. There is a slight duplication between the SetNull function and the constructor, but it comes at almost no extra complexity. Please let’s not introduce explicit non-initializing constructors.

sipa merged this on May 1, 2017

sipa closed this on May 1, 2017

sipa referenced this in commit e4bbd3d230 on May 1, 2017

PastaPastaPasta referenced this in commit 649f48dde1 on Jun 10, 2019

PastaPastaPasta referenced this in commit f2e64b8099 on Jun 10, 2019

PastaPastaPasta referenced this in commit 14dfb8d272 on Jun 11, 2019

PastaPastaPasta referenced this in commit b7ecfcdbd4 on Jun 11, 2019

PastaPastaPasta referenced this in commit 8064a1900a on Jun 12, 2019

PastaPastaPasta referenced this in commit d832afe1bd on Jun 14, 2019

PastaPastaPasta referenced this in commit 3cd93ca8c3 on Jun 14, 2019

PastaPastaPasta referenced this in commit ae55e3f34f on Jun 14, 2019

PastaPastaPasta referenced this in commit 717e0957ec on Jun 14, 2019

PastaPastaPasta referenced this in commit d83a24ba77 on Jun 15, 2019

PastaPastaPasta referenced this in commit 2284e60480 on Jun 19, 2019

barrystyle referenced this in commit 60938477ac on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

Improved efficiency in COutPoint constructors #10292