Re: [lmi] [lmi-commits] valyuta/005 4483b09 1/9: Avoid passing 'currency' by const reference

[Greg Chicares]

On 1/20/21 5:09 PM, Vadim Zeitlin wrote:
> On Wed, 20 Jan 2021 00:51:31 -0500 (EST) Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
> 
> GC> commit 4483b09a9bf445c58b1be677221a72fbde4de13f
[...]
> GC>     Avoid passing 'currency' by const reference
> GC>     
> GC>     See:
> GC>       https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in
> GC>     sizeof(currency) equals sizeof(double), so pass by value.
> GC>     
> GC>     This change would appear to make both 32- and 64-bit msw builds 
> slightly
> GC>     slower, so is the guideline appropriate for currency operators?
> 
>  This is very, very astonishing. Are the benchmark results reproducible for
> you?

This seems like that supraluminal-neutrino finding a few years ago.
It'd be nice to see it disproven, but I guess everyone already knew
it wasn't going to be reproducible. Instead of trying to reproduce
those 'make cli_timing' results, I might add a unit test that measures
the speed of some realistic combination of these operators, e.g.:

  currency c0 {whatever}, c1 {something else}, c2;
  c2 += c0 + c1 - 3 * c0;
  assert(c2 == 2 * -c0 + c1);

>  I'd expect gcc to optimize away the references anyhow with any -O options,

[and of course I'm using '-O2']

> so I'm surprised that the results are not exactly the same, but I just
> can't explain how could they differ in this direction. I may look at the
> generated assembly, if you'd like, because this is just too strange.

Good idea--that's better than measuring how long even a tiny
unit test takes.

Provided that the generated machine code is the same, would you
favor retaining this change, e.g.:
  +inline bool operator==(currency lhs, currency rhs)
  -inline bool operator==(currency const& lhs, currency const& rhs)
and thus reverting the later commit b835c6d99 that reverted it
with this rationale:

    Undid changes to 'currency.hpp' but not to 'round_to.hpp', and updated
    'Speed*'. The same guidelines [cited in 4483b09]:
      https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rt-complete
    say
      bool operator==(const Minimal&, const Minimal&);
    which might be read as making an exception for operator functions; but
    the real reason is that faster is better than slower, if the slight
    measured difference is credible, else that unconventional and not faster
    is not better than conventional.

IOW, while I doubt that my measurements are meaningful, should we
follow the spirit and not the letter of the Stroustrup-Sutter
guidelines, reasoning that they really meant to write
-   bool operator==(const Minimal&, const Minimal&);
+   bool operator==(Minimal, Minimal);
? Do we abandon ancient convention when even these demigods don't?