code review 7128053: time: Sleep does better job then runtime.Gosched in Tes...

time: Sleep does better job then runtime.Gosched in TestAfterStress

for slow windows-386 builder

[brainman, 2013-01-18 01:16:05]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[dfc, 2013-01-18 01:18:32]

Nice fix, but I wonder if it is masking a deeper issue.
On 18 Jan 2013 12:16, <alex.brainman@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> time: Sleep does better job then runtime.Gosched in TestAfterStress
>
> for slow windows-386 builder
>
> Please review this at
https://codereview.appspot.**com/7128053/<https://codereview.appspot.com/7128...
>
> Affected files:
>   M src/pkg/time/sleep_test.go
>
>
> Index: src/pkg/time/sleep_test.go
> ==============================**==============================**=======
> --- a/src/pkg/time/sleep_test.go
> +++ b/src/pkg/time/sleep_test.go
> @@ -56,7 +56,7 @@
>                         runtime.GC()
>                         // Need to yield, because otherwise
>                         // the main goroutine will never set the stop flag.
> -                       runtime.Gosched()
> +                       Sleep(Nanosecond)
>                 }
>         }()
>         c := Tick(1)
>
>
>

[brainman, 2013-01-18 01:19:28]

This change is, probably, not enough to fix windows-386. It also struggles with
new TestReset. Perhaps, it is possible to make TestReset work by fiddling with
timeout values. But, I think, this pc is running our of puff in general. It was
not fast to start with, but we keep adding new tests.

Alex

[brainman, 2013-01-18 01:20:46]

On 2013/01/18 01:18:32, dfc wrote:
> Nice fix, but I wonder if it is masking a deeper issue.

Sure thing. Explain yourself, please.

Alex

[rsc, 2013-01-18 03:24:18]

LGTM

The problem is that the timer thread is waiting to run (the timer has
gone off but the OS hasn't run the thread in response yet) and so we
don't see a goroutine waiting, and instead we just keep spinning doing
our own work. It's hard for the timer thread to compete with that, and
apparently Windows is okay with letting the one thread run away. It's
not a goroutine scheduler issue.

// Yield so that the OS can wake up the timer thread,
// so that it can generate channel sends for the main goroutine,
// which will eventually set stop = 1 for us.
Sleep(1*Nanosecond)

[brainman, 2013-01-18 04:31:09]

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

time: Sleep does better job then runtime.Gosched in TestAfterStress

for slow windows-386 builder

R=golang-dev, dave, rsc
CC=golang-dev
https://codereview.appspot.com/7128053

[ioe, 2013-01-18 22:16:40]

I know, I'm late in the game.

Maybe the culprit is that the OS and Go scheduler are not aware about the 
atomics.
What about giving the schedulers sth. to sleep/wake on?

So I would suggest sth. like

func TestAfterStress(t *testing.T) {
        stop := make(chan bool)
        go func() {
                       select {
                       case _, _ = <- stop:
                       }
        }()
        c := Tick(1)
        for i := 0; i < 100; i++ {
                <-c
        }
        close(stop)
}

[brainman, 2013-01-18 23:29:51]

On 2013/01/18 22:16:40, ioe wrote:
> 
> Maybe the culprit is that the OS and Go scheduler are not aware about the 
> atomics.
> What about giving the schedulers sth. to sleep/wake on?
> 

I am not 100% clear what is been tested here in the first place. So it is hard
for me to comment on your proposed approach.

Alex

[rsc, 2013-01-22 18:25:11]

I think what's there now is fine. Let's do more important things.

Russ