go/token: use fine-grained locking in FileSet

Before, all accesses to the lines and infos tables of each File were
serialized by the lock of the owning FileSet, causing parsers running
in parallel to contend.  Now, each File has its own mutex.

This fixes a data race in (*File).PositionFor, which used to call
f.position then f.unpack without holding the mutex's lock.

Fixes golang/go#18348

Change-Id: Iaa5989b2eba88a7fb2e91c1a0a8bc1e7f6497f2b
Reviewed-on: https://go-review.googlesource.com/34591
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/go/token/position.go

@@ -94,7 +94,8 @@ type File struct {
 	base int    // Pos value range for this file is [base...base+size]
 	size int    // file size as provided to AddFile
 
-	// lines and infos are protected by set.mutex
+	// lines and infos are protected by mutex
+	mutex sync.Mutex
 	lines []int // lines contains the offset of the first character for each line (the first entry is always 0)
 	infos []lineInfo
 }
@@ -116,9 +117,9 @@ func (f *File) Size() int {
 
 // LineCount returns the number of lines in file f.
 func (f *File) LineCount() int {
-	f.set.mutex.RLock()
+	f.mutex.Lock()
 	n := len(f.lines)
-	f.set.mutex.RUnlock()
+	f.mutex.Unlock()
 	return n
 }
 
@@ -127,11 +128,11 @@ func (f *File) LineCount() int {
 // and smaller than the file size; otherwise the line offset is ignored.
 //
 func (f *File) AddLine(offset int) {
-	f.set.mutex.Lock()
+	f.mutex.Lock()
 	if i := len(f.lines); (i == 0 || f.lines[i-1] < offset) && offset < f.size {
 		f.lines = append(f.lines, offset)
 	}
-	f.set.mutex.Unlock()
+	f.mutex.Unlock()
 }
 
 // MergeLine merges a line with the following line. It is akin to replacing
@@ -143,8 +144,8 @@ func (f *File) MergeLine(line int) {
 	if line <= 0 {
 		panic("illegal line number (line numbering starts at 1)")
 	}
-	f.set.mutex.Lock()
+	f.mutex.Lock()
-	defer f.set.mutex.Unlock()
+	defer f.mutex.Unlock()
 	if line >= len(f.lines) {
 		panic("illegal line number")
 	}
@@ -176,9 +177,9 @@ func (f *File) SetLines(lines []int) bool {
 	}
 
 	// set lines table
-	f.set.mutex.Lock()
+	f.mutex.Lock()
 	f.lines = lines
-	f.set.mutex.Unlock()
+	f.mutex.Unlock()
 	return true
 }
 
@@ -198,9 +199,9 @@ func (f *File) SetLinesForContent(content []byte) {
 	}
 
 	// set lines table
-	f.set.mutex.Lock()
+	f.mutex.Lock()
 	f.lines = lines
-	f.set.mutex.Unlock()
+	f.mutex.Unlock()
 }
 
 // A lineInfo object describes alternative file and line number
@@ -222,11 +223,11 @@ type lineInfo struct {
 // information for //line filename:line comments in source files.
 //
 func (f *File) AddLineInfo(offset int, filename string, line int) {
-	f.set.mutex.Lock()
+	f.mutex.Lock()
 	if i := len(f.infos); i == 0 || f.infos[i-1].Offset < offset && offset < f.size {
 		f.infos = append(f.infos, lineInfo{offset, filename, line})
 	}
-	f.set.mutex.Unlock()
+	f.mutex.Unlock()
 }
 
 // Pos returns the Pos value for the given file offset;
@@ -267,6 +268,8 @@ func searchLineInfos(a []lineInfo, x int) int {
 // possibly adjusted by //line comments; otherwise those comments are ignored.
 //
 func (f *File) unpack(offset int, adjusted bool) (filename string, line, column int) {
+	f.mutex.Lock()
+	defer f.mutex.Unlock()
 	filename = f.name
 	if i := searchInts(f.lines, offset); i >= 0 {
 		line, column = i+1, offset-f.lines[i]+1
@@ -371,7 +374,7 @@ func (s *FileSet) AddFile(filename string, base, size int) *File {
 		panic("illegal base or size")
 	}
 	// base >= s.base && size >= 0
-	f := &File{s, filename, base, size, []int{0}, nil}
+	f := &File{set: s, name: filename, base: base, size: size, lines: []int{0}}
 	base += size + 1 // +1 because EOF also has a position
 	if base < 0 {
 		panic("token.Pos offset overflow (> 2G of source code in file set)")
@@ -446,9 +449,7 @@ func (s *FileSet) File(p Pos) (f *File) {
 func (s *FileSet) PositionFor(p Pos, adjusted bool) (pos Position) {
 	if p != NoPos {
 		if f := s.file(p); f != nil {
-			s.mutex.RLock()
+			return f.position(p, adjusted)
-			pos = f.position(p, adjusted)
-			s.mutex.RUnlock()
 		}
 	}
 	return

src/go/token/serialize.go

@@ -30,7 +30,14 @@ func (s *FileSet) Read(decode func(interface{}) error) error {
 	files := make([]*File, len(ss.Files))
 	for i := 0; i < len(ss.Files); i++ {
 		f := &ss.Files[i]
-		files[i] = &File{s, f.Name, f.Base, f.Size, f.Lines, f.Infos}
+		files[i] = &File{
+			set:   s,
+			name:  f.Name,
+			base:  f.Base,
+			size:  f.Size,
+			lines: f.Lines,
+			infos: f.Infos,
+		}
 	}
 	s.files = files
 	s.last = nil
@@ -47,7 +54,15 @@ func (s *FileSet) Write(encode func(interface{}) error) error {
 	ss.Base = s.base
 	files := make([]serializedFile, len(s.files))
 	for i, f := range s.files {
-		files[i] = serializedFile{f.name, f.base, f.size, f.lines, f.infos}
+		f.mutex.Lock()
+		files[i] = serializedFile{
+			Name:  f.name,
+			Base:  f.base,
+			Size:  f.size,
+			Lines: append([]int(nil), f.lines...),
+			Infos: append([]lineInfo(nil), f.infos...),
+		}
+		f.mutex.Unlock()
 	}
 	ss.Files = files
 	s.mutex.Unlock()