code review 56540046: time: use an alternative method of yielding during Over...

time: use an alternative method of yielding during Overflow timer test

Fixes issue 6874.

Use runtime.GC() as a stronger version of runtime.Gosched() which tends to bias the running goroutine in an otherwise idle system. This appears to reduce the worst case number of spins from 600 down to 30 on my 2 core system under high load.

[dfc, 2014-02-01 04:20:10]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[lucio, 2014-02-01 05:14:02]

yeild -> yield

Lucio.

On 2/1/14, dave@cheney.net <dave@cheney.net> wrote:
> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> time: use an alternative method of yielding during Overflow timer test
>
> Fixes issue 6874.
>
> Use runtime.GC() as a stronger version of runtime.Gosched() which tends
> to bias the running goroutine in an otherwise idle system. This appears
> to reduce the worst case number of spins from 600 down to 30 on my 2
> core system under high load.
>
> Please review this at https://codereview.appspot.com/56540046/
>
> Affected files (+9, -1 lines):
>    M src/pkg/time/internal_test.go
>
>
> Index: src/pkg/time/internal_test.go
> ===================================================================
> --- a/src/pkg/time/internal_test.go
> +++ b/src/pkg/time/internal_test.go
> @@ -78,7 +78,15 @@
>   			if Now().After(stop) {
>   				return errors.New("runtime timer stuck: overflow in addtimer")
>   			}
> -			runtime.Gosched()
> +			// Issue 6874. This test previously called runtime.Gosched to try to
> yeild
> +			// to the goroutine servicing t, however the scheduler has a bias
> towards the
> +			// previously running goroutine in an idle system. Combined with high
> load due
> +			// to all CPUs busy running tests t's goroutine could be delayed beyond
>
> the
> +			// timeout window.
> +			//
> +			// Calling runtime.GC() reduces the worst case lantency for scheduling
>
> t by 20x
> +			// under the current Go 1.3 scheduler.
> +			runtime.GC()
>   		}
>   	}
>   }
>
>
> --
> 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-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[iant, 2014-02-01 05:21:17]

I have a feeling that will just cause a different overflow on other systems.

What you think about simply skipping this test if testing.Short()?  It's a good
test for the package but since it's checking for a bug that is in fact fixed it
doesn't seem critical that we run it every time.

[dfc, 2014-02-01 05:28:28]

On Sat, Feb 1, 2014 at 4:21 PM, <iant@golang.org> wrote:

> I have a feeling that will just cause a different overflow on other
> systems.
>

Most likely.


>
> What you think about simply skipping this test if testing.Short()?  It's
> a good test for the package but since it's checking for a bug that is in
> fact fixed it doesn't seem critical that we run it every time.
>

I'm fine with that.


>
>
> https://codereview.appspot.com/56540046/
>

[dvyukov, 2014-02-01 10:50:40]

On 2014/02/01 05:28:28, dfc wrote:
> On Sat, Feb 1, 2014 at 4:21 PM, <mailto:iant@golang.org> wrote:
> 
> > I have a feeling that will just cause a different overflow on other
> > systems.
> >
> 
> Most likely.
> 
> 
> >
> > What you think about simply skipping this test if testing.Short()?  It's
> > a good test for the package but since it's checking for a bug that is in
> > fact fixed it doesn't seem critical that we run it every time.
> >
> 
> I'm fine with that.
> 
> 
> >
> >
> > https://codereview.appspot.com/56540046/
> >



sounds good to me
let's do both -- GC and skip in short mode

[dfc, 2014-02-01 11:00:35]

Hello golang-codereviews@googlegroups.com, lucio.dere@gmail.com,
iant@golang.org, dvyukov@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[dfc, 2014-02-01 11:01:15]

Hello golang-codereviews@googlegroups.com, lucio.dere@gmail.com,
iant@golang.org, dvyukov@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-02-01 17:40:53]

On 2014/02/01 11:01:15, dfc wrote:
> Hello mailto:golang-codereviews@googlegroups.com, mailto:lucio.dere@gmail.com,
> mailto:iant@golang.org, mailto:dvyukov@google.com (cc:
mailto:golang-codereviews@googlegroups.com),
> 
> Please take another look.

I'm not seeing it skip in short mode.

Ian

[dfc, 2014-02-01 22:42:46]

On 2014/02/01 17:40:53, iant wrote:
> On 2014/02/01 11:01:15, dfc wrote:
> > Hello mailto:golang-codereviews@googlegroups.com,
mailto:lucio.dere@gmail.com,
> > mailto:iant@golang.org, mailto:dvyukov@google.com (cc:
> mailto:golang-codereviews@googlegroups.com),
> > 
> > Please take another look.
> 
> I'm not seeing it skip in short mode.
> 
> Ian

I'm sorry, i forgot to add the second file.

[dfc, 2014-02-01 23:14:18]

Hello golang-codereviews@googlegroups.com, lucio.dere@gmail.com,
iant@golang.org, dvyukov@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-02-02 04:55:18]

LGTM

[dfc, 2014-02-02 05:05:19]

*** Submitted as https://code.google.com/p/go/source/detail?r=143fe449d738 ***

time: use an alternative method of yielding during Overflow timer test

Fixes issue 6874.

Use runtime.GC() as a stronger version of runtime.Gosched() which tends to bias
the running goroutine in an otherwise idle system. This appears to reduce the
worst case number of spins from 600 down to 30 on my 2 core system under high
load.

LGTM=iant
R=golang-codereviews, lucio.dere, iant, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/56540046

[gobot, 2014-02-02 05:09:47]

This CL appears to have broken the freebsd-amd64 builder.

[dfc, 2014-02-02 06:21:00]

Ironically this is the *other* bug, the segv during the reflect tests. 

> On 2 Feb 2014, at 16:09, gobot@golang.org wrote:
> 
> This CL appears to have broken the freebsd-amd64 builder.
> 
> https://codereview.appspot.com/56540046/