code review 5071044: testing: Add support for running tests in parallel (t.P...

testing: Add support for running tests in parallel (t.Parallel API).

See discussion at https://groups.google.com/d/topic/golang-dev/RAKiqi44GEU/discussion

[tebeka, 2011-09-19 22:51:51]

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

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

[bradfitz, 2011-09-19 22:56:43]

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:151: // Signal that this test can be run in parallel
to others.
Go comments start with the name of the subject.

// Parallel signals that ...

Also, rather than referring to mailing list threads in commit messages users
won't see, you should elaborate here on the details.

Notably, does this run it in parallel to any test, or only tests which were also
marked Parallel?

[dvyukov, 2011-09-19 23:57:30]

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:155: t.ch = t.pch
So parallel tests run in parallel with non-parallel ones. Am I missing
something? We decided to not anything in parallel with sequential tests, so this
function must include some blocking, and released only when all sequential tests
have been completed.

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:178: println("--- PASS:", t.name, tstr)
It will lead to intermixed output of test results. Isn't it going to be a
complete mess? I am not sure what to do with it right now, but I think we must
at least consider some alternatives.
Maybe collect all test-related output to a separate buffer, and then output it
at once?

[tebeka, 2011-09-20 17:26:44]

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:151: // Signal that this test can be run in parallel
to others.
> Go comments start with the name of the subject.
> 
> // Parallel signals that ...
OK
 
> Also, rather than referring to mailing list threads in commit messages users
> won't see, you should elaborate here on the details.
OK, will do next time.

> Notably, does this run it in parallel to any test, or only tests which were
also
> marked Parallel?
In parallel to all other tests, but this is going to be fixed.

http://codereview.appspot.com/5071044/diff/4001/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:155: t.ch = t.pch
On 2011/09/19 23:57:30, dvyukov wrote:
> So parallel tests run in parallel with non-parallel ones. Am I missing
> something? We decided to not anything in parallel with sequential tests, so
this
> function must include some blocking, and released only when all sequential
tests
> have been completed.
You're right. Missed that one, will fix.

[tebeka, 2011-09-20 17:27:53]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[tebeka, 2011-09-20 23:18:46]

Any comments?

[dvyukov, 2011-09-21 00:15:19]

LGTM

[rog, 2011-09-21 08:22:45]

i wonder whether it's right that all the parallel tests should run at once.
it would be trivial to limit the number (GOMAXPROCS*2 ?) of tests
run at once.

On 21 September 2011 01:15,  <dvyukov@google.com> wrote:
> LGTM
>
> http://codereview.appspot.com/5071044/
>

[rog, 2011-09-21 08:35:38]

http://codereview.appspot.com/5071044/diff/12001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/12001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:243: t := &T{ch: make(chan *T), pch: pch, name:
testName, wch: wch}
i don't think you need two channels here; you could just use the same channel
(store it in t.ch) throughout - no need to make a new one each time.

http://codereview.appspot.com/5071044/diff/12001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:247: numParallel += 1
numParallel++

[tebeka, 2011-09-21 19:08:35]

On 2011/09/21 08:22:45, rog wrote:
> i wonder whether it's right that all the parallel tests should run at once.
> it would be trivial to limit the number (GOMAXPROCS*2 ?) of tests
> run at once.
IMO it depends on the test scenario. In my case most of my tests wait on IO, so
I'd like to run as much as possible in parallel. Probably a configuration
option, but this can be done in a later step after we have some feedback from
users.

[tebeka, 2011-09-21 19:10:36]

On 2011/09/21 08:35:38, rog wrote:
>
http://codereview.appspot.com/5071044/diff/12001/src/pkg/testing/testing.go#n...
> src/pkg/testing/testing.go:243: t := &T{ch: make(chan *T), pch: pch, name:
> testName, wch: wch}
> i don't think you need two channels here; you could just use the same channel
> (store it in t.ch) throughout - no need to make a new one each time.
Can you explain. I *think* you mean to move the creation of pch to the outer
loop. If you mean having only t.ch, then I couldn't find a way to do it since
t.Parallel is called after the test is being run.
(Or do you mean as a closure?).

[rog, 2011-09-21 19:25:31]

On 21 September 2011 20:10,  <miki.tebeka@gmail.com> wrote:
> On 2011/09/21 08:35:38, rog wrote:
>
>
http://codereview.appspot.com/5071044/diff/12001/src/pkg/testing/testing.go#n...
>>
>> src/pkg/testing/testing.go:243: t := &T{ch: make(chan *T), pch: pch,
>
> name:
>>
>> testName, wch: wch}
>> i don't think you need two channels here; you could just use the same
>
> channel
>>
>> (store it in t.ch) throughout - no need to make a new one each time.
>
> Can you explain. I *think* you mean to move the creation of pch to the
> outer loop. If you mean having only t.ch, then I couldn't find a way to
> do it since t.Parallel is called after the test is being run.
> (Or do you mean as a closure?).

i mean that you could store the same channel (created in the outer loop)
in each T. there would be no contention for it because Parallel waits
for t.wch before using t.ch again.

[tebeka, 2011-09-21 19:54:48]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rog, 2011-09-21 20:22:53]

i've tried to be a bit more explicit about what i mean here.

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:98: pch      chan *T   // Output for parallel tests
d

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:155: t.parallel = true
this doesn't appear to be used. perhaps it should be, to make t.Parallel
idempotent. as it is it'll deadlock if called twice.

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:157: t.ch = t.pch
d

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:209: pch := make(chan *T)
s/pch/ch/

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:229: wch := make(chan bool)
i'm not sure wch is a good name here. startParallel? (same in T)

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:243: t := &T{ch: make(chan *T), pch: pch, name:
testName, wch: wch}
t := &T{ch: ch, name: testName, wch: wch}

http://codereview.appspot.com/5071044/diff/6002/src/pkg/testing/testing.go#ne...
src/pkg/testing/testing.go:255: out := <-pch
s/pch/ch/

[tebeka, 2011-09-22 01:53:10]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[tebeka, 2011-09-22 01:57:16]

Please ignore this patch, sorry

[tebeka, 2011-09-22 02:09:15]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rog, 2011-09-22 08:13:20]

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:212: }()
i don't think there's any need for this. there's no printing which is called
concurrently. revert to println. actually, i can't see why println was used -
given that fmt is a dependency anyway, it should probably use fmt.Printf or
Fprintf(os.Stderr as appropriate, but that's probably fodder for another CL.

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:217: PROCS:
d

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:223: break PROCS
s/ PROCS//
but can this ever actually happen? the old code didn't bother checking¸and the
docs don't say that it GOMAXPROCS might not succeed.

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:226: numParallel, startParallel := 0, make(chan bool)
i think these would be better on two separate lines. they're not that closely
related.

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:246: } else {
no need for the else. save an indentation level.

[tebeka, 2011-09-22 20:30:35]

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:212: }()
On 2011/09/22 08:13:20, rog wrote:
> i don't think there's any need for this. there's no printing which is called
> concurrently. revert to println. actually, i can't see why println was used -
> given that fmt is a dependency anyway, it should probably use fmt.Printf or
> Fprintf(os.Stderr as appropriate, but that's probably fodder for another CL.

Done.

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:217: PROCS:
On 2011/09/22 08:13:20, rog wrote:
> d

Done.

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:223: break PROCS
On 2011/09/22 08:13:20, rog wrote:
> s/ PROCS//
> but can this ever actually happen? the old code didn't bother checking¸and the
> docs don't say that it GOMAXPROCS might not succeed.

Done.

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:226: numParallel, startParallel := 0, make(chan bool)
On 2011/09/22 08:13:20, rog wrote:
> i think these would be better on two separate lines. they're not that closely
> related.

Done.

http://codereview.appspot.com/5071044/diff/23001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:246: } else {
On 2011/09/22 08:13:20, rog wrote:
> no need for the else. save an indentation level.

Done.

[tebeka, 2011-09-22 20:31:08]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[tebeka, 2011-09-27 15:19:23]

Any more comments? I'd really like to get this into a coming release.

[rog, 2011-09-27 15:56:55]

LGTM

On 27 September 2011 16:19, Miki Tebeka <miki.tebeka@gmail.com> wrote:
> Any more comments? I'd really like to get this into a coming release.
>

[r, 2011-09-28 15:45:01]

This needs a rethink.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:95: errors        string
s/errors/error/
add
// Error string from test.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:96: failed        bool
// Test has failed.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:97: ch            chan *T   // Output for serial
tests
s/$/./ here and below

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:99: name          string
// Name of test.

also move this to the first entry

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:100: ns            int64
// Duration of test in nanoseconds.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:151: // Parallel signal that this test can be run in
parallel to other parallel tests.
s/signal/signals/

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:152: // This will block until all serial tests in
this cpu group are done.
s/This will block/It waits/

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:238: close(startParallel) // Release parallel tests
s/$/./

cute trick. gross but cute.

that said, this is not acceptable. if i have 1000 parallel tests, it will run
them all at once. they need to be gated with at most GOMAXPROCS at once.
moreover, if you're running with procs not equal to the start value of
GOMAXPROCS, you shouldn't be doing this at all.

--parallel should be for default parallel testing in the simplest case, with
GOMAXPROCS tests running at once. nothing else should happen.

[rog, 2011-09-28 16:59:40]

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:238: close(startParallel) // Release parallel tests
On 2011/09/28 15:45:01, r wrote:
> s/$/./
> 
> cute trick. gross but cute.
> 
> that said, this is not acceptable. if i have 1000 parallel tests, it will run
> them all at once. they need to be gated with at most GOMAXPROCS at once.
> moreover, if you're running with procs not equal to the start value of
> GOMAXPROCS, you shouldn't be doing this at all.
> 
> --parallel should be for default parallel testing in the simplest case, with
> GOMAXPROCS tests running at once. nothing else should happen.

i mentioned this earlier and tend to agree. it's fairly trivial to do without
changing anything other than these few lines of code, i think.

something like this, perhaps:

waiting := numParallel
running := 0
finished := 0
for finished < numParallel {
    if running < GOMAXPROCS && waiting > 0 {
        startParallel <- true
        waiting--
        running++
        continue
    }
    t := <-ch
    report(t)
    ok = ok && !t.failed
    running--
    finished++
}

[dvyukov, 2011-09-28 17:40:08]

> that said, this is not acceptable. if i have 1000 parallel tests, it will run
> them all at once.

And... what is the problem with that?

> they need to be gated with at most GOMAXPROCS at once.

Why?

> moreover, if you're running with procs not equal to the start value of
> GOMAXPROCS, you shouldn't be doing this at all.
> 
> --parallel should be for default parallel testing in the simplest case, with
> GOMAXPROCS tests running at once. nothing else should happen.

Runtime will automatically gate number of simultaneously executing tests to
GOMAXPROCS. If there are some inactive tests due to IO or timers, well, that's
great, user will get substantial speedups.

Would you limit number of concurrent requests in a server to GOMAXPROCS? Yeah,
it's not a very good idea. Tests are exactly the same. It is the whole point of
goroutines interleaved on limited number of threads.

The only reason to introduce some gating I see is to prevent total system
resources over-subscription, but then is must be gated to something like
1000*GOMAXPROCS. And actually I do not see reason to do it now, when we have
exactly 0 parallel tests out there.

[r2, 2011-09-28 17:42:53]

On Sep 28, 2011, at 10:40 AM, dvyukov@google.com wrote:

>> that said, this is not acceptable. if i have 1000 parallel tests, it
> will run
>> them all at once.
> 
> And... what is the problem with that?
> 
>> they need to be gated with at most GOMAXPROCS at once.
> 
> Why?

It's good design.
It's good citizenship.
It avoids oversubscription of resources.
It reduces cache pressure, so the whole job will run faster if CPU-bound.
It's easy.

>> moreover, if you're running with procs not equal to the start value of
>> GOMAXPROCS, you shouldn't be doing this at all.
> 
>> --parallel should be for default parallel testing in the simplest
> case, with
>> GOMAXPROCS tests running at once. nothing else should happen.
> 
> Runtime will automatically gate number of simultaneously executing tests
> to GOMAXPROCS. If there are some inactive tests due to IO or timers,
> well, that's great, user will get substantial speedups.
> 
> Would you limit number of concurrent requests in a server to GOMAXPROCS?
> Yeah, it's not a very good idea. Tests are exactly the same. It is the
> whole point of goroutines interleaved on limited number of threads.
> 
> The only reason to introduce some gating I see is to prevent total
> system resources over-subscription, but then is must be gated to
> something like 1000*GOMAXPROCS. And actually I do not see reason to do
> it now, when we have exactly 0 parallel tests out there.

When it's easy to do the right thing, there's no excuse not to.

-rob

[dvyukov, 2011-09-28 18:07:36]

On Wed, Sep 28, 2011 at 9:42 PM, Rob 'Commander' Pike <r@google.com> wrote:

>
> On Sep 28, 2011, at 10:40 AM, dvyukov@google.com wrote:
>
> >> that said, this is not acceptable. if i have 1000 parallel tests, it
> > will run
> >> them all at once.
> >
> > And... what is the problem with that?
> >
> >> they need to be gated with at most GOMAXPROCS at once.
> >
> > Why?
>
> It's good design.
>

It's bad design, because it ties together unrelated things: GOMAXPROCS and
number of concurrent requests/tests.


> It's good citizenship.

It avoids oversubscription of resources.
>

It's not the case. A lot of goroutines do not oversubscribe processors and
limited number of goroutines do not oversubscribe memory.
Moreover, a single test can perfectly utilize all processors, and at the
same time GOMAXPROCS tests can easily totally under-utilize processors.
It's unrelated things.


It reduces cache pressure, so the whole job will run faster if CPU-bound.
>

It's a common (not saying that it's the right thing) for a Go program to
have more gorotuines than threads. Then, efficient usage of system resources
is the work for gorotuine scheduler. User programs should not limit number
of gorotuines to GOMAXPROCS.


> It's easy.


> >> moreover, if you're running with procs not equal to the start value of
> >> GOMAXPROCS, you shouldn't be doing this at all.
> >
> >> --parallel should be for default parallel testing in the simplest
> > case, with
> >> GOMAXPROCS tests running at once. nothing else should happen.
> >
> > Runtime will automatically gate number of simultaneously executing tests
> > to GOMAXPROCS. If there are some inactive tests due to IO or timers,
> > well, that's great, user will get substantial speedups.
> >
> > Would you limit number of concurrent requests in a server to GOMAXPROCS?
> > Yeah, it's not a very good idea. Tests are exactly the same. It is the
> > whole point of goroutines interleaved on limited number of threads.
> >
> > The only reason to introduce some gating I see is to prevent total
> > system resources over-subscription, but then is must be gated to
> > something like 1000*GOMAXPROCS. And actually I do not see reason to do
> > it now, when we have exactly 0 parallel tests out there.
>
> When it's easy to do the right thing, there's no excuse not to.
>
>
The right thing to do is to limit number of concurrent tests to
min(K1*GOMAXPROCS, K2*SYSTEM_MEM, ...). The problem is that we don't know K1
and K2, so it's not easy. And once again, it's the work for
scheduler/runtime, not for user program.

[borman, 2011-09-28 18:59:24]

I tend to agree with Dmitry.  Given the foundations of Go, I think the
arguments that Rob is mostly point to implementation issues or mismatched
expectations.

Throttling the number of goroutines that are runnable does not reduce the
number of goroutines in this design, though it *might* reduce the memory
resident set size.  If the goroutine scheduler is inefficient for large
number of runnable goroutines (I don't believe this to be the case) then
throttling the number of goroutines fixes a symptom and not a the problem.
 Throttling the number of goroutines that are runnable will reduce the wall
clock time for an individual test but may increase the wall clock time for
the sum of the tests.  In order for this throttling to make the total wall
clock time to be reduced then at least one of the following would need to
happen:

   1. Overcome inefficient scheduling/thrashing of multiple goroutines
   2. Reduce the number of "blocking" threads started
   3. Reduce the amount of time in synchronization (which reduces contention
   time)
   4. Reduce cache misses by reducing RSS
   5. Some how keep affinity of memory to a a chip (or better, a core)

The first three would be addressing issues in the Go runtime.  I include 3
in this case because in theory the tests themselves should have no
synchronization.  3 is probably a variant of 1.

4 would require some benchmarking and will have different characteristics as
the capabilities of CPUs constantly changes.

5 can actually have significant impact (which is why GOMAXPROCS=1 is often a
good idea).  Addressing 5 will require the Go runtime to assign processor
affinity to the threads and goroutine affinity to the threads.  My work with
realtime multi-core scheduling for VxWorks demonstrated how huge of an
impact 5 has.

All of that said, I don't think this whole parallel idea is all that
helpful.  Rather than run multiple tests within a single test program in
parallel, it would be more efficient to run multiple test programs in
parallel where each one is running its tests in serial.  This was a big
lesson learned when working with high performance super computing.  Use the
coarsest grain parallelism that will keep the resources busy.  It addresses
all 5 above.

    -Paul

On Wed, Sep 28, 2011 at 11:07 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Wed, Sep 28, 2011 at 9:42 PM, Rob 'Commander' Pike <r@google.com>wrote:
>
>>
>> On Sep 28, 2011, at 10:40 AM, dvyukov@google.com wrote:
>>
>> >> that said, this is not acceptable. if i have 1000 parallel tests, it
>> > will run
>> >> them all at once.
>> >
>> > And... what is the problem with that?
>> >
>> >> they need to be gated with at most GOMAXPROCS at once.
>> >
>> > Why?
>>
>> It's good design.
>>
>
> It's bad design, because it ties together unrelated things: GOMAXPROCS and
> number of concurrent requests/tests.
>
>
>> It's good citizenship.
>
> It avoids oversubscription of resources.
>>
>
> It's not the case. A lot of goroutines do not oversubscribe processors and
> limited number of goroutines do not oversubscribe memory.
> Moreover, a single test can perfectly utilize all processors, and at the
> same time GOMAXPROCS tests can easily totally under-utilize processors.
> It's unrelated things.
>
>
> It reduces cache pressure, so the whole job will run faster if CPU-bound.
>>
>
> It's a common (not saying that it's the right thing) for a Go program to
> have more gorotuines than threads. Then, efficient usage of system resources
> is the work for gorotuine scheduler. User programs should not limit number
> of gorotuines to GOMAXPROCS.
>
>
>> It's easy.
>
>
>> >> moreover, if you're running with procs not equal to the start value of
>> >> GOMAXPROCS, you shouldn't be doing this at all.
>> >
>> >> --parallel should be for default parallel testing in the simplest
>> > case, with
>> >> GOMAXPROCS tests running at once. nothing else should happen.
>> >
>> > Runtime will automatically gate number of simultaneously executing tests
>> > to GOMAXPROCS. If there are some inactive tests due to IO or timers,
>> > well, that's great, user will get substantial speedups.
>> >
>> > Would you limit number of concurrent requests in a server to GOMAXPROCS?
>> > Yeah, it's not a very good idea. Tests are exactly the same. It is the
>> > whole point of goroutines interleaved on limited number of threads.
>> >
>> > The only reason to introduce some gating I see is to prevent total
>> > system resources over-subscription, but then is must be gated to
>> > something like 1000*GOMAXPROCS. And actually I do not see reason to do
>> > it now, when we have exactly 0 parallel tests out there.
>>
>> When it's easy to do the right thing, there's no excuse not to.
>>
>>
> The right thing to do is to limit number of concurrent tests to
> min(K1*GOMAXPROCS, K2*SYSTEM_MEM, ...). The problem is that we don't know K1
> and K2, so it's not easy. And once again, it's the work for
> scheduler/runtime, not for user program.
>
>

[tebeka, 2011-09-29 17:03:15]

>
> that said, this is not acceptable. if i have 1000 parallel tests, it
> will run them all at once. they need to be gated with at most GOMAXPROCS
> at once. moreover, if you're running with procs not equal to the start
> value of GOMAXPROCS, you shouldn't be doing this at all.
>
> --parallel should be for default parallel testing in the simplest case,
> with GOMAXPROCS tests running at once. nothing else should happen.
>
 I disagree with that. I don't think that GOMAXPROCS and number of parallel 
tests should be connected.
And also since the default value of GOMAXPROCS is 1, it mean that by default 
you will not get any parallelism even when you add t.Parallel() to your 
tests.

As said, I'm doing Selenium tests, and they mostly wait on IO from the 
server. I *want* as many of them in parallel as possible.
Maybe add GOMAXTESTS environment variable to control that? (by default be 
unbounded).

[tebeka, 2011-09-30 22:06:09]

Is there a way to move this forward?

[r2, 2011-09-30 23:39:52]

On Sep 30, 2011, at 3:06 PM, Miki Tebeka wrote:

> Is there a way to move this forward?

Yes, address my comments.

-rob

[tebeka, 2011-10-04 00:18:45]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com, r@golang.org, r@google.com, borman@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[tebeka, 2011-10-04 00:19:09]

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:95: errors        string
On 2011/09/28 15:45:01, r wrote:
> s/errors/error/
> add
> // Error string from test.
This is not my doing. I'd like to focus the review on the issue I'm trying to
get in. I already fixed several other issues that are not related to the problem
solved here.

Also this is an accumulation of errors (what Log function does it append to this
string). So the name looks ok to me, I'll add a comment.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:96: failed        bool
On 2011/09/28 15:45:01, r wrote:
> // Test has failed.

Done.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:97: ch            chan *T   // Output for serial
tests
On 2011/09/28 15:45:01, r wrote:
> s/$/./ here and below

Done.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:99: name          string
On 2011/09/28 15:45:01, r wrote:
> // Name of test.
> 
> also move this to the first entry

Done.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:100: ns            int64
On 2011/09/28 15:45:01, r wrote:
> // Duration of test in nanoseconds.

Done.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:151: // Parallel signal that this test can be run in
parallel to other parallel tests.
On 2011/09/28 15:45:01, r wrote:
> s/signal/signals/

Done.

http://codereview.appspot.com/5071044/diff/26003/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:152: // This will block until all serial tests in
this cpu group are done.
On 2011/09/28 15:45:01, r wrote:
> s/This will block/It waits/

Done.

[r, 2011-10-04 02:42:56]

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:97: errors        string    // Accumlation of
messages from Log calls.
Even though it's not yours, you're there: please fix this comment. It's got a
spelling mistake anyway.
// Error string from test.

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:154: // Note that only test.parallel tests will run
at the same time.
// Parallel signals that this test is to be run in parallel with (and only with)
// other parallel tests in this CPU group.

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:194: print(fmt.Sprintf(format, "PASS", t.name, tstr,
t.errors))
we have fmt - why not use it?
fmt.Fprintf(os.Stderr, ....)

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:200: println("testing: warning: no tests to run")
s/println/fmt.Println/

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:243: for finished < numParallel {
you don't need finished. just count down numParallel.
for ; numParallel; numParallel-- {
}

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:244: if (running < *parallel) && (waiting > 0) {
parens unnecessary

[tebeka, 2011-10-04 03:51:19]

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:97: errors        string    // Accumlation of
messages from Log calls.
On 2011/10/04 02:42:56, r wrote:
> Even though it's not yours, you're there: please fix this comment. It's got a
> spelling mistake anyway.
> // Error string from test.

Done.

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:154: // Note that only test.parallel tests will run
at the same time.
On 2011/10/04 02:42:56, r wrote:
> // Parallel signals that this test is to be run in parallel with (and only
with)
> // other parallel tests in this CPU group.

Done.

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:194: print(fmt.Sprintf(format, "PASS", t.name, tstr,
t.errors))
On 2011/10/04 02:42:56, r wrote:
> we have fmt - why not use it?
> fmt.Fprintf(os.Stderr, ....)

Done.

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:200: println("testing: warning: no tests to run")
On 2011/10/04 02:42:56, r wrote:
> s/println/fmt.Println/
println prints to os.Stderr, while fmt.Println prints to os.Stdout. Will change
to fmt.Fprintln(os.Stderr ...)

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:243: for finished < numParallel {
On 2011/10/04 02:42:56, r wrote:
> you don't need finished. just count down numParallel.
> for ; numParallel; numParallel-- {
> }
It's not the same logic. "finished" makes sure we wait for all tests, otherwise
the top level loop will exit before we wait on the final tests.

http://codereview.appspot.com/5071044/diff/35001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:244: if (running < *parallel) && (waiting > 0) {
On 2011/10/04 02:42:56, r wrote:
> parens unnecessary

Done.

[tebeka, 2011-10-04 03:51:37]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com, r@golang.org, r@google.com, borman@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-10-04 03:53:03]

http://codereview.appspot.com/5071044/diff/40001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/40001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:253: finished++
i missed the continue. still, it's the same logic if you do numParallell-- here.
you don't need all these variables

[rog, 2011-10-04 07:47:01]

http://codereview.appspot.com/5071044/diff/40001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/40001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:253: finished++
On 2011/10/04 03:53:04, r wrote:
> i missed the continue. still, it's the same logic if you do numParallell--
here.
> you don't need all these variables

+1.
but i think perhaps the distinction between parallel and numParallel is
confusing. i wonder if the flag would be better named "maxprocs", and then the
association with GOMAXPROCS would be more obvious too.

[r2, 2011-10-04 14:19:39]

the problem with that renaming is it decouples from the function call,
Parallel(). plus i think --parallel=10 is clear.

but i do take your point.

-rob

[tebeka, 2011-10-06 02:04:21]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com, r@golang.org, r@google.com, borman@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-10-06 03:33:31]

Now we're down to one keystroke :)

Also please fill in the CLA as described in contribute.html if you haven't
already.

http://codereview.appspot.com/5071044/diff/43001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/43001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:244: numParallel --
did this run through gofmt? there's a space before the --
please fix.

[tebeka, 2011-10-06 16:49:08]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dvyukov@google.com,
rogpeppe@gmail.com, r@golang.org, r@google.com, borman@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[tebeka, 2011-10-06 16:49:11]

http://codereview.appspot.com/5071044/diff/43001/src/pkg/testing/testing.go
File src/pkg/testing/testing.go (right):

http://codereview.appspot.com/5071044/diff/43001/src/pkg/testing/testing.go#n...
src/pkg/testing/testing.go:244: numParallel --
On 2011/10/06 03:33:31, r wrote:
> did this run through gofmt? there's a space before the --
> please fix.

Done.

[tebeka, 2011-10-06 16:52:44]

> Now we're down to one keystroke :)
Finally, this has been a humbling experience. Thanks for all the help.
>
> Also please fill in the CLA as described in contribute.html if you haven't
> already.
CLA filled.

[r, 2011-10-06 16:58:41]

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

testing: Add support for running tests in parallel (t.Parallel API).

See discussion at
https://groups.google.com/d/topic/golang-dev/RAKiqi44GEU/discussion

R=golang-dev, bradfitz, dvyukov, rogpeppe, r, r, borman
CC=golang-dev
http://codereview.appspot.com/5071044

Committer: Rob Pike <r@golang.org>

[tebeka, 2011-12-15 18:35:08]

*** Abandoned ***