From 3566f695a758c57ebe9bf6aca724b60f1a0c961e Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Fri, 28 Oct 2022 15:45:27 -0400
Subject: [PATCH] gopls/internal/lsp/source: minor space optimizations

Memory profiles show heavy allocation for the stack
and the function closure of FindDeclAndField.
This change moves both outside the loop, reducing
this function's fraction of allocation from 6.7%
before to 5.0% after, and reducing running time
by 3-7% on these benchmarks.

before
BenchmarkStructCompletion/completion-8               100          10432280 ns/op
BenchmarkImportCompletion/completion-8              1350            921785 ns/op
BenchmarkSliceCompletion/completion-8                100          10876852 ns/op
BenchmarkFuncDeepCompletion/completion-8             142           7136768 ns/op
BenchmarkCompletionFollowingEdit/completion-8         63          21267031 ns/op

After
BenchmarkStructCompletion/completion-8               100          10030458 ns/op
BenchmarkImportCompletion/completion-8              1311            918306 ns/op
BenchmarkSliceCompletion/completion-8                100          10179937 ns/op
BenchmarkFuncDeepCompletion/completion-8             150           6986303 ns/op
BenchmarkCompletionFollowingEdit/completion-8         63          20575987 ns/op

Change-Id: Ia459e41ecf20851ff4544f76ad7b415a24606cd1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446185
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
---
 gopls/internal/lsp/source/hover.go | 120 +++++++++++++++--------------
 1 file changed, 61 insertions(+), 59 deletions(-)

diff --git a/gopls/internal/lsp/source/hover.go b/gopls/internal/lsp/source/hover.go
index c758d688dd..09f7224c80 100644
--- a/gopls/internal/lsp/source/hover.go
+++ b/gopls/internal/lsp/source/hover.go
@@ -904,78 +904,80 @@ func FindDeclAndField(files []*ast.File, pos token.Pos) (decl ast.Decl, field *a
 	}()
 
 	// Visit the files in search of the node at pos.
-	var stack []ast.Node
-	for _, file := range files {
-		ast.Inspect(file, func(n ast.Node) bool {
-			if n != nil {
-				stack = append(stack, n) // push
-			} else {
-				stack = stack[:len(stack)-1] // pop
-				return false
-			}
+	stack := make([]ast.Node, 0, 20)
+	// Allocate the closure once, outside the loop.
+	f := func(n ast.Node) bool {
+		if n != nil {
+			stack = append(stack, n) // push
+		} else {
+			stack = stack[:len(stack)-1] // pop
+			return false
+		}
 
-			// Skip subtrees (incl. files) that don't contain the search point.
-			if !(n.Pos() <= pos && pos < n.End()) {
-				return false
-			}
+		// Skip subtrees (incl. files) that don't contain the search point.
+		if !(n.Pos() <= pos && pos < n.End()) {
+			return false
+		}
 
-			switch n := n.(type) {
-			case *ast.Field:
-				checkField := func(f ast.Node) {
-					if f.Pos() == pos {
-						field = n
-						for i := len(stack) - 1; i >= 0; i-- {
-							if d, ok := stack[i].(ast.Decl); ok {
-								decl = d // innermost enclosing decl
-								break
-							}
+		switch n := n.(type) {
+		case *ast.Field:
+			checkField := func(f ast.Node) {
+				if f.Pos() == pos {
+					field = n
+					for i := len(stack) - 1; i >= 0; i-- {
+						if d, ok := stack[i].(ast.Decl); ok {
+							decl = d // innermost enclosing decl
+							break
 						}
-						panic(nil) // found
 					}
-				}
-
-				// Check *ast.Field itself. This handles embedded
-				// fields which have no associated *ast.Ident name.
-				checkField(n)
-
-				// Check each field name since you can have
-				// multiple names for the same type expression.
-				for _, name := range n.Names {
-					checkField(name)
-				}
-
-				// Also check "X" in "...X". This makes it easy
-				// to format variadic signature params properly.
-				if ell, ok := n.Type.(*ast.Ellipsis); ok && ell.Elt != nil {
-					checkField(ell.Elt)
-				}
-
-			case *ast.FuncDecl:
-				if n.Name.Pos() == pos {
-					decl = n
 					panic(nil) // found
 				}
+			}
 
-			case *ast.GenDecl:
-				for _, spec := range n.Specs {
-					switch spec := spec.(type) {
-					case *ast.TypeSpec:
-						if spec.Name.Pos() == pos {
+			// Check *ast.Field itself. This handles embedded
+			// fields which have no associated *ast.Ident name.
+			checkField(n)
+
+			// Check each field name since you can have
+			// multiple names for the same type expression.
+			for _, name := range n.Names {
+				checkField(name)
+			}
+
+			// Also check "X" in "...X". This makes it easy
+			// to format variadic signature params properly.
+			if ell, ok := n.Type.(*ast.Ellipsis); ok && ell.Elt != nil {
+				checkField(ell.Elt)
+			}
+
+		case *ast.FuncDecl:
+			if n.Name.Pos() == pos {
+				decl = n
+				panic(nil) // found
+			}
+
+		case *ast.GenDecl:
+			for _, spec := range n.Specs {
+				switch spec := spec.(type) {
+				case *ast.TypeSpec:
+					if spec.Name.Pos() == pos {
+						decl = n
+						panic(nil) // found
+					}
+				case *ast.ValueSpec:
+					for _, id := range spec.Names {
+						if id.Pos() == pos {
 							decl = n
 							panic(nil) // found
 						}
-					case *ast.ValueSpec:
-						for _, id := range spec.Names {
-							if id.Pos() == pos {
-								decl = n
-								panic(nil) // found
-							}
-						}
 					}
 				}
 			}
-			return true
-		})
+		}
+		return true
+	}
+	for _, file := range files {
+		ast.Inspect(file, f)
 	}
 
 	return nil, nil