go/token: fix data race on FileSet.last

Fixes #4345.

Benchmarks are promising,

benchmark         old ns/op    new ns/op    delta
BenchmarkPrint     14716391     14747131   +0.21%

benchmark         old ns/op    new ns/op    delta
BenchmarkParse      8846219      8809343   -0.42%

benchmark          old MB/s     new MB/s  speedup
BenchmarkParse         6.61         6.64    1.00x

Also includes additional tests to improve token.FileSet coverage.

R=dvyukov, gri
CC=golang-dev
https://golang.org/cl/6968044

diff --git a/src/pkg/go/token/position.go b/src/pkg/go/token/position.go
index fc45c1e7693efd56fc38784556f4c7e27d6602a3..f5d999561862581049092f0473c3767c53a19229 100644 (file)
--- a/src/pkg/go/token/position.go
+++ b/src/pkg/go/token/position.go
@@ -295,9 +295,9 @@ type FileSet struct {
 
 // NewFileSet creates a new file set.
 func NewFileSet() *FileSet {
-       s := new(FileSet)
-       s.base = 1 // 0 == NoPos
-       return s
+       return &FileSet{
+               base: 1, // 0 == NoPos
+       }
 }
 
 // Base returns the minimum base offset that must be provided to
@@ -367,8 +367,10 @@ func searchFiles(a []*File, x int) int {
 }
 
 func (s *FileSet) file(p Pos) *File {
+       s.mutex.RLock()
        // common case: p is in last file
        if f := s.last; f != nil && f.base <= int(p) && int(p) <= f.base+f.size {
+               s.mutex.RUnlock()
                return f
        }
        // p is not in last file - search all files
@@ -376,10 +378,14 @@ func (s *FileSet) file(p Pos) *File {
                f := s.files[i]
                // f.base <= int(p) by definition of searchFiles
                if int(p) <= f.base+f.size {
-                       s.last = f
+                       s.mutex.RUnlock()
+                       s.mutex.Lock()
+                       s.last = f // race is ok - s.last is only a cache
+                       s.mutex.Unlock()
                        return f
                }
        }
+       s.mutex.RUnlock()
        return nil
 }
 
@@ -389,9 +395,7 @@ func (s *FileSet) file(p Pos) *File {
 //
 func (s *FileSet) File(p Pos) (f *File) {
        if p != NoPos {
-               s.mutex.RLock()
                f = s.file(p)
-               s.mutex.RUnlock()
        }
        return
 }
@@ -399,11 +403,9 @@ func (s *FileSet) File(p Pos) (f *File) {
 // Position converts a Pos in the fileset into a general Position.
 func (s *FileSet) Position(p Pos) (pos Position) {
        if p != NoPos {
-               s.mutex.RLock()
                if f := s.file(p); f != nil {
                        pos = f.position(p)
                }
-               s.mutex.RUnlock()
        }
        return
 }

diff --git a/src/pkg/go/token/position_test.go b/src/pkg/go/token/position_test.go
index 3e7d552b7541c2b25d1bca1d92cc15101c39e514..1d36c22268d3f6b94b3bde4448e0bac9c84576cd 100644 (file)
--- a/src/pkg/go/token/position_test.go
+++ b/src/pkg/go/token/position_test.go
@@ -182,6 +182,32 @@ func TestFiles(t *testing.T) {
        }
 }
 
+// FileSet.File should return nil if Pos is past the end of the FileSet.
+func TestFileSetPastEnd(t *testing.T) {
+       fset := NewFileSet()
+       for _, test := range tests {
+               fset.AddFile(test.filename, fset.Base(), test.size)
+       }
+       if f := fset.File(Pos(fset.Base())); f != nil {
+               t.Errorf("expected nil, got %v", f)
+       }
+}
+
+func TestFileSetCacheUnlikely(t *testing.T) {
+       fset := NewFileSet()
+       offsets := make(map[string]int)
+       for _, test := range tests {
+               offsets[test.filename] = fset.Base()
+               fset.AddFile(test.filename, fset.Base(), test.size)
+       }
+       for file, pos := range offsets {
+               f := fset.File(Pos(pos))
+               if f.Name() != file {
+                       t.Errorf("expecting %q at position %d, got %q", file, pos, f.Name())
+               }
+       }
+}
+
 // issue 4345. Test concurrent use of FileSet.Pos does not trigger a
 // race in the FileSet position cache.
 func TestFileSetRace(t *testing.T) {