Cleanup move semantics and other misc cleanups #3348

seelabs · 2020-04-10T20:45:17Z

Clean up some code flagged by a static analysis run

codecov-io · 2020-04-10T21:33:32Z

Codecov Report

Merging #3348 into develop will decrease coverage by 0.00%.
The diff coverage is 68.42%.

@@             Coverage Diff             @@
##           develop    #3348      +/-   ##
===========================================
- Coverage    70.22%   70.22%   -0.01%     
===========================================
  Files          683      683              
  Lines        54621    54625       +4     
===========================================
+ Hits         38360    38362       +2     
- Misses       16261    16263       +2

Impacted Files	Coverage Δ
src/ripple/app/ledger/Ledger.cpp	`78.99% <ø> (ø)`
src/ripple/app/misc/TxQ.h	`96.66% <ø> (ø)`
src/ripple/app/misc/impl/TxQ.cpp	`95.98% <ø> (-0.02%)`	⬇️
src/ripple/nodestore/impl/Shard.cpp	`0.00% <0.00%> (ø)`
src/ripple/overlay/impl/OverlayImpl.cpp	`29.13% <0.00%> (ø)`
src/ripple/overlay/impl/PeerImp.cpp	`0.00% <0.00%> (ø)`
src/ripple/rpc/impl/ShardArchiveHandler.cpp	`62.84% <0.00%> (-0.29%)`	⬇️
src/ripple/rpc/impl/TransactionSign.cpp	`89.04% <ø> (ø)`
src/ripple/shamap/SHAMap.h	`96.96% <ø> (ø)`
src/ripple/shamap/SHAMapTreeNode.h	`100.00% <ø> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 023f570...6a69340. Read the comment docs.

HowardHinnant · 2020-04-10T22:47:39Z

src/ripple/app/main/GRPCServer.cpp

@@ -55,7 +55,7 @@ GRPCServerImpl::CallData<Request, Response>::CallData(
    , responder_(&ctx_)
    , bindListener_(std::move(bindListener))
    , handler_(std::move(handler))
-    , requiredCondition_(std::move(requiredCondition))
+    , requiredCondition_(requiredCondition)


Intended for wider team review:

I'm uncomfortable removing a std::move in a context like this because it will cause future readers to wonder if the missing move on this one argument is perhaps an oversight and should be added. Future readers will continually have to look up the underlying type to discover that a move on it will neither help nor hurt.

Move semantics was designed from day 1 to interoperate with "copy only" types with no negative consequences so that programmers would not have to worry about this issue so much. Especially in generic code, but more generally, even in situations like this.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1377.htm#Cast%20to%20rvalue

The "request to move" semantics turn out to be very handy in generic code. One can request that a type move itself without having to know whether or not the type is really movable. If the type is movable it will move, else if the type is copyable, it will copy, else you will get a compile-time error.

I'm not sure there's a good universal answer for this. I agree that uses of std::move in copy-only situations are benign. I think there are two additional considerations however.

Does the presence of the std::move cause an otherwise useful static checker to produce enough noise that the static checker becomes less useful? [E.g., it generatesstd::move has no effect]. We have to think about keeping our tools useful.

It is possible for there to be a situation where if a move took place there would be a use after move, but that move doesn't currently happen because the type is only copyable. (That's not the situation right here.) So leaving that move in place would become a bug if the type actually becomes moveable in the future.

I think consideration 1 is the most important. If consideration 1 can be worked around then tools and code reviews will probably manage issue 2 for us? But consideration 2 would add risk to making a type moveable if it hasn't been before.

Scott D tells me that this warning can be disabled, without disabling a warning about use after move, or a warning about moves that cause the code to become more expensive.

I agree that case 2 is a bug that should be fixed (except when that use has no preconditions, e.g. assign-to). And I believe that the tools will catch that.

Another aspect that has come up between Scott and I in private conversations is that this warning is flagging cases where the client signals his intention that he no longer needs the value with move. But the std::move is only a request to move, not a requirement. It is up to the far away client code whether to actually pilfer the object or not. And that can change with the implementation of the far away code, without needing to survey clients. Clients silently get the optimization when the far away code changes, with no semantic or syntax change to the client code. Example:

06740e6#diff-9647085a1d5d0d2cf7b0b835b0e8c742R147

In this case the client is saying I don't need the value of ledger any more, but the RCLValidatedLedger constructor doesn't move from it. But in the RCLValidatedLedger might change to move from it. If that happens, the client gets the optimization automatically and silently. And in the mean time, the move in the client code causes no harm, and signals to the reader the intent: last use.

I'm a bit torn here. On the one hand, I understand that std::move is just a request, and programmers can't expect it to actually mean that the given instance is being moved from. On the other hand, specifying it, especially in a non-generic-programming context, when you know it's not applicable only serves to confuse.

Either way, a future programmer has to go looking at the code to attempt to decipher what's going on and if this is a case where the move was missed or explicitly omitted. In that context, I wonder if it makes sense to use a marker similar to std::move that expressly indicates that the programmer expliictly chose to not use std::move?

In the vast majority of cases move is used for two things: 1) as an optimization technique; 2) To pass ownership of unique resources.

Code that's missing move (that compiles) is almost never a bug. I don't think anyone is going to look at code where a statement has a missing move and wonder if there's a bug (although they may wondering if there's a missed optimization opportunity).

Up to now, we haven't put move in code that didn't have a benefit (at least intentionally). If we see a move, we can infer that the type would benefit from a move. Seeing move(requiredCondition), gives a false signal about the type of requiredCondition. I would not have guessed that someone would put a move around an enum (of course, that's a circular argument - it sends a signal about the type because that's how we've used it; you are proposing to change that). The other "cost" of putting in a unneeded move is readability suffers. foo(std::move(a), std::move(b), std::move(c)) is more cluttered foo(a, b, c). If we put a move, I'd like it to have a potential benefit more than the surrounding code uses move.

The move in the RCLValidatedLedger(ledger) that you pointed out is a different case. It would not send the wrong signal about the ledger type, and there is a realistic chance that a future change would take advantage of the move.

My vote is I don't want the move for the enum. I'm weakly against the move for RCLValidatedLedger, but only weakly (I'd prefer moves only where it makes a difference); I do see legitimate advantages of putting it there.

Of course, Howard has thought about move semantics move deeply that any of us. I think I'll add the move back in to the RCLValidatedLedger call - Howard's vote should count move than my "weakly against".

I'll also don't think putting a move around an enum is that big a deal. The "costs" I've mentioned are all minor. While I'm more strongly against that change, I'm OK doing it if you (Howard) feel strongly.

Note: I'm not worried about making the static analyzer happy. It would be a large task and I suspect the code would be worse for the effort.

@seelabs Maybe I am misunderstanding how move works, but can't we do something like this?: cjcobb23@92a60d5

Based on my understanding of move semantics, the actual move is only going to happen in the callback at NetworkOPs.cpp:2273; specifically, instead of copying the Blobs into the vector, the data will be moved. For the callback that doesn't move the parameters ( NetworkOPs.cpp:2234), no actual move will occur; the arguments will be passed by reference. So, the callback that does not move will not behave any differently. Feel free to correct me if I am wrong (@HowardHinnant ).

Re: reallocation within soci. The parameters used by soci are actually the txnData and txnMeta variables, which have type soci::blob( link). We then, call the convert function (defined here), to copy data from a soci::blob into a Blob. This convert function first performs a resize on the Blob, and then copies the data from soci::blob into Blob.
The move I am proposing would then move those Blobs into the vector (which is captured by reference in the lambdas), instead of copying into the vector.

Let's consider each case. First, without the move: The Blob objects are being reused, which means that the resize in convert might not perform a reallocation; the object will already own some heap storage from the previous iteration of the loop. Then, a copy is performed (inside convert). Then, the Blob is copied into the result vector, resulting in a copy as well as an allocation; the allocation occurs during the copy constructor of Blob. In summary, there is a possible but unlikely reallocation, followed by a copy, another copy and an allocation.

Now, with the move: The Blob objects that are being reused will not own any heap storage, since at the end of each iteration of the loop, the Blobs will be moved from. So, the resize in convert will always perform an allocation. Then, a copy is performed (inside convert). Then, Blob is moved into the result vector, meaning no copy and no allocation.

Comparing the two:
Without move: possible but unlikely allocation, copy, copy, allocation
With move: allocation, copy
To keep things simple, we can ignore the possible but unlikely allocation in the first case. In this way, the two approaches are identical except for an additional copy if the move is not used.

Sorry in advance if I am mistaken and this is all wrong. I would really like the move to occur in this situation, since the account_tx call is rather expensive, and it would be nice to increase the performance.

@cjcobb23 Very nice! I did not consider passing rvalue references to the callback (I almost always pass sink parameters as values - that way callers can choose to move or copy at their discretion). In this case there's only one caller, and we know we can move there. Love it! (And smacking myself on the head for not thinking of it myself).

@cjcobb23 Just a note: For the case of the callback on NetworkOPs.cpp:2234, if we did pass by value, that change would be pessimizing for that case (but of course, not for pass by rvalue ref). So we really wouldn't want to do this if you didn't suggest passing by rvalue ref.

@cjcobb23 fixed in 334821850 [fold] Use rvalue refs on accountTxPage callbacks

I feel like the only person who doesn't find std::move around a small value type confusing. I'd be more confused by seeing a list of constructor initializers where some but not all arguments are moved.

I think Howard has already said everything I feel:

A std::move that doesn't move doesn't hurt anything, and could help if, one day, it starts to move.

A std::move serves as a good indicator of "I'm done with this value", even if it doesn't move.

A std::move is not a guarantee of a move.

In my opinion, any confusion surrounding an appearance of std::move is likely caused by the reader's misinterpretation of std::move as an expression of "this value will be moved" instead of "I'm done with this value, and don't care if it is moved or not". Perhaps we could define an alias like give.

nbougalis · 2020-04-11T04:25:19Z

src/ripple/app/main/Application.cpp

@@ -1726,7 +1727,7 @@ int ApplicationImp::fdRequired() const
    int needed = 128;

    // 1.5 times the configured peer limit for peer connections:
-    needed += static_cast<int>(0.5 + (1.5 * overlay_->limit()));
+    needed += lround(1.5 * overlay_->limit());


We should just do away with the floating point math here and just use 2 * overlay_->limit() instead.

Configuring additional file descriptors doesn't actually hurt us, with one exception: we hit the hard rlimit and if that happens, we produce a useful message and exit.

Fixed in bd4cc1711 [fold] Misc fixes:

nbougalis · 2020-04-11T05:09:16Z

src/ripple/app/main/GRPCServer.cpp

@@ -55,7 +55,7 @@ GRPCServerImpl::CallData<Request, Response>::CallData(
    , responder_(&ctx_)
    , bindListener_(std::move(bindListener))
    , handler_(std::move(handler))
-    , requiredCondition_(std::move(requiredCondition))
+    , requiredCondition_(requiredCondition)


I'm a bit torn here. On the one hand, I understand that std::move is just a request, and programmers can't expect it to actually mean that the given instance is being moved from. On the other hand, specifying it, especially in a non-generic-programming context, when you know it's not applicable only serves to confuse.

Either way, a future programmer has to go looking at the code to attempt to decipher what's going on and if this is a case where the move was missed or explicitly omitted. In that context, I wonder if it makes sense to use a marker similar to std::move that expressly indicates that the programmer expliictly chose to not use std::move?

cjcobb23 · 2020-04-13T16:58:29Z

src/ripple/app/main/GRPCServer.cpp

@@ -55,7 +55,7 @@ GRPCServerImpl::CallData<Request, Response>::CallData(
    , responder_(&ctx_)
    , bindListener_(std::move(bindListener))
    , handler_(std::move(handler))
-    , requiredCondition_(std::move(requiredCondition))
+    , requiredCondition_(requiredCondition)


Move semantics was designed from day 1 to interoperate with "copy only" types with no negative consequences so
that programmers would not have to worry about this issue so much.

This is generally how I feel. I see the use of std::move as a request to move, written as an optimization. If the move doesn't happen, its not usually a bug or a problem, unless I really, really need that optimization. For this reason, I am hesitant to remove a move.

However, I agree that a move around an enum, or any type that clearly has no heap storage (int, bool, etc), is confusing to the reader; an actual move could never happen, so using move makes it seem like the programmer made a mistake, makes the intentions of the code unclear and might cause the reader to question whether they know what is going on. However, I think this situation is somewhat of an exception; the other arguments to the constructor are being moved from, so the intention seems clear to me: "Move each argument, if possible". I'm fine if you want to remove the move here, though I don't think its necessary to do so.

However, I am confused why the move is being removed from here: 06740e6#diff-3f6df86ad506fd7e45db86b186e102e2R2273 .
This seems like a perfect use case for move. Why is this being removed? Is the move not actually happening? If so, instead of removing the std::move, we should figure out how to make the move actually happen.

src/ripple/app/misc/NetworkOPs.cpp

cjcobb23

LGTM

nbougalis

Left a few comments. I'd like to see the two comments about addGiveUpdate and addUpdateItem addressed. Beyond that, it's at your discretion.

src/ripple/app/ledger/Ledger.cpp

src/ripple/app/misc/NetworkOPs.cpp

src/ripple/app/misc/impl/AccountTxPaging.cpp

src/ripple/app/misc/impl/TxQ.cpp

nbougalis · 2020-04-17T03:24:30Z

src/ripple/app/main/Application.cpp

@@ -1726,7 +1727,7 @@ int ApplicationImp::fdRequired() const
    int needed = 128;

    // 1.5 times the configured peer limit for peer connections:
-    needed += static_cast<int>(0.5 + (1.5 * overlay_->limit()));
+    needed += lround(1.5 * overlay_->limit());


HowardHinnant · 2020-04-21T21:03:53Z

Here, https://github.com/ripple/rippled/blob/develop/src/ripple/app/misc/FeeVoteImpl.cpp#L254 , tItem should now be moved.

HowardHinnant · 2020-04-21T21:17:12Z

Here, https://github.com/ripple/rippled/blob/develop/src/ripple/shamap/impl/SHAMapTreeNode.cpp#L99 , item should now be moved (in 12 similar places).

seelabs · 2020-04-22T02:11:20Z

@HowardHinnant I can't comment on top level comments, but all of those top level comments should be fixed in 4e58b1edc [fold] Add some missing std::moves

BTW, I know this needs to be rebased. I expect there will be a new beta tomorrow (the re-formatting beta), so I'll rebase after that beta gets merged.

seelabs · 2020-04-24T15:46:40Z

Squashed and rebased onto b3

seelabs · 2020-04-28T18:00:38Z

rebased onto b4

* Make sure variables are always initialized * Use lround instead of adding .5 and casting * Remove some unneeded vars * Check for null before calling strcmp * Remove redundant if conditions * Remove make_TxQ factory function

seelabs · 2020-05-08T20:19:31Z

rebased on b5

codecov-commenter · 2020-05-26T04:00:54Z

Codecov Report

Merging #3348 into develop will decrease coverage by 0.00%.
The diff coverage is 68.42%.

@@             Coverage Diff             @@
##           develop    #3348      +/-   ##
===========================================
- Coverage    70.44%   70.44%   -0.01%     
===========================================
  Files          682      682              
  Lines        54392    54396       +4     
===========================================
+ Hits         38315    38317       +2     
- Misses       16077    16079       +2

Impacted Files	Coverage Δ
src/ripple/app/ledger/Ledger.cpp	`78.99% <ø> (ø)`
src/ripple/app/misc/TxQ.h	`96.66% <ø> (ø)`
src/ripple/app/misc/impl/TxQ.cpp	`95.98% <ø> (-0.02%)`	⬇️
src/ripple/nodestore/impl/Shard.cpp	`0.00% <0.00%> (ø)`
src/ripple/overlay/impl/OverlayImpl.cpp	`28.76% <0.00%> (ø)`
src/ripple/overlay/impl/PeerImp.cpp	`0.00% <0.00%> (ø)`
src/ripple/rpc/impl/ShardArchiveHandler.cpp	`62.84% <0.00%> (-0.29%)`	⬇️
src/ripple/rpc/impl/TransactionSign.cpp	`89.04% <ø> (ø)`
src/ripple/shamap/SHAMap.h	`96.96% <ø> (ø)`
src/ripple/shamap/SHAMapTreeNode.h	`100.00% <ø> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9771210...ef7c9da. Read the comment docs.

seelabs requested review from HowardHinnant and cjcobb23 April 10, 2020 20:45

seelabs assigned HowardHinnant and cjcobb23 Apr 10, 2020

HowardHinnant reviewed Apr 10, 2020

View reviewed changes

nbougalis previously approved these changes Apr 11, 2020

View reviewed changes

cjcobb23 suggested changes Apr 13, 2020

View reviewed changes

cjcobb23 reviewed Apr 13, 2020

View reviewed changes

src/ripple/app/misc/NetworkOPs.cpp Outdated Show resolved Hide resolved

seelabs dismissed nbougalis’s stale review via d3684aa April 13, 2020 21:30

cjcobb23 previously approved these changes Apr 14, 2020

View reviewed changes

nbougalis suggested changes Apr 17, 2020

View reviewed changes

seelabs dismissed cjcobb23’s stale review via bd4cc17 April 17, 2020 16:25

HowardHinnant previously approved these changes Apr 22, 2020

View reviewed changes

seelabs dismissed HowardHinnant’s stale review via 6a69340 April 24, 2020 15:45

seelabs force-pushed the cleanups branch from 4e58b1e to 6a69340 Compare April 24, 2020 15:45

seelabs force-pushed the cleanups branch from 6a69340 to 4ee8a79 Compare April 28, 2020 17:59

nbougalis previously approved these changes Apr 28, 2020

View reviewed changes

seelabs added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Apr 28, 2020

carlhua added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Apr 28, 2020

seelabs mentioned this pull request Apr 29, 2020

Implemented NegativeUNL #3380

Closed

Cleanup code using move semantics

162e374

Minor cleanups:

ef7c9da

* Make sure variables are always initialized * Use lround instead of adding .5 and casting * Remove some unneeded vars * Check for null before calling strcmp * Remove redundant if conditions * Remove make_TxQ factory function

seelabs dismissed nbougalis’s stale review via ef7c9da May 8, 2020 20:18

seelabs force-pushed the cleanups branch from 4ee8a79 to ef7c9da Compare May 8, 2020 20:18

HowardHinnant self-requested a review May 8, 2020 20:35

HowardHinnant approved these changes May 8, 2020

View reviewed changes

cjcobb23 approved these changes May 9, 2020

View reviewed changes

This was referenced May 21, 2020

Proposed 1.6.0-b6 #3409

Closed

Proposed 1.6.0-b6 #3411

Closed

manojsdoshi mentioned this pull request May 27, 2020

Proposed 1.6.0-b6 #3413

Merged

manojsdoshi closed this in #3413 May 27, 2020

seelabs deleted the cleanups branch May 27, 2020 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup move semantics and other misc cleanups #3348

Cleanup move semantics and other misc cleanups #3348

seelabs commented Apr 10, 2020

codecov-io commented Apr 10, 2020 •

edited

Loading

HowardHinnant Apr 10, 2020

scottschurr Apr 11, 2020

HowardHinnant Apr 11, 2020

nbougalis Apr 11, 2020

seelabs Apr 12, 2020

cjcobb23 Apr 14, 2020

seelabs Apr 14, 2020

seelabs Apr 14, 2020

seelabs Apr 14, 2020 •

edited

Loading

thejohnfreeman Apr 18, 2020

nbougalis Apr 11, 2020

nbougalis Apr 17, 2020

seelabs Apr 17, 2020

nbougalis Apr 11, 2020

cjcobb23 Apr 13, 2020

cjcobb23 left a comment

nbougalis left a comment

nbougalis Apr 17, 2020

HowardHinnant commented Apr 21, 2020

HowardHinnant commented Apr 21, 2020

seelabs commented Apr 22, 2020 •

edited

Loading

seelabs commented Apr 24, 2020

seelabs commented Apr 28, 2020

seelabs commented May 8, 2020

codecov-commenter commented May 26, 2020

Cleanup move semantics and other misc cleanups #3348

Cleanup move semantics and other misc cleanups #3348

Conversation

seelabs commented Apr 10, 2020

codecov-io commented Apr 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcobb23 left a comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HowardHinnant commented Apr 21, 2020

HowardHinnant commented Apr 21, 2020

seelabs commented Apr 22, 2020 • edited Loading

seelabs commented Apr 24, 2020

seelabs commented Apr 28, 2020

seelabs commented May 8, 2020

codecov-commenter commented May 26, 2020

Codecov Report

codecov-io commented Apr 10, 2020 •

edited

Loading

seelabs Apr 14, 2020 •

edited

Loading

seelabs commented Apr 22, 2020 •

edited

Loading