Avoid unnecessary copying of non-cheaply-copied types #10776

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:const-ref changing 12 files +28 −28

practicalswift commented at 9:37 PM on July 8, 2017: contributor
- Avoid unnecessary copying of non-cheaply-copied types. Use pass by reference to const consistently where appropriate.
- Don't use pass by reference to const for cheaply-copied types (bool, char, etc.).
Avoid unnecessary copying of non-cheaply-copied types. Use pass by reference to const consistently. 2eb3e797aa
Don't use pass by reference to const for cheaply-copied fundamental types (bool, char, etc.). 2b7df00a5c
promag commented at 11:04 PM on July 8, 2017: member

PR title too long. Please keep it short and clear.

in src/wallet/wallet.cpp:1686 in 2b7df00a5c

1682 | @@ -1683,7 +1683,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const
1683 |      return credit;
1684 |  }
1685 |  
1686 | -CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const
1687 | +CAmount CWalletTx::GetImmatureCredit(const bool fUseCache) const

promag commented at 12:00 AM on July 9, 2017:

Why these changes?

practicalswift commented at 5:24 AM on July 9, 2017:

To get signature consistency with GetImmatureWatchOnlyCredit. See the other changes made to the same file.

promag commented at 12:09 AM on July 9, 2017: member
What are the non-cheaply-copied types? Have you measured the difference?

Also, this is incomplete, for instance, for type uint256:
```
bool bumpFee(uint256 hash);
```
could be
```
bool bumpFee(const uint256& hash);
```
sipa commented at 12:28 AM on July 9, 2017: member

@promag Generally, anything larger than 1 or 2 pointers is better passed by reference.
practicalswift commented at 5:31 AM on July 9, 2017: contributor

@promag See @sipa:s answer for the common definition of (non-)cheaply-copied types.

Regarding completeness: I only targeted const arguments.
practicalswift renamed this:
~~Avoid unnecessary copying of non-cheaply-copied types. Use pass by reference to const consistently where appropriate.~~
Avoid unnecessary copying of non-cheaply-copied types
on Jul 9, 2017
in src/wallet/feebumper.cpp:69 in 2b7df00a5c
```
  65 | @@ -66,7 +66,7 @@ bool CFeeBumper::preconditionChecks(const CWallet *pWallet, const CWalletTx& wtx
  66 |      return true;
  67 |  }
  68 |  
  69 | -CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConfirmTarget, bool ignoreGlobalPayTxFee, CAmount totalFee, bool newTxReplaceable)
  70 | +CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256& txidIn, int newConfirmTarget, bool ignoreGlobalPayTxFee, CAmount totalFee, bool newTxReplaceable)
```
TheBlueMatt commented at 11:29 PM on July 9, 2017:

This is broken - txidIn is std::move()d two lines down.

sipa commented at 8:13 AM on July 10, 2017:

It certainly seems misplaced, but it's not actually wrong, as std::move in this case just casts to const uint256&&, which won't be moved from (also, uint256 doesn't have a move constructor that destroys the argument).

dcousens commented at 8:14 AM on July 10, 2017:

const uint256& will not be moved, std::move is simply a cast, it will do nothing here. Ref: https://stackoverflow.com/questions/28595117/why-can-we-use-stdmove-on-a-const-object

ArielLorenzo-Luaces commented at 1:36 PM on July 10, 2017:

txidIn should not be const nor pass-by-reference. If CFeeBumper is going to cannibalize txidIn two lines down it should be copied in the case it was an lvalue. In the case that it was an rvalue, it will not be copied anyway. const uint256& only causes inefficiencies (due to unnecessary copying) when the caller sends txidIn as an rvalue

edit: My mistake, the inefficiency I mentioned above only applies when having to reallocate data for each pointer in uint256, which there are none. While using pass by reference will save you 1 copy.

TheBlueMatt commented at 1:48 PM on July 10, 2017:

@sipa sorry, broken is a bad word there, I meant "unsafe code", ie we want to be careful of having code like that.
TheBlueMatt commented at 11:31 PM on July 9, 2017: member

These kinds of changes scare me, see the bug introduced in CFeeBumper. Some of them are obvious, some of them could subtley introduce bugs (did you look deeply at the CTxMempool changes?).
fanquake deleted a comment on Jul 10, 2017
fanquake deleted a comment on Jul 10, 2017
practicalswift commented at 5:18 AM on July 10, 2017: contributor

@TheBlueMatt Good point! I missed the std::move(…). Closing this PR - I'll have to evaluate it deeper and contemplate if the performance reward is worth any potential risk. Thanks for reviewing!
practicalswift closed this on Jul 10, 2017
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 16, 2022

Contributors