internal/lsp/cache: two minor optimizations
1. Avoid unnecessary intermediate map updates.
2. Avoid accumulating defers in a loop when the control is simple.
Yield: -10% CPU, -37% allocs.
Typical results:
$ go test -v ./gopls/internal/regtest/bench -run=TestBenchmarkDidChange -didchange_dir=$HOME/w/kubernetes -didchange_file=pkg/util/hash/hash.go
Before:
BenchmarkStatistics 100 25932206 ns/op 11684109 B/op 75458 allocs/op
After:
BenchmarkStatistics 100 23294195 ns/op 11293472 B/op 47299 allocs/op
Also, move profiling logic outside the loop so that later runs
don't overwrite earlier runs. (This doesn't appear to be a problem
in practice, presumably because the last run is the big one.)
Updates golang/go#45686
Change-Id: I538ca6bb88cc18f1eefe35d2db29a62e5190280e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/410697
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go
index f37c2f6..5e4eb5f 100644
--- a/gopls/internal/regtest/bench/bench_test.go
+++ b/gopls/internal/regtest/bench/bench_test.go
@@ -163,19 +163,22 @@
env.Await(env.DoneWithOpen())
// Insert the text we'll be modifying at the top of the file.
env.EditBuffer(*benchFile, fake.Edit{Text: "// __REGTEST_PLACEHOLDER_0__\n"})
- result := testing.Benchmark(func(b *testing.B) {
- if *benchProfile != "" {
- profile, err := os.Create(*benchProfile)
- if err != nil {
- t.Fatal(err)
- }
- defer profile.Close()
- if err := pprof.StartCPUProfile(profile); err != nil {
- t.Fatal(err)
- }
- defer pprof.StopCPUProfile()
+
+ // Run the profiler after the initial load,
+ // across all benchmark iterations.
+ if *benchProfile != "" {
+ profile, err := os.Create(*benchProfile)
+ if err != nil {
+ t.Fatal(err)
}
- b.ResetTimer()
+ defer profile.Close()
+ if err := pprof.StartCPUProfile(profile); err != nil {
+ t.Fatal(err)
+ }
+ defer pprof.StopCPUProfile()
+ }
+
+ result := testing.Benchmark(func(b *testing.B) {
for i := 0; i < b.N; i++ {
env.EditBuffer(*benchFile, fake.Edit{
Start: fake.Pos{Line: 0, Column: 0},
@@ -185,7 +188,6 @@
})
env.Await(StartedChange(uint64(i + 1)))
}
- b.StopTimer()
})
printBenchmarkResults(result)
})
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 9beb9e6..c7cd337 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -1985,9 +1985,9 @@
deleteInvalidMetadata := forceReloadMetadata || workspaceModeChanged
idsInSnapshot := map[PackageID]bool{} // track all known IDs
for uri, ids := range s.ids {
+ var resultIDs []PackageID
for _, id := range ids {
- invalidateMetadata := idsToInvalidate[id]
- if skipID[id] || (invalidateMetadata && deleteInvalidMetadata) {
+ if skipID[id] || deleteInvalidMetadata && idsToInvalidate[id] {
continue
}
// The ID is not reachable from any workspace package, so it should
@@ -1996,8 +1996,9 @@
continue
}
idsInSnapshot[id] = true
- result.ids[uri] = append(result.ids[uri], id)
+ resultIDs = append(resultIDs, id)
}
+ result.ids[uri] = resultIDs
}
// Copy the package metadata. We only need to invalidate packages directly
diff --git a/internal/memoize/memoize.go b/internal/memoize/memoize.go
index b3f93b6..89f79c6 100644
--- a/internal/memoize/memoize.go
+++ b/internal/memoize/memoize.go
@@ -241,11 +241,11 @@
}
h.mu.Lock()
- defer h.mu.Unlock()
if h.state == stateDestroyed {
panic(fmt.Sprintf("inheriting destroyed handle %#v (type %T) into generation %v", h.key, h.key, g.name))
}
h.generations[g] = struct{}{}
+ h.mu.Unlock()
}
}
