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>
diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index 668c437..ab55743 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -278,7 +278,7 @@
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 ca61286..bb402cf 100644
--- a/internal/lsp/debug/trace.go
+++ b/internal/lsp/debug/trace.go
@@ -119,8 +119,6 @@
}
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 @@
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 @@
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 @@
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 @@
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 {
