Make QPCValueToMicroseconds faster in case the value is less than 44 bits

base/time/time_win.cc

Index: base/time/time_win.cc
diff --git a/base/time/time_win.cc b/base/time/time_win.cc
index cf7be687d2e5a0b292414796f1b3c25e2535f518..330ee3d68401d2abf42cb283596a0102a0b88cb1 100644
--- a/base/time/time_win.cc
+++ b/base/time/time_win.cc
@@ -391,17 +391,21 @@ class HighResNowSingleton {
   }
 
   int64 QPCValueToMicroseconds(LONGLONG qpc_value) {
-    if (!ticks_per_second_)

[brianderson, 2014/07/29 23:53:29]

I remember you had an explanation for why this could be removed. Can you explain
again?

[fmeawad, 2014/07/30 00:49:49]

I added that check in the CPUReliablySupportsHighResTime, that is used to set
the function pointer to the correct function.
By moving the check to the setup phase, we guarantee that no calls will have the
ticks_per_second_ = 0

[brianderson, 2014/07/30 00:58:19]

Thanks for the explanation.

I was thinking about asking for a DCHECK to replace the if, but I guess we'll
get a division by zero exception anyway.

-      return 0;
-
-    // Intentionally calculate microseconds in a round about manner to avoid
-    // overflow and precision issues. Think twice before simplifying!
-    int64 whole_seconds = qpc_value / ticks_per_second_;
-    int64 leftover_ticks = qpc_value % ticks_per_second_;
-    int64 microseconds = (whole_seconds * Time::kMicrosecondsPerSecond) +
-                         ((leftover_ticks * Time::kMicrosecondsPerSecond) /
-                          ticks_per_second_);
-    return microseconds;
+    // Time::kMicrosecondsPerSecond has 20 bits, to avoid overflow we do a
+    // simple multiplication then division if the qpc_value has less than 44
+    // bits.
+    if (qpc_value & 0xFFFFF00000000000)

[jar (doing other things), 2014/07/30 23:43:09]

Please unit test at exactly above and below this border.

I *think* this constant might be too big....

should be around 2^63 - 1

...and then a compare, rather than a "&" might get you more mileage.

[jar (doing other things), 2014/07/31 00:01:43]

I think the largest positive 64 bit number is 2^63 - 1
Since you want to multiply by 1,000,000 (number of microseconds per second), to
avoid an overflow, you'll need to avoid being larger than:
(2^63 - 1) / 1E6
That evaluates to: 
9223372036854
in hex that is:
0x8637BD05AF6

Your current test checks to see if the number has any bits in the high end,
which means you're doing a "compare" with:
if (qpc_value >= 0x100000000000)

but the actual test you need is probably:
if (qpc_value >  0x8637BD05AF6)
or
if (qpc_value >= 0x8637BD05AF7)

The unit test should be able to get all this right, and validate correctness
against the "standard" (previous?) code.

WDYT?

[fmeawad, 2014/07/31 18:54:21]

Done.

[fmeawad, 2014/07/31 18:54:21]

Done.

+    {
+      // Intentionally calculate microseconds in a round about manner to avoid
+      // overflow and precision issues. Think twice before simplifying!
+      int64 whole_seconds = qpc_value / ticks_per_second_;
+      int64 leftover_ticks = qpc_value % ticks_per_second_;
+      int64 microseconds = (whole_seconds * Time::kMicrosecondsPerSecond) +
+                           ((leftover_ticks * Time::kMicrosecondsPerSecond) /
+                            ticks_per_second_);
+      return microseconds;
+    }
+    return qpc_value * Time::kMicrosecondsPerSecond / ticks_per_second_;

[brianderson, 2014/07/29 23:53:29]

Would a single double-precision multiplication be faster than an integer
multiply and divide? I think 64-bit integer division takes significantly longer
than a double-precision multiplication on a lot of hardware.

If it's faster, we should be able to apply the same optimization to
leftover_ticks above too.

[fmeawad, 2014/07/30 00:49:49]

I made a quick experiment, and I have found no win. Also for a large sample I
found some loss of percision errors. (1 in 3 million).

[brianderson, 2014/07/30 00:58:19]

Thanks for testing that.

   }
 
  private:
@@ -447,7 +451,8 @@ NowFunction now_function = RolloverProtectedNow;
 
 bool CPUReliablySupportsHighResTime() {
   base::CPU cpu;
-  if (!cpu.has_non_stop_time_stamp_counter())
+  if (!cpu.has_non_stop_time_stamp_counter() ||
+      !GetHighResNowSingleton()->IsUsingHighResClock())
     return false;
 
   if (IsBuggyAthlon(cpu))