code review 4777043: test: disable sigchld test on Windows

test: disable sigchld test on Windows

Alex Brainman reports that this is the only test
that keeps us from running test/run.

[rsc, 2011-07-19 14:26:09]

Hello brainman (cc: golang-dev@googlegroups.com),

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

[lucio, 2011-07-19 14:30:35]

On 7/19/11, rsc@golang.org <rsc@golang.org> wrote:
> Reviewers: brainman,
>
> Message:
> Hello brainman (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> test: disable sigchld test on Windows
>
> Alex Brainman reports that this is the only test
> that keeps us from running test/run.
>
> Please review this at http://codereview.appspot.com/4777043/
>
> Affected files:
>    M src/run.bash
>    M test/sigchld.go
>
>
> Index: src/run.bash
> ===================================================================
> --- a/src/run.bash
> +++ b/src/run.bash
> @@ -93,7 +93,6 @@
>   ./timing.sh -test
>   ) || exit $?
>
> -[ "$GOHOSTOS" == windows ] ||
>   (xcd ../test
>   ./run
>   ) || exit $?
> Index: test/sigchld.go
> ===================================================================
> --- a/test/sigchld.go
> +++ b/test/sigchld.go
> @@ -1,3 +1,4 @@
> +// [ "$GOOS" = windows ] ||
>   // $G $D/$F.go && $L $F.$A && ./$A.out
>
>   // Copyright 2009 The Go Authors. All rights reserved.
>
>
>

[lucio, 2011-07-19 15:00:16]

Huh?  Could I have pressed the wrong button?  Which one?

Lucio.

On 7/19/11, Lucio De Re <lucio.dere@gmail.com> wrote:
> On 7/19/11, rsc@golang.org <rsc@golang.org> wrote:
>> Reviewers: brainman,
>>
>> Message:
>> Hello brainman (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg
>>
>>
>> Description:
>> test: disable sigchld test on Windows
>>
>> Alex Brainman reports that this is the only test
>> that keeps us from running test/run.
>>
>> Please review this at http://codereview.appspot.com/4777043/
>>
>> Affected files:
>>    M src/run.bash
>>    M test/sigchld.go
>>
>>
>> Index: src/run.bash
>> ===================================================================
>> --- a/src/run.bash
>> +++ b/src/run.bash
>> @@ -93,7 +93,6 @@
>>   ./timing.sh -test
>>   ) || exit $?
>>
>> -[ "$GOHOSTOS" == windows ] ||
>>   (xcd ../test
>>   ./run
>>   ) || exit $?
>> Index: test/sigchld.go
>> ===================================================================
>> --- a/test/sigchld.go
>> +++ b/test/sigchld.go
>> @@ -1,3 +1,4 @@
>> +// [ "$GOOS" = windows ] ||
>>   // $G $D/$F.go && $L $F.$A && ./$A.out
>>
>>   // Copyright 2009 The Go Authors. All rights reserved.
>>
>>
>>
>

[brainman, 2011-07-19 23:41:29]

Tried it:

$ time ./run
./run: line 42: ulimit: virtual memory: cannot modify limit: Invalid argument
fail: ./sigchld.go
47c47
< survived SIGCHLD
---
> cannot open file: sigchld.8
0 known bugs; 0 unexpected bugs; test output differs
FAILED

real    9m42.358s
user    0m30.091s
sys     1m42.747s

$

It doesn't work as is. I would try and use os *Process.Kill instead to implement
it. I'm happy to have a go.

I am more concerned abouth the time it takes to run this test. This is extra 10
minutes (your millage might vary) for every build. Our windows go builder is
slowest to finish as is, this change will make it even longer. Perhaps we could
enable this test, but run it condition of an environment variable (like I do
with DISABLE_CGO). I will set this variable for windows gobuilder, so it doesn't
run this test. Alternatively, we could make gobuilder run this test, but only
once in a while.

Alex

[brainman, 2011-07-20 00:41:23]

I lied. I can't make it work on windows.

os *Process.Kill will terminate process immediately and unconditionally. We want
the opposite, because it is SIGCHLD - we want it to be ignored. There is no such
thing on Windows.

The closest I can think of is RaiseException
(http://msdn.microsoft.com/en-us/library/ms680552%28v=vs.85%29.aspx). But all
this will do, it will call our exception handler. Our exception handler just
terminates process at this moment (only EXCEPTION_BREAKPOINT is treated
differently). We would have to change it. But I'm not sure what to do, just
ignore an exception?

I think you were correct from the start. This test is too unix specific. We
would just write it in such way that test PASSes on windows.

Alex

[rsc, 2011-07-20 00:46:02]

We can just delete the test if that will make the directory work.
There is enough other signal handling going on now that it
is not as helpful as it once was.

[brainman, 2011-07-20 00:48:14]

On 2011/07/20 00:46:02, rsc wrote:
> We can just delete the test ...

That should do the trick.
What about extra time to run the test in windows gobuilder?

[bradfitz, 2011-07-20 00:51:35]

On Tue, Jul 19, 2011 at 5:48 PM, <alex.brainman@gmail.com> wrote:

> On 2011/07/20 00:46:02, rsc wrote:
>
>> We can just delete the test ...
>>
>
> That should do the trick.
> What about extra time to run the test in windows gobuilder?


It kinda sucks on Linux too.  Windows having slow process creation just
exacerbates the problem. Rather than the test/run script, we should have a
Go harness that runs them in parallel and interleaves any error output
sanely.

[brainman, 2011-07-20 01:13:04]

On 2011/07/20 00:51:35, bradfitz wrote:
> 
> ... Rather than the test/run script, we should have a
> Go harness that runs them in parallel and interleaves any error output
> sanely.

I agree. I suspect it would improve speed. But we need to try to see it.

Also, if we reshuffle this process, we might solve another issue that I'm stuck
with at this moment: http://code.google.com/p/go/issues/detail?id=1899. This
will certainly bite us once we start using Windows 7 on fast PCs.

I have done some work in rewriting this in go, but only the Perl part. I stopped
working on it, because we have perl in mingw now. But I could bring it back.

Also, people with slow PCs will suffer anyways.

Alex

[rsc, 2011-07-20 01:17:25]

>> That should do the trick.
>> What about extra time to run the test in windows gobuilder?
>
> It kinda sucks on Linux too.  Windows having slow process creation just
> exacerbates the problem. Rather than the test/run script, we should have a
> Go harness that runs them in parallel and interleaves any error output
> sanely.

I really hope we can not spend time on this.
Making the test/run directory depend on Go to
run the tests is going to really suck for
bootstrapping.

It's just cycles.

[brainman, 2011-07-20 01:52:29]

On 2011/07/20 00:46:02, rsc wrote:
> There is enough other signal handling going on ...

While we're talking about syscall.Kill, I looked at time package. One of the
tests that still fails on windows is TestSleep. TestSleep does
syscall.Kill(os.Getpid(), syscall.SIGCHLD) and expects time.Sleep to exit early.
This will not work on windows for the same reason. On the other hand we were
talking about redoing time.Sleep so it doesn't occupy os thread. Perhaps, we
could go ahead with this plan and rewrite TestSleep at the same time. I have no
idea where to start, but if you give me some pointers, I will have a go.

Alex

[hector, 2011-09-20 15:14:57]

Is there any reason why this can't go in?  I don't mind gobuilder taking longer
if it means better test coverage.  I think the diff needs to delete sigchld.go,
though.

[brainman, 2011-09-20 23:41:19]

On 2011/09/20 15:14:57, hector wrote:
> Is there any reason why this can't go in?  I don't mind gobuilder taking
longer
> if it means better test coverage.  ...

"Gobuilder taking longer" is the only reason I know. I do not like the extra
time it will take to see builder succeed or fail. In my estimation this will add
~10 minutes to every build (It takes ~15 minutes to build windows build now).
That means extra ~20 minutes (currently building build to finish and newest
build to start and complete) wait to see if you submit fails or succeeds. If
that does not bother anyone, then we should proceed. I will go along with
whatever others will decide.

Alex

[rsc, 2011-09-21 00:34:51]

> "Gobuilder taking longer" is the only reason I know. I do not like the
> extra time it will take to see builder succeed or fail. In my estimation
> this will add ~10 minutes to every build (It takes ~15 minutes to build
> windows build now). That means extra ~20 minutes (currently building
> build to finish and newest build to start and complete) wait to see if
> you submit fails or succeeds. If that does not bother anyone, then we
> should proceed. I will go along with whatever others will decide.

What takes so long?  Running the tests leaves
a $GOROOT/test/timing.log file that shows what
took the time, assuming it is not the compilation
and linking.

Russ

[brainman, 2011-09-21 00:57:50]

On 2011/09/21 00:34:51, rsc wrote:
> 
> What takes so long?  ...

$ time run
./run: line 6: gomake: command not found
./run: line 42: ulimit: virtual memory: cannot modify limit: Invalid argument
fail: ./sigchld.go
43c43,44
< survived SIGCHLD
---
> ./sigchld.go:12: undefined: syscall.Kill
> ./sigchld.go:12: undefined: syscall.SIGCHLD
1 known bugs; 0 unexpected bugs; test output differs
FAILED

real    7m30.793s
user    0m37.616s
sys     2m28.080s

$ cat times.out
0.68 ./235
26.76 ./64bit
3.14 ./append
0.29 ./args
1.95 ./assign
0.20 ./assign1
0.20 ./bigalg
0.18 ./bigmap
0.56 ./blank
0.20 ./blank1
0.20 ./chancap
0.29 ./char_lit
0.18 ./char_lit1
0.25 ./closedchan
0.18 ./closure
0.20 ./cmp1
0.15 ./cmp2
0.18 ./cmp3
0.20 ./cmp4
0.18 ./cmp5
0.18 ./cmp6
0.18 ./cmplx
1.14 ./cmplxdivide
0.06 ./cmplxdivide1
0.18 ./complit
0.17 ./compos
0.18 ./const
0.18 ./const1
0.18 ./const2
0.62 ./const3
0.48 ./convert
0.20 ./convert3
0.21 ./convlit
0.20 ./convlit1
1.28 ./copy
0.23 ./ddd
0.18 ./ddd1
0.07 ./ddd2
0.23 ./ddd3
0.18 ./decl
0.20 ./declbad
0.56 ./defer
0.20 ./deferprint
0.51 ./divide
0.07 ./empty
0.26 ./env
0.07 ./eof
0.09 ./eof1
0.20 ./escape
0.28 ./escape2
0.15 ./escape3
0.18 ./float_lit
0.31 ./floatcmp
0.20 ./for
0.18 ./func
0.18 ./func1
0.09 ./func2
0.20 ./func3
0.21 ./func4
0.18 ./func5
0.09 ./func6
0.20 ./func7
0.18 ./gc
0.20 ./gc1
0.81 ./gc2
1.10 ./goprint
0.21 ./goto
0.18 ./hashmap
0.20 ./helloworld
0.20 ./if
0.09 ./import
0.32 ./import1
0.06 ./import2
0.09 ./import3
0.34 ./import4
3.65 ./index
0.18 ./indirect
0.17 ./indirect1
0.17 ./init
0.31 ./initcomma
0.56 ./initialize
0.21 ./initializerr
0.35 ./initsyscall
0.28 ./int_lit
0.18 ./intcvt
0.18 ./iota
0.18 ./label
0.18 ./label1
0.20 ./literal
0.56 ./malloc1
4.32 ./mallocfin
0.89 ./mallocrand
0.68 ./mallocrep
1.28 ./mallocrep1
0.67 ./map
0.18 ./method
0.20 ./method1
0.18 ./method2
0.18 ./method3
0.20 ./named
0.18 ./named1
0.62 ./nil
0.70 ./nul1
0.09 ./parentype
0.29 ./peano
0.18 ./printbig
0.21 ./range
0.20 ./recover
0.18 ./recover1
0.31 ./recover2
0.32 ./recover3
0.60 ./rename
0.18 ./rename1
0.18 ./runtime
0.18 ./shift1
0.09 ./shift2
0.14 ./sieve
0.09 ./sigchld
0.18 ./simassign
0.14 ./sinit
0.07 ./sizeof
0.14 ./solitaire
0.25 ./stack
0.29 ./string_lit
0.51 ./stringrange
0.18 ./struct0
0.18 ./switch
0.29 ./switch1
0.20 ./test0
0.28 ./turing
0.28 ./typeswitch
0.50 ./typeswitch1
0.18 ./typeswitch2
0.18 ./undef
0.20 ./utf
0.20 ./varerr
0.21 ./varinit
0.31 ./vectors
0.64 ./zerodivide
0.17 ken/array
0.35 ken/chan
0.17 ken/chan1
0.23 ken/complit
0.23 ken/convert
0.18 ken/cplx0
0.17 ken/cplx1
0.21 ken/cplx2
0.48 ken/cplx3
0.57 ken/cplx4
0.18 ken/cplx5
1.14 ken/divconst
0.23 ken/divmod
0.21 ken/embed
0.18 ken/for
0.18 ken/interbasic
0.18 ken/interfun
0.17 ken/intervar
0.18 ken/label
0.17 ken/litfun
0.18 ken/mfunc
1.14 ken/modconst
0.17 ken/ptrfun
0.17 ken/ptrvar
0.20 ken/range
0.17 ken/rob1
0.25 ken/rob2
0.20 ken/robfor
0.18 ken/robfunc
0.17 ken/shift
0.17 ken/simparray
0.17 ken/simpbool
0.18 ken/simpconv
0.17 ken/simpfun
0.20 ken/simpprint
0.15 ken/simpswitch
0.17 ken/simpvar
0.18 ken/slicearray
0.18 ken/sliceslice
0.18 ken/string
0.20 ken/strvar
1.26 chan/doubleselect
0.28 chan/fifo
0.43 chan/goroutines
0.62 chan/nonblock
0.18 chan/perm
0.29 chan/powser1
0.34 chan/powser2
0.18 chan/select
0.36 chan/select2
1.35 chan/select3
0.20 chan/select4
3.82 chan/select5
0.20 chan/select6
0.17 chan/sendstmt
0.26 chan/sieve1
0.42 chan/sieve2
0.17 chan/zerosize
0.18 interface/bigdata
0.18 interface/convert
0.18 interface/convert1
0.20 interface/convert2
0.26 interface/embed
0.06 interface/embed0
0.25 interface/embed1
0.18 interface/embed2
0.18 interface/explicit
0.20 interface/fail
0.46 interface/fake
0.18 interface/pointer
0.20 interface/private
0.06 interface/private1
0.17 interface/receiver
0.18 interface/receiver1
0.09 interface/recursive
0.18 interface/returntype
0.29 interface/struct
0.23 nilptr/arrayindex
9.54 nilptr/arrayindex1
9.32 nilptr/arraytoslice
15.36 nilptr/arraytoslice1
14.71 nilptr/arraytoslice2
16.89 nilptr/slicearray
3.73 nilptr/structfield
3.21 nilptr/structfield1
3.09 nilptr/structfield2
4.92 nilptr/structfieldaddr
1.92 syntax/chan
0.20 syntax/chan1
0.18 syntax/else
0.18 syntax/forvar
0.18 syntax/if
0.25 syntax/import
0.18 syntax/interface
0.18 syntax/semi1
0.20 syntax/semi2
0.18 syntax/semi3
0.18 syntax/semi4
0.20 syntax/semi5
0.20 syntax/semi6
0.18 syntax/semi7
0.18 syntax/topexpr
0.20 syntax/typesw
0.23 syntax/vareq
0.32 syntax/vareq1
0.86 dwarf/linedirectives
0.23 dwarf/main
0.06 dwarf/z1
0.07 dwarf/z10
0.07 dwarf/z11
0.09 dwarf/z12
0.09 dwarf/z13
0.07 dwarf/z14
0.07 dwarf/z15
0.07 dwarf/z16
0.07 dwarf/z17
0.42 dwarf/z18
0.09 dwarf/z19
0.06 dwarf/z2
0.06 dwarf/z20
0.07 dwarf/z3
0.18 dwarf/z4
0.09 dwarf/z5
0.07 dwarf/z6
0.12 dwarf/z7
0.09 dwarf/z8
0.07 dwarf/z9
0.39 fixedbugs/bug000
0.32 fixedbugs/bug002
0.18 fixedbugs/bug003
0.17 fixedbugs/bug004
0.21 fixedbugs/bug005
2.92 fixedbugs/bug006
0.17 fixedbugs/bug007
0.17 fixedbugs/bug008
0.20 fixedbugs/bug009
0.20 fixedbugs/bug010
0.17 fixedbugs/bug011
0.18 fixedbugs/bug012
0.17 fixedbugs/bug013
0.23 fixedbugs/bug014
0.18 fixedbugs/bug015
0.18 fixedbugs/bug016
0.18 fixedbugs/bug017
0.09 fixedbugs/bug020
0.18 fixedbugs/bug021
0.18 fixedbugs/bug022
0.20 fixedbugs/bug023
0.15 fixedbugs/bug024
0.15 fixedbugs/bug026
0.17 fixedbugs/bug027
0.17 fixedbugs/bug028
0.18 fixedbugs/bug030
0.17 fixedbugs/bug031
0.32 fixedbugs/bug035
0.25 fixedbugs/bug036
0.31 fixedbugs/bug037
0.11 fixedbugs/bug038
0.18 fixedbugs/bug039
0.09 fixedbugs/bug040
0.18 fixedbugs/bug045
0.18 fixedbugs/bug046
0.20 fixedbugs/bug047
0.20 fixedbugs/bug048
0.18 fixedbugs/bug049
0.18 fixedbugs/bug050
0.20 fixedbugs/bug051
0.17 fixedbugs/bug052
0.17 fixedbugs/bug053
0.20 fixedbugs/bug054
0.18 fixedbugs/bug055
0.18 fixedbugs/bug056
0.11 fixedbugs/bug057
0.21 fixedbugs/bug058
0.87 fixedbugs/bug059
0.29 fixedbugs/bug060
0.21 fixedbugs/bug061
0.18 fixedbugs/bug062
0.09 fixedbugs/bug063
0.09 fixedbugs/bug064
0.20 fixedbugs/bug065
0.40 fixedbugs/bug066
0.18 fixedbugs/bug067
0.25 fixedbugs/bug068
0.11 fixedbugs/bug069
0.93 fixedbugs/bug070
0.10 fixedbugs/bug071
0.20 fixedbugs/bug072
0.21 fixedbugs/bug073
0.20 fixedbugs/bug074
0.39 fixedbugs/bug075
0.32 fixedbugs/bug076
0.10 fixedbugs/bug077
0.21 fixedbugs/bug078
0.17 fixedbugs/bug080
0.40 fixedbugs/bug081
0.28 fixedbugs/bug082
0.70 fixedbugs/bug083
0.25 fixedbugs/bug084
0.25 fixedbugs/bug085
0.26 fixedbugs/bug086
0.10 fixedbugs/bug087
0.14 fixedbugs/bug088
0.17 fixedbugs/bug089
0.28 fixedbugs/bug090
0.20 fixedbugs/bug091
0.90 fixedbugs/bug092
0.23 fixedbugs/bug093
0.09 fixedbugs/bug094
0.10 fixedbugs/bug096
0.37 fixedbugs/bug097
0.10 fixedbugs/bug098
0.23 fixedbugs/bug099
0.79 fixedbugs/bug101
0.25 fixedbugs/bug102
0.20 fixedbugs/bug103
0.70 fixedbugs/bug104
0.17 fixedbugs/bug106
0.21 fixedbugs/bug107
0.21 fixedbugs/bug108
0.34 fixedbugs/bug109
0.18 fixedbugs/bug110
0.21 fixedbugs/bug111
0.09 fixedbugs/bug112
...

I had to cut this, because of codereview.

[hector, 2011-09-21 14:36:05]

It would be interesting to see exactly what percentage of the time is spent
running make/8g/8l.  Firstly the list can be sorted in descending order to find
out if there are any outliers, and then the times can be summed to find out the
total time spent running Go code.

[brainman, 2011-09-22 00:16:13]

On 2011/09/21 14:36:05, hector wrote:
> It would be interesting to see exactly what percentage of the time is spent
> running make/8g/8l.  Firstly the list can be sorted in descending order to
find
> out if there are any outliers, and then the times can be summed to find out
the
> total time spent running Go code.

# cat times.out | sort -n -k1 | tail -n 20
1.67 fixedbugs/bug323
1.92 syntax/chan
1.95 ./assign
1.98 fixedbugs/bug257
2.42 fixedbugs/bug206
2.92 fixedbugs/bug006
3.09 nilptr/structfield2
3.14 ./append
3.21 nilptr/structfield1
3.65 ./index
3.73 nilptr/structfield
3.82 chan/select5
4.32 ./mallocfin
4.92 nilptr/structfieldaddr
9.32 nilptr/arraytoslice
9.54 nilptr/arrayindex1
14.71 nilptr/arraytoslice2
15.36 nilptr/arraytoslice1
16.89 nilptr/slicearray
26.76 ./64bit
# cat times.out | awk '{SUM+=$1} END{print SUM}'
297
#

[rsc, 2011-09-23 02:24:20]

Note that the times are to run the entire tiny
shell script at the top of the test case, so
it includes compile + link + run.

The most interesting case to look into is why
nilptr/* takes so long.  It does on QEMU too.

Russ

[hector, 2011-09-23 17:06:50]

Since all the tests that are slow are committing a large chunk of memory
it's probably due to Windows having to grow the page file or swapping that's
causing the delay.  Incidentally on machines with more memory these tests
run in under a second.  If I may inquire Alex, what is the configuration of
the virtual memory on your machine?  For instance how much physical memory,
configuration and size of page file and total and limit commit charge, as
visible in Task Manager.
On Sep 23, 2011 3:24 AM, "Russ Cox" <rsc@golang.org> wrote:
> Note that the times are to run the entire tiny
> shell script at the top of the test case, so
> it includes compile + link + run.
>
> The most interesting case to look into is why
> nilptr/* takes so long. It does on QEMU too.
>
> Russ

[brainman, 2011-09-24 00:58:49]

On 2011/09/23 17:06:50, hector wrote:
> run in under a second.  If I may inquire Alex, what is the configuration of
> the virtual memory on your machine?  For instance how much physical memory,
> configuration and size of page file and total and limit commit charge, as
> visible in Task Manager.

It is a vmware hosted PC, so take it with a grain of salt:
physical memory: 2G
configuration and size of page file: on C: drive, initial size of 0.75G, max of
1.5G
total and limit commit charge: 2G and 3G

Alex

[hector, 2011-09-24 12:49:57]

On 2011/09/24 00:58:49, brainman wrote:
> It is a vmware hosted PC, so take it with a grain of salt:
> physical memory: 2G
> configuration and size of page file: on C: drive, initial size of 0.75G, max
of
> 1.5G
> total and limit commit charge: 2G and 3G

Thanks. Are we able to time the nilptr tests (or indeed, the whole test suite)
on the Windows builder to see whether the problems with virtual memory exist
there?

[brainman, 2011-09-24 12:58:40]

On 2011/09/24 12:49:57, hector wrote:
> 
> ... Are we able to time the nilptr tests (or indeed, the whole test suite)
> on the Windows builder to see whether the problems with virtual memory exist
> there?

I don't have access to it. It is Brad's PC. Ask him to help you.

Alex

[rsc, 2011-09-26 17:11:26]

*** Submitted as http://code.google.com/p/go/source/detail?r=821669218333 ***

test: disable sigchld test on Windows

Alex Brainman reports that this is the only test
that keeps us from running test/run.

R=alex.brainman, lucio.dere, bradfitz, hectorchu
CC=golang-dev
http://codereview.appspot.com/4777043