memset(0) executed twice, first in uint256_t default constructor and then in SetNull.
Improved efficiency in COutPoint constructors #10292
pull mm-s wants to merge 1 commits into bitcoin:master from mm-s:master changing 1 files +2 −2-
mm-s commented at 8:06 am on April 28, 2017: contributor
-
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 ashashIn
andnIn
so we don’t gethash(hash), n(n)
?dcousens approveddcousens commented at 8:23 am on April 28, 2017: contributorutACK with @kallewoof ’s suggestionImproved efficiency in COutPoint constructors 4fbae77929in 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 finemm-s force-pushed on Apr 28, 2017mm-s commented at 8:44 am on April 28, 2017: contributorFixed 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 approveddcousens commented at 9:20 am on April 28, 2017: contributorutACK 4fbae77kallewoof commented at 3:02 pm on April 28, 2017: memberI 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: contributorit 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: memberThat’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: contributorWeak 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, 2017mm-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: contributorI’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: memberutACK 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, 2017sipa closed this on May 1, 2017
sipa referenced this in commit e4bbd3d230 on May 1, 2017PastaPastaPasta referenced this in commit 649f48dde1 on Jun 10, 2019PastaPastaPasta referenced this in commit f2e64b8099 on Jun 10, 2019PastaPastaPasta referenced this in commit 14dfb8d272 on Jun 11, 2019PastaPastaPasta referenced this in commit b7ecfcdbd4 on Jun 11, 2019PastaPastaPasta referenced this in commit 8064a1900a on Jun 12, 2019PastaPastaPasta referenced this in commit d832afe1bd on Jun 14, 2019PastaPastaPasta referenced this in commit 3cd93ca8c3 on Jun 14, 2019PastaPastaPasta referenced this in commit ae55e3f34f on Jun 14, 2019PastaPastaPasta referenced this in commit 717e0957ec on Jun 14, 2019PastaPastaPasta referenced this in commit d83a24ba77 on Jun 15, 2019PastaPastaPasta referenced this in commit 2284e60480 on Jun 19, 2019barrystyle referenced this in commit 60938477ac on Jan 22, 2020MarcoFalke locked this on Sep 8, 2021
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC
More mirrored repositories can be found on mirror.b10c.me