Re: [lmi] Is it unhelpful to move containers explicitly?

[Vadim Zeitlin]

On Sun, 7 Oct 2018 15:24:50 +0000 Greg Chicares <address@hidden> wrote:

GC> As an experiment, I changed make_evaluator() to pass vector
GC> parameters as const& instead of &&, and the measured speed did
GC> not discernibly change. Perhaps the operation of passing these
GC> parameters is just a negligible portion of the work done by
GC> make_evaluator(),

 Yes, I'm pretty sure this is the case. This vector doesn't have that many
elements and it's copied only once, AFAICS, so it's not going to take any
significant time on its own.

GC> but even in that case wouldn't it be better to simplify the code by not
GC> writing std::move()?

 I don't think so. This would be a premature pessimization, similar to
passing a vector by value instead of const reference when you don't need to
copy it. Sure, you'd have trouble seeing the effect of copying a single
vector in any non-trivial program as well, but the cumulative effect of
copying all the vectors would be quite noticeable, both in time and memory
consumption, which is why we don't do this. And for the same reason we
should avoid unnecessary copies in situations such as this, when we can
move instead.

 The difference will certainly turn out to be trivial in this case but,
again, these small differences do add up and it costs nothing to avoid
them, so I'm still quite convinced that we should use std::move() where it
makes sense, regardless of whether we see a noticeable performance
advantage of using it in each particular case. It makes our own life
simpler as we don't have to worry about benchmarking each and every
function, but can just write the code matching what makes semantic sense:
if a function should/can consume its arguments, then it should take them by
r-value reference and the caller should use std::move() when calling it
with non-temporaries.

GC> See:
GC> 
GC>   
https://isocpp.org/blog/2013/02/no-really-moving-a-return-value-is-easy-stackoverflow
GC> | often the cleanest, simplest code that doesn't even mention
GC> | move or && anywhere is just what you want

 Sorry, but this was said in a completely different context. The question
above is about _returning_ objects from functions, not passing them into
them and there is no implicit optimization (even in practice, let alone
guaranteed by the standard) similar to RVO that applies in the latter case.

GC> the idea being, AIUI, that standard containers already have move
GC> ctors and C++1X compilers will automatically use them in copy
GC> elision if beneficial, even if we don't invoke them explicitly.

 No, the compiler will never do this except for the special case of RVO
which is not really using about using move ctor instead of the copy ctor
but rather avoiding constructing the temporary in any case.

GC> I ran 'ledger_test' ten times--without, and then with, the
GC> patch below, thus:
GC> 
GC> /opt/lmi/src/build/lmi/Linux/gcc/ship[0]$for z in {1..10}; do wine 
./ledger_test.exe -a |grep evaluator; done
GC> 
GC> and here are totals of the ten runs, reporting two statistics:
GC>  - mean run time for the seven runs that can be done in one second
GC>  - lowest run time of those seven (probably more robust than mean):
GC> 
GC>     mean    least
GC>   1.5363  1484521 explicit move (without patch)
GC>   1.5305  1487392 with patch

 It's actually amazing that there is any difference at all, but I'd chalk
it up to the measurement error because actually no moves are being done
even with the current code because I forgot another std::move() :-( To make
the moves really happen, please apply the following patch:

---------------------------------- >8 --------------------------------------
diff --git a/ledger_evaluator.hpp b/ledger_evaluator.hpp
index 1a2230bc7..ca211dc1b 100644
--- a/ledger_evaluator.hpp
+++ b/ledger_evaluator.hpp
@@ -30,6 +30,7 @@
 
 #include <string>
 #include <unordered_map>
+#include <utility>                      // move()
 #include <vector>
 
 /// Class allowing to retrieve the string representation of any scalar or
@@ -52,8 +53,8 @@ class LMI_SO ledger_evaluator
   private:
     // Constructible only by friends: see Ledger::make_evaluator().
     ledger_evaluator(scalar_map_t&& scalars, vector_map_t&& vectors)
-        :scalars_ {scalars}
-        ,vectors_ {vectors}
+        :scalars_ {std::move(scalars)}
+        ,vectors_ {std::move(vectors)}
     {
     }
---------------------------------- >8 --------------------------------------

 I'd be somewhat curious if this makes any noticeable difference but even
if it doesn't, I think this patch should still be applied instead of the
original one because it does make sense to move the maps into the evaluator
here: it takes ownership of them, if you'd like.
 
 Sorry for forgetting the crucial std::move() which might have skewed your
testing results,
VZ