From 63dfc2d3a9c936e6c11ef46dc4b4a899ea7e1e70 Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Mon, 6 Jun 2022 16:24:52 -0400
Subject: [PATCH] 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>
---
 gopls/internal/regtest/bench/bench_test.go | 28 ++++++++++++----------
 internal/lsp/cache/snapshot.go             |  7 +++---
 internal/memoize/memoize.go                |  2 +-
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go
index f37c2f6da20..5e4eb5fc23a 100644
--- a/gopls/internal/regtest/bench/bench_test.go
+++ b/gopls/internal/regtest/bench/bench_test.go
@@ -163,19 +163,22 @@ func TestBenchmarkDidChange(t *testing.T) {
 		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)
+			}
+			defer profile.Close()
+			if err := pprof.StartCPUProfile(profile); err != nil {
+				t.Fatal(err)
 			}
-			b.ResetTimer()
+			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 @@ func TestBenchmarkDidChange(t *testing.T) {
 				})
 				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 9beb9e6579e..c7cd337184c 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -1985,9 +1985,9 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 	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 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 				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 b3f93b6238b..89f79c68b7d 100644
--- a/internal/memoize/memoize.go
+++ b/internal/memoize/memoize.go
@@ -241,11 +241,11 @@ func (g *Generation) Inherit(hs ...*Handle) {
 		}
 
 		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()
 	}
 }