From 381ac87aae563aca1b0fcf5ced49d661e1dba9f4 Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Thu, 16 Jun 2022 16:22:28 -0400
Subject: [PATCH] internal/lsp/debug: reduce critical sections in trace

This change reduces the sizes of the critical section
in traces.ProcessEvent, in particular moving allocations
ahead of Lock. This reduces the contention according
to the trace profiler.

See https://go-review.googlesource.com/c/go/+/411909 for
another reduction in contention. The largest remaining
contention is Handle.Get, which thousands of goroutines
wait for because we initiate typechecking top down.

(Second attempt at https://go-review.googlesource.com/c/tools/+/411910,
reverted in https://go-review.googlesource.com/c/tools/+/412694.
The changes to Generation.Bind have been dropped.)

Change-Id: Ia9050c97bd12d2d75055f8d1dfcda3ef1f5ad334
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412820
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
---
 internal/lsp/cache/parse.go |  2 +-
 internal/lsp/debug/trace.go | 39 ++++++++++++++++++++++---------------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index 668c437f5c..ab55743ccf 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -278,7 +278,7 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod
 
 	tok := fset.File(file.Pos())
 	if tok == nil {
-		// file.Pos is the location of the package declaration. If there was
+		// file.Pos is the location of the package declaration (issue #53202). If there was
 		// none, we can't find the token.File that ParseFile created, and we
 		// have no choice but to recreate it.
 		tok = fset.AddFile(fh.URI().Filename(), -1, len(src))
diff --git a/internal/lsp/debug/trace.go b/internal/lsp/debug/trace.go
index ca612867a5..bb402cfaa8 100644
--- a/internal/lsp/debug/trace.go
+++ b/internal/lsp/debug/trace.go
@@ -119,8 +119,6 @@ func formatEvent(ctx context.Context, ev core.Event, lm label.Map) string {
 }
 
 func (t *traces) ProcessEvent(ctx context.Context, ev core.Event, lm label.Map) context.Context {
-	t.mu.Lock()
-	defer t.mu.Unlock()
 	span := export.GetSpan(ctx)
 	if span == nil {
 		return ctx
@@ -128,11 +126,8 @@ func (t *traces) ProcessEvent(ctx context.Context, ev core.Event, lm label.Map)
 
 	switch {
 	case event.IsStart(ev):
-		if t.sets == nil {
-			t.sets = make(map[string]*traceSet)
-			t.unfinished = make(map[export.SpanContext]*traceData)
-		}
-		// just starting, add it to the unfinished map
+		// Just starting: add it to the unfinished map.
+		// Allocate before the critical section.
 		td := &traceData{
 			TraceID:  span.ID.TraceID,
 			SpanID:   span.ID.SpanID,
@@ -141,6 +136,13 @@ func (t *traces) ProcessEvent(ctx context.Context, ev core.Event, lm label.Map)
 			Start:    span.Start().At(),
 			Tags:     renderLabels(span.Start()),
 		}
+
+		t.mu.Lock()
+		defer t.mu.Unlock()
+		if t.sets == nil {
+			t.sets = make(map[string]*traceSet)
+			t.unfinished = make(map[export.SpanContext]*traceData)
+		}
 		t.unfinished[span.ID] = td
 		// and wire up parents if we have them
 		if !span.ParentID.IsValid() {
@@ -155,7 +157,19 @@ func (t *traces) ProcessEvent(ctx context.Context, ev core.Event, lm label.Map)
 		parent.Children = append(parent.Children, td)
 
 	case event.IsEnd(ev):
-		// finishing, must be already in the map
+		// Finishing: must be already in the map.
+		// Allocate events before the critical section.
+		events := span.Events()
+		tdEvents := make([]traceEvent, len(events))
+		for i, event := range events {
+			tdEvents[i] = traceEvent{
+				Time: event.At(),
+				Tags: renderLabels(event),
+			}
+		}
+
+		t.mu.Lock()
+		defer t.mu.Unlock()
 		td, found := t.unfinished[span.ID]
 		if !found {
 			return ctx // if this happens we are in a bad place
@@ -164,14 +178,7 @@ func (t *traces) ProcessEvent(ctx context.Context, ev core.Event, lm label.Map)
 
 		td.Finish = span.Finish().At()
 		td.Duration = span.Finish().At().Sub(span.Start().At())
-		events := span.Events()
-		td.Events = make([]traceEvent, len(events))
-		for i, event := range events {
-			td.Events[i] = traceEvent{
-				Time: event.At(),
-				Tags: renderLabels(event),
-			}
-		}
+		td.Events = tdEvents
 
 		set, ok := t.sets[span.Name]
 		if !ok {