go/internal/gcimporter: avoid setting unnecessary lines in fakeFileSet
As discussed in golang/go#46586, the gcimporter spends large portion of
time just validating fake lines.
Fix this by only setting the lines we need. Benchcmd results for
TestImportStdLib:
name old time/op new time/op delta
TestImportStdLib 1.11s ±40% 0.57s ±58% -48.17% (p=0.000 n=18+18)
name old user-time/op new user-time/op delta
TestImportStdLib 1.24s ±46% 0.71s ±61% -42.84% (p=0.000 n=18+18)
name old sys-time/op new sys-time/op delta
TestImportStdLib 249ms ±64% 139ms ±47% -44.19% (p=0.000 n=19+18)
name old peak-RSS-bytes new peak-RSS-bytes delta
TestImportStdLib 80.1MB ±24% 86.6MB ±13% +8.06% (p=0.001 n=20+20)
Fixes golang/go#46586
Change-Id: I5ef99ea9f242fc80d2d17750a8d9b28f7fc71245
Reviewed-on: https://go-review.googlesource.com/c/tools/+/357291
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
diff --git a/go/internal/gcimporter/bimport.go b/go/internal/gcimporter/bimport.go
index b023120..b85de01 100644
--- a/go/internal/gcimporter/bimport.go
+++ b/go/internal/gcimporter/bimport.go
@@ -74,9 +74,10 @@
pathList: []string{""}, // empty string is mapped to 0
fake: fakeFileSet{
fset: fset,
- files: make(map[string]*token.File),
+ files: make(map[string]*fileInfo),
},
}
+ defer p.fake.setLines() // set lines for files in fset
// read version info
var versionstr string
@@ -338,37 +339,49 @@
// Synthesize a token.Pos
type fakeFileSet struct {
fset *token.FileSet
- files map[string]*token.File
+ files map[string]*fileInfo
}
+type fileInfo struct {
+ file *token.File
+ lastline int
+}
+
+const maxlines = 64 * 1024
+
func (s *fakeFileSet) pos(file string, line, column int) token.Pos {
// TODO(mdempsky): Make use of column.
- // Since we don't know the set of needed file positions, we
- // reserve maxlines positions per file.
- const maxlines = 64 * 1024
+ // Since we don't know the set of needed file positions, we reserve maxlines
+ // positions per file. We delay calling token.File.SetLines until all
+ // positions have been calculated (by way of fakeFileSet.setLines), so that
+ // we can avoid setting unnecessary lines. See also golang/go#46586.
f := s.files[file]
if f == nil {
- f = s.fset.AddFile(file, -1, maxlines)
+ f = &fileInfo{file: s.fset.AddFile(file, -1, maxlines)}
s.files[file] = f
- // Allocate the fake linebreak indices on first use.
- // TODO(adonovan): opt: save ~512KB using a more complex scheme?
- fakeLinesOnce.Do(func() {
- fakeLines = make([]int, maxlines)
- for i := range fakeLines {
- fakeLines[i] = i
- }
- })
- f.SetLines(fakeLines)
}
-
if line > maxlines {
line = 1
}
+ if line > f.lastline {
+ f.lastline = line
+ }
- // Treat the file as if it contained only newlines
- // and column=1: use the line number as the offset.
- return f.Pos(line - 1)
+ // Return a fake position assuming that f.file consists only of newlines.
+ return token.Pos(f.file.Base() + line - 1)
+}
+
+func (s *fakeFileSet) setLines() {
+ fakeLinesOnce.Do(func() {
+ fakeLines = make([]int, maxlines)
+ for i := range fakeLines {
+ fakeLines[i] = i
+ }
+ })
+ for _, f := range s.files {
+ f.file.SetLines(fakeLines[:f.lastline])
+ }
}
var (
diff --git a/go/internal/gcimporter/iimport.go b/go/internal/gcimporter/iimport.go
index 58d4e2b..806030d 100644
--- a/go/internal/gcimporter/iimport.go
+++ b/go/internal/gcimporter/iimport.go
@@ -159,9 +159,10 @@
fake: fakeFileSet{
fset: fset,
- files: make(map[string]*token.File),
+ files: make(map[string]*fileInfo),
},
}
+ defer p.fake.setLines() // set lines for files in fset
for i, pt := range predeclared() {
p.typCache[uint64(i)] = pt
