Issue 6940078: code review 6940078: go/token: add test for concurrent use of FileSet.Pos (Closed)
|
Created:
11 years, 4 months ago by dfc Modified:
11 years, 4 months ago Reviewers:
CC:
dvyukov, gri, albert.strasheim, golang-dev Visibility:
Public. |
Descriptiongo/token: add test for concurrent use of FileSet.Pos
Update issue 4354.
Add a test to expose the race in the FileSet position cache.
Patch Set 1 #Patch Set 2 : diff -r 3684de5292bf https://code.google.com/p/go #Patch Set 3 : diff -r 3684de5292bf https://code.google.com/p/go #
Total comments: 4
Patch Set 4 : diff -r 3684de5292bf https://code.google.com/p/go #Patch Set 5 : diff -r 3684de5292bf https://code.google.com/p/go #Patch Set 6 : diff -r 3684de5292bf https://code.google.com/p/go #
Total comments: 4
Patch Set 7 : diff -r 3684de5292bf https://code.google.com/p/go #MessagesTotal messages: 14
Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com, golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_te... File src/pkg/go/token/position_test.go (right): https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_te... src/pkg/go/token/position_test.go:189: defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU())) Is it required? Race detector does not need GOMAXPROCS to find races. It can also be executed on a machine with 1 core.
Sign in to reply to this message.
leaving the rest for gri
Sign in to reply to this message.
> Is it required?
> Race detector does not need GOMAXPROCS to find races. It can also be
> executed on a machine with 1 core.
The race detector does not report this version races unless GOMAXPROCS > 1
func TestFileSetRace(t *testing.T) {
fset := NewFileSet()
for i := 0; i < 100; i++ {
fset.AddFile(fmt.Sprintf("file-%d", i), fset.Base(), 1031)
}
max := int32(fset.Base())
var wg sync.WaitGroup
for i := 0; i < 16; i++ {
wg.Add(1)
go func() {
defer wg.Done()
for i := 0 ; i < 10000 ; i++ {
p := Pos(rand.Int31n(max))
fset.Position(p)
}
}()
}
wg.Wait()
}
Sign in to reply to this message.
Humm... I think it's possible. First, the goroutines may run till completion w/o blocking calls, and the second is that I suspect rand.Int31n() contains internal mutex and so synchronizes goroutines. Perhaps set GOMAXPROCS to 4 then. On Tue, Dec 18, 2012 at 3:12 PM, Dave Cheney <dave@cheney.net> wrote: > > Is it required? > > Race detector does not need GOMAXPROCS to find races. It can also be > > executed on a machine with 1 core. > > The race detector does not report this version races unless GOMAXPROCS > 1 > > func TestFileSetRace(t *testing.T) { > fset := NewFileSet() > for i := 0; i < 100; i++ { > fset.AddFile(fmt.Sprintf("file-%d", i), fset.Base(), 1031) > } > max := int32(fset.Base()) > var wg sync.WaitGroup > for i := 0; i < 16; i++ { > wg.Add(1) > go func() { > defer wg.Done() > for i := 0 ; i < 10000 ; i++ { > p := Pos(rand.Int31n(max)) > fset.Position(p) > } > }() > } > wg.Wait() > } >
Sign in to reply to this message.
some minor comments otherwise looks great; just what I was waiting for https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_te... File src/pkg/go/token/position_test.go (right): https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_te... src/pkg/go/token/position_test.go:196: for i := 0; i < runtime.GOMAXPROCS(0)*2; i++ { maybe count down? (no need to call GOMAXPROCS repeatedly). for i := runtime.GOMAXPROCS(0)*2; i > 0; i-- https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_te... src/pkg/go/token/position_test.go:204: fset.Position(p) fset.Position(Pos(rand.Int31n(max))) https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_te... src/pkg/go/token/position_test.go:209: <-time.After(200 * time.Millisecond) time.Sleep(200 * time.Millisecond)
Sign in to reply to this message.
Dmitry, Robert, please take another look. I have removed the blocking on the
shared math.Rand and the race detector no longer needs GOMAXPROCS > 1 to fire.
Dmitry: there is one issue
==================
WARNING: DATA RACE
Read by goroutine 8:
go/token.(*FileSet).file()
/home/dfc/go/src/pkg/go/token/position.go:371 +0x38
go/token.(*FileSet).Position()
/home/dfc/go/src/pkg/go/token/position.go:403 +0x70
go/token.func·004()
/home/dfc/go/src/pkg/go/token/position_test.go:201 +0xbd
Previous write by goroutine 7:
[failed to restore the stack]
Goroutine 8 (running) created at:
go/token.TestFileSetRace()
/home/dfc/go/src/pkg/go/token/position_test.go:203 +0x46e
testing.tRunner()
/home/dfc/go/src/pkg/testing/testing.go:302 +0xe8
Goroutine 7 (finished) created at:
go/token.TestFileSetRace()
/home/dfc/go/src/pkg/go/token/position_test.go:203 +0x46e
testing.tRunner()
/home/dfc/go/src/pkg/testing/testing.go:302 +0xe8
==================
Note the missing stack trace in the 2nd goroutine.
Sign in to reply to this message.
Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com, golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
FYI https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_t... File src/pkg/go/token/position_test.go (right): https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_t... src/pkg/go/token/position_test.go:199: defer stop.Done() just call stop.Done() at the end - defer seems overkill https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_t... src/pkg/go/token/position_test.go:205: stop.Wait() if the race shows up very quickly, just use time.Sleep() here (with a short time) and then this test's runt-time is independent of machine. And then you don't need the WaitGroup and the inner loop can just be endless. Even less code overall.
Sign in to reply to this message.
Thanks for your comments. https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_t... File src/pkg/go/token/position_test.go (right): https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_t... src/pkg/go/token/position_test.go:199: defer stop.Done() On 2012/12/18 22:55:00, gri wrote: > just call stop.Done() at the end - defer seems overkill Done. https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_t... src/pkg/go/token/position_test.go:205: stop.Wait() On 2012/12/18 22:55:00, gri wrote: > if the race shows up very quickly, just use time.Sleep() here (with a short > time) and then this test's runt-time is independent of machine. And then you > don't need the WaitGroup and the inner loop can just be endless. Even less code > overall. I'm concerned about leaving these goroutines running after the Test is finished. I've reduced the loop count to 1000 which is sufficient for triggering the race. The run time on the slowest arm host that I have for go test go/token is 300ms, much less than the time it takes to compile and link the test binary.
Sign in to reply to this message.
LGTM. Thanks.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=e7cd0a82d669 *** go/token: add test for concurrent use of FileSet.Pos Update issue 4354. Add a test to expose the race in the FileSet position cache. R=dvyukov, gri CC=fullung, golang-dev https://codereview.appspot.com/6940078 Committer: Robert Griesemer <gri@golang.org>
Sign in to reply to this message.
On Wed, Dec 19, 2012 at 2:03 AM, <dave@cheney.net> wrote: > Dmitry, Robert, please take another look. I have removed the blocking on > the shared math.Rand and the race detector no longer needs GOMAXPROCS > > 1 to fire. > > Dmitry: there is one issue > > ================== > WARNING: DATA RACE > Read by goroutine 8: > go/token.(*FileSet).file() > /home/dfc/go/src/pkg/go/token/**position.go:371 +0x38 > go/token.(*FileSet).Position() > /home/dfc/go/src/pkg/go/token/**position.go:403 +0x70 > go/token.func·004() > /home/dfc/go/src/pkg/go/token/**position_test.go:201 +0xbd > > Previous write by goroutine 7: > [failed to restore the stack] > > Goroutine 8 (running) created at: > go/token.TestFileSetRace() > /home/dfc/go/src/pkg/go/token/**position_test.go:203 +0x46e > testing.tRunner() > /home/dfc/go/src/pkg/testing/**testing.go:302 +0xe8 > > Goroutine 7 (finished) created at: > go/token.TestFileSetRace() > /home/dfc/go/src/pkg/go/token/**position_test.go:203 +0x46e > testing.tRunner() > /home/dfc/go/src/pkg/testing/**testing.go:302 +0xe8 > > ================== > > Note the missing stack trace in the 2nd goroutine. Let's see whether documentation is usable :) http://code.google.com/p/go-wiki/wiki/RaceDetector
Sign in to reply to this message.
|
