Re: [lmi] Contradictory performance measurements

[Greg Chicares]

On 4/8/21 5:29 PM, Vadim Zeitlin wrote:
> 
> GC> I'm surprised it runs for you. Here, I get:
> GC>   Database key 'MinIssSpecAmt: value 50000 not preserved in conversion to 
> 5e+06 cents.
> GC> The reason is that, given a double D whose value is an exact
> GC> integer congruent to 0 (mod 100),
> GC>   D / 100.0
> GC> is an exact integer, but
> GC>   D * 0.01
> GC> is not.
> 
>  I'd rerun product_files and ran lmi_cli --selftest, nothing else. Do you
> see this in the selftest run too? If so, I should probably investigate why
> I don't see it here -- but I definitely don't.

I think I can explain why. That error message appears for
i686 but not x86_64, and I think you tested only the latter.

It took me a little while to track down the i686 issue.
It occurs in this code:

    dst = round_to_cents.c(z);
    if(dblize(dst) != z)

so I tried printing the values that differ.

Both 'z' and 'dblize(dst)' print as exactly 50000, even
with "<< setprecision(DECIMAL_DIG)", but their difference
does not print as zero. I needed to cast them to type
'long double' first in order to make them print differently:

--8<----8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/database.cpp b/database.cpp
index 8fc29a6c3..5bdb74ee4 100644
--- a/database.cpp
+++ b/database.cpp
@@ -37,6 +37,8 @@
 #include "yare_input.hpp"
 
 #include <algorithm>                    // min()
+#include <cfloat>
+#include <iomanip>
 
 /// Construct from essential input (product and axes).
 
@@ -144,6 +146,11 @@ void product_database::query_into(e_database_key k, 
currency& dst) const
             << ": value " << z
             << " not preserved in conversion"
             << " to " << dst.cents() << " cents."
+            << std::setprecision(DECIMAL_DIG)
+            << '\n' << dblize(dst)
+            << '\n' << z
+            << '\n' << static_cast<long double>(dblize(dst))
+            << '\n' << static_cast<long double>(z)
             << LMI_FLUSH
             ;
         }
--8<----8<----8<----8<----8<----8<----8<----8<----8<--

Database key 'MinIssSpecAmt: value 50000 not preserved in conversion to 5e+06 
cents.
50000
50000
50000.0000000000010409
50000
[database.cpp : 154]

so the difference is in the seventeenth significant digit:

  00000 000011111111112222222 significant...
  12345 678901234567890123456 ...digits
                   |
  50000.0000000000010409

and I guess that means the compiler is leaving the result
in an 80-bit register as an "optimization". It's not the
classic "double rounding" issue--instead, the problem is
that the result isn't rounded to 'double' at all.

Then I tried

-    if(dblize(dst) != z)
+    double volatile xyzzy = dblize(dst);
+    if(xyzzy != z)

which "fixed" that problem...but then I got

Database key 'GuarMonthlyPolFee', duration 0: value 8 not preserved in 
conversion to 800 cents.
[database.cpp : 183]

But that's happening on a different line, which can
be fixed similarly (below), and then the speed test
runs to completion, and of course it's faster.

However:

$make $coefficiency system_test
[...]
*** System test failed ***
  1504 system-test files compared
  269 system-test files match
  1235 system-test files differ
  0 system-test files missing
...system test completed.

and therefore this isn't suitable for production.

Here's the complete set of experimental patches (skip them:
I'm just copying them here before wiping them out):

--8<----8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/currency.hpp b/currency.hpp
index ce04d75a2..a73a2b627 100644
--- a/currency.hpp
+++ b/currency.hpp
@@ -40,6 +40,7 @@ class currency
 
     static constexpr int    cents_digits     = 2;
     static constexpr double cents_per_dollar = 100.0;
+    static constexpr double cents_per_dollar_inv = 0.01;
 
   public:
     using data_type = double;
@@ -58,8 +59,7 @@ class currency
 
     data_type cents() const {return m_;}
     // CURRENCY !! add a unit test for possible underflow
-    // CURRENCY !! is multiplication by reciprocal faster or more accurate?
-    double d() const {return m_ / cents_per_dollar;}
+    double d() const {return m_ * cents_per_dollar_inv;}
 
   private:
     explicit currency(data_type z, raw_cents) : m_ {z} {}
diff --git a/database.cpp b/database.cpp
index 8fc29a6c3..36beca4ab 100644
--- a/database.cpp
+++ b/database.cpp
@@ -37,6 +37,8 @@
 #include "yare_input.hpp"
 
 #include <algorithm>                    // min()
+#include <cfloat>
+#include <iomanip>
 
 /// Construct from essential input (product and axes).
 
@@ -137,13 +139,21 @@ void product_database::query_into(e_database_key k, 
currency& dst) const
     double z;
     query_into(k, z);
     dst = round_to_cents.c(z);
-    if(dblize(dst) != z)
+    double volatile xyzzy = dblize(dst);
+    if(xyzzy != z)
         {
         alarum()
             << "Database key '" << db_name_from_key(k)
             << ": value " << z
             << " not preserved in conversion"
             << " to " << dst.cents() << " cents."
+            << std::setprecision(DECIMAL_DIG)
+            << '\n' << dblize(dst)
+            << '\n' << z
+            << '\n' << xyzzy
+            << '\n' << static_cast<long double>(dblize(dst))
+            << '\n' << static_cast<long double>(z)
+            << '\n' << static_cast<long double>(xyzzy)
             << LMI_FLUSH
             ;
         }
@@ -162,7 +172,7 @@ void product_database::query_into(e_database_key k, 
std::vector<currency>& dst)
     dst = round_to_cents.c(z);
     for(int j = 0; j < lmi::ssize(z); ++j)
         {
-        if(dblize(dst[j]) != z[j])
+        if(double volatile xyzzy = dblize(dst[j]); xyzzy != z[j])
             {
             alarum()
                 << "Database key '" << db_name_from_key(k)
--8<----8<----8<----8<----8<----8<----8<----8<----8<--

>  BTW, I don't remember any more, is the decision to use double as
> currency::data_type final or do you plan to revisit it, and explore the
> possibility of using (long long) int instead later?

I don't plan to revisit it. I do plan to document the
reasoning, but haven't yet found the time.