[go] time: use 1e9 rather than 1e-9 in Duration calculations

553 views
Skip to first unread message

Ian Lance Taylor (Gerrit)

unread,
Nov 10, 2016, 8:11:46 PM11/10/16
to golang-co...@googlegroups.com

Ian Lance Taylor uploaded time: use 1e9 rather than 1e-9 in Duration calculations for review.

View Change

time: use 1e9 rather than 1e-9 in Duration calculations

1e-9 has a 1 in the last place, causing some Duration calculations to
have unnecessary rounding errors.  1e9 does not, so use that instead.

Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
---
M src/time/time.go
M src/time/time_test.go
2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/time/time.go b/src/time/time.go
index 03fde33..bee7e59 100644
--- a/src/time/time.go
+++ b/src/time/time.go
@@ -592,21 +592,21 @@
 func (d Duration) Seconds() float64 {
 	sec := d / Second
 	nsec := d % Second
-	return float64(sec) + float64(nsec)*1e-9
+	return float64(sec) + float64(nsec)/1e9
 }
 
 // Minutes returns the duration as a floating point number of minutes.
 func (d Duration) Minutes() float64 {
 	min := d / Minute
 	nsec := d % Minute
-	return float64(min) + float64(nsec)*(1e-9/60)
+	return float64(min) + float64(nsec)/(60*1e9)
 }
 
 // Hours returns the duration as a floating point number of hours.
 func (d Duration) Hours() float64 {
 	hour := d / Hour
 	nsec := d % Hour
-	return float64(hour) + float64(nsec)*(1e-9/60/60)
+	return float64(hour) + float64(nsec)/(60*60*1e9)
 }
 
 // Add returns the time t+d.
diff --git a/src/time/time_test.go b/src/time/time_test.go
index 0af9da3..04ad546 100644
--- a/src/time/time_test.go
+++ b/src/time/time_test.go
@@ -1003,6 +1003,21 @@
 	}
 }
 
+var secDurationTests = []struct {
+	d    Duration
+	want float64
+}{
+	{Duration(300000000), 0.3},
+}
+
+func TestDurationSeconds(t *testing.T) {
+	for _, tt := range secDurationTests {
+		if got := tt.d.Seconds(); got != tt.want {
+			t.Errorf("d.Seconds() = %g; want: %g", got, tt.want)
+		}
+	}
+}
+
 var minDurationTests = []struct {
 	d    Duration
 	want float64
@@ -1011,6 +1026,7 @@
 	{Duration(-1), -1 / 60e9},
 	{Duration(1), 1 / 60e9},
 	{Duration(60000000000), 1},
+	{Duration(3000), 5e-8},
 }
 
 func TestDurationMinutes(t *testing.T) {
@@ -1029,6 +1045,7 @@
 	{Duration(-1), -1 / 3600e9},
 	{Duration(1), 1 / 3600e9},
 	{Duration(3600000000000), 1},
+	{Duration(36), 1e-11},
 }
 
 func TestDurationHours(t *testing.T) {
@@ -1117,8 +1134,8 @@
 
 	{"Truncate", func(t1, t2 Time) bool { return t1.Truncate(Hour).Equal(t2.Truncate(Hour)) }},
 	{"Round", func(t1, t2 Time) bool { return t1.Round(Hour).Equal(t2.Round(Hour)) }},
-	
-	{"== Time{}", func(t1, t2 Time) bool { return (t1==Time{}) == (t2==Time{}) }},
+
+	{"== Time{}", func(t1, t2 Time) bool { return (t1 == Time{}) == (t2 == Time{}) }},
 }
 
 func TestDefaultLoc(t *testing.T) {

To view, visit this change. To unsubscribe, visit settings.

Gerrit-MessageType: newchange
Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
Gerrit-PatchSet: 1
Gerrit-Project: go
Gerrit-Branch: master
Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>

Gobot Gobot (Gerrit)

unread,
Nov 10, 2016, 8:12:12 PM11/10/16
to Ian Lance Taylor, golang-co...@googlegroups.com

Gobot Gobot posted comments on time: use 1e9 rather than 1e-9 in Duration calculations.

View Change

Patch Set 1: TryBots beginning. Status page: http://farmer.golang.org/try?commit=74597a03

    To view, visit this change. To unsubscribe, visit settings.

    Gerrit-MessageType: comment


    Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
    Gerrit-PatchSet: 1
    Gerrit-Project: go
    Gerrit-Branch: master
    Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>

    Gerrit-HasComments: No

    Gobot Gobot (Gerrit)

    unread,
    Nov 10, 2016, 8:17:51 PM11/10/16
    to Ian Lance Taylor, golang-co...@googlegroups.com

    Gobot Gobot posted comments on time: use 1e9 rather than 1e-9 in Duration calculations.

    View Change

    Patch Set 1: TryBot-Result+1 TryBots are happy.

      To view, visit this change. To unsubscribe, visit settings.

      Gerrit-MessageType: comment


      Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
      Gerrit-PatchSet: 1
      Gerrit-Project: go
      Gerrit-Branch: master

      Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

      Gerrit-HasComments: No

      Brad Fitzpatrick (Gerrit)

      unread,
      Nov 11, 2016, 7:25:21 PM11/11/16
      to Ian Lance Taylor, Brad Fitzpatrick, Gobot Gobot, golang-co...@googlegroups.com

      Brad Fitzpatrick posted comments on time: use 1e9 rather than 1e-9 in Duration calculations.

      View Change

      Patch Set 1: Code-Review+2 I assume it was written the original way for performance, to avoid the divide. Does that matter? Probably not much.

        To view, visit this change. To unsubscribe, visit settings.

        Gerrit-MessageType: comment


        Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
        Gerrit-PatchSet: 1
        Gerrit-Project: go
        Gerrit-Branch: master

        Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

        Gerrit-HasComments: No

        Ian Lance Taylor (Gerrit)

        unread,
        Nov 11, 2016, 8:18:31 PM11/11/16
        to golang-...@googlegroups.com, Brad Fitzpatrick, Gobot Gobot, golang-co...@googlegroups.com

        Ian Lance Taylor merged time: use 1e9 rather than 1e-9 in Duration calculations.

        View Change

        time: use 1e9 rather than 1e-9 in Duration calculations
        
        1e-9 has a 1 in the last place, causing some Duration calculations to
        have unnecessary rounding errors.  1e9 does not, so use that instead.
        
        Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
        Reviewed-on: https://go-review.googlesource.com/33116
        Run-TryBot: Ian Lance Taylor <ia...@golang.org>
        TryBot-Result: Gobot Gobot <go...@golang.org>
        Reviewed-by: Brad Fitzpatrick <brad...@golang.org>
        ---
        M src/time/time.go
        M src/time/time_test.go
        2 files changed, 20 insertions(+), 3 deletions(-)
        
        
        Approvals:
          Brad Fitzpatrick: Looks good to me, approved
          Ian Lance Taylor: Run TryBots
          Gobot Gobot: TryBots succeeded
        
        
        diff --git a/src/time/time.go b/src/time/time.go
        index 175c9a9..00fafb6 100644
        --- a/src/time/time.go
        +++ b/src/time/time.go
        @@ -603,21 +603,21 @@
         func (d Duration) Seconds() float64 {
         	sec := d / Second
         	nsec := d % Second
        -	return float64(sec) + float64(nsec)*1e-9
        +	return float64(sec) + float64(nsec)/1e9
         }
         
         // Minutes returns the duration as a floating point number of minutes.
         func (d Duration) Minutes() float64 {
         	min := d / Minute
         	nsec := d % Minute
        -	return float64(min) + float64(nsec)*(1e-9/60)
        +	return float64(min) + float64(nsec)/(60*1e9)
         }
         
         // Hours returns the duration as a floating point number of hours.
         func (d Duration) Hours() float64 {
         	hour := d / Hour
         	nsec := d % Hour
        -	return float64(hour) + float64(nsec)*(1e-9/60/60)
        +	return float64(hour) + float64(nsec)/(60*60*1e9)
         }
         
         // Add returns the time t+d.
        diff --git a/src/time/time_test.go b/src/time/time_test.go
        index 07afcff..2922560 100644
        --- a/src/time/time_test.go
        +++ b/src/time/time_test.go
        @@ -1003,6 +1003,21 @@
         	}
         }
         
        +var secDurationTests = []struct {
        +	d    Duration
        +	want float64
        +}{
        +	{Duration(300000000), 0.3},
        +}
        +
        +func TestDurationSeconds(t *testing.T) {
        +	for _, tt := range secDurationTests {
        +		if got := tt.d.Seconds(); got != tt.want {
        +			t.Errorf("d.Seconds() = %g; want: %g", got, tt.want)
        +		}
        +	}
        +}
        +
         var minDurationTests = []struct {
         	d    Duration
         	want float64
        @@ -1011,6 +1026,7 @@
         	{Duration(-1), -1 / 60e9},
         	{Duration(1), 1 / 60e9},
         	{Duration(60000000000), 1},
        +	{Duration(3000), 5e-8},
         }
         
         func TestDurationMinutes(t *testing.T) {
        @@ -1029,6 +1045,7 @@
         	{Duration(-1), -1 / 3600e9},
         	{Duration(1), 1 / 3600e9},
         	{Duration(3600000000000), 1},
        +	{Duration(36), 1e-11},
         }
         
         func TestDurationHours(t *testing.T) {
        

        To view, visit this change. To unsubscribe, visit settings.

        Gerrit-MessageType: merged
        Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
        Gerrit-PatchSet: 2
        Gerrit-Project: go
        Gerrit-Branch: master
        Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>Gerrit-Reviewer: Gobot Gobot <go...@golang.org>Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

        Rob Pike (Gerrit)

        unread,
        Nov 12, 2016, 10:34:08 AM11/12/16
        to Ian Lance Taylor, Brad Fitzpatrick, Gobot Gobot, golang-co...@googlegroups.com, Rob Pike

        Rob Pike posted comments on time: use 1e9 rather than 1e-9 in Duration calculations.

        View Change

        Patch Set 2: I'm curious to know more. There is no issue filed, and I would think that the compiler might generate the same code in both cases. Mysterious.

          To view, visit this change. To unsubscribe, visit settings.

          Gerrit-MessageType: comment
          Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
          Gerrit-PatchSet: 2
          Gerrit-Project: go
          Gerrit-Branch: master


          Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>Gerrit-Reviewer: Gobot Gobot <go...@golang.org>Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

          Gerrit-HasComments: No

          Michael Jones

          unread,
          Nov 12, 2016, 11:27:45 AM11/12/16
          to r...@golang.org, Ian Lance Taylor, Brad Fitzpatrick, Gobot Gobot, golang-co...@googlegroups.com

          Side comments:

           

          float64(1e9) is 0x41CDCD6500000000

          which represents 1.86264514923095703125 * 2^29

          aka, 1.00000000000000000000 * 10^9

           

          float64(1e-9) is 0x3E112E0BE826D695

          which represents 1.0737418240000000668743496134993620216846 * 2^-30

          aka, 1.0000000000000000622815914577798564188970 * 10^-9

           

          The difference observed, naturally, is the difference between “multiplies before divisions” (the numerical analysts mantra) and “avoid divides” (the cycle-counter’s mantra).

           

          Since {1/(10^n), n integer > 0} is never exact in binary floating point, the issue is about the numerator size. If the numerator is big (a minute as 60 billion nanoseconds, for example) then the inexact representation of the 1e-9 will be hidden by the rounding after the multiply.

           

          1/1.0000000000000000622815914577798564188970 * 10^-9 is

          9.9999999999999993771840854222014746009961 * 10^8

          that is, 10^9 - 6.22815914577798525399004*10^-8

          so you have about 7 decimal places down there before you begin to see the problem with small numerators.

           

          The real issue is not this instance, but the notion of using 1/x as an optimization; it works, but is a double-edged sword that cuts both ways.

           

          Michael “The multiplies before divides instruction scheduler” Jones

          --
          You received this message because you are subscribed to the Google Groups "golang-codereviews" group.
          To unsubscribe from this group and stop receiving emails from it, send an email to golang-coderevi...@googlegroups.com.
          For more options, visit https://groups.google.com/d/optout.

          Martin Moehrmann (Gerrit)

          unread,
          Nov 12, 2016, 5:49:24 PM11/12/16
          to Ian Lance Taylor, Brad Fitzpatrick, Gobot Gobot, golang-co...@googlegroups.com

          Martin Moehrmann posted comments on time: use 1e9 rather than 1e-9 in Duration calculations.

          View Change

          Patch Set 2: The old version uses MULSD and the new version uses DIVSD. I do not think its possible for the compiler to optimize divs to muls in general for floating point arithmetic with constants without loss of precision and with a speedup. For some divisions by powers of 2 it might be possible.

            To view, visit this change. To unsubscribe, visit settings.

            Gerrit-MessageType: comment
            Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
            Gerrit-PatchSet: 2
            Gerrit-Project: go
            Gerrit-Branch: master


            Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>Gerrit-Reviewer: Gobot Gobot <go...@golang.org>Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

            Gerrit-HasComments: No

            Ian Lance Taylor (Gerrit)

            unread,
            Nov 12, 2016, 7:00:25 PM11/12/16
            to Brad Fitzpatrick, Gobot Gobot, golang-co...@googlegroups.com

            Ian Lance Taylor posted comments on time: use 1e9 rather than 1e-9 in Duration calculations.

            View Change

            Patch Set 2: > I'm curious to know more. There is no issue filed, and I would think that the compiler might generate the same code in both cases. Mysterious. The bug was filed internally to Google. I didn't bother replicating it outside, because the test cases added in this CL fail before the patch and succeed after the patch. The compiler won't generate the same code--unlike with integers you generally can't replace floating point division with multiplication, precisely because of these kinds of rounding issues. The rounding problem can be seen by fmt.Printf("%b %b\n", 1e9, 1e-9) which produces 8388608000000000p-23 4835703278458517p-82 Math combining the latter with values like .3 that seem simple to people but also have many bits sets in the floating point representation gives great scope for unexpected values in the last bit. The former value, 1e9, generally avoids such problems. It's not a panacea but makes simple values more likely to remain simple.

              To view, visit this change. To unsubscribe, visit settings.

              Gerrit-MessageType: comment
              Gerrit-Change-Id: I96334a2c47e7a014b532eb4b8a3ef9550e7ed057
              Gerrit-PatchSet: 2
              Gerrit-Project: go
              Gerrit-Branch: master


              Gerrit-Owner: Ian Lance Taylor <ia...@golang.org>Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>Gerrit-Reviewer: Gobot Gobot <go...@golang.org>Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>

              Gerrit-HasComments: No

              Reply all
              Reply to author
              Forward
              0 new messages