diff options
author | Alan Donovan <adonovan@google.com> | 2022-06-16 13:41:08 +0000 |
---|---|---|
committer | Alan Donovan <adonovan@google.com> | 2022-06-16 15:42:24 +0000 |
commit | 041035c34a090a6505169f0dbb856b7ea9ce2ffa (patch) | |
tree | f9b7e8298d3cfe3cd0a2d585875d546f051c4284 | |
parent | d097bc9f9d0168a89c8d85f5e2110d54bcbab78e (diff) | |
download | golang-x-tools-041035c34a090a6505169f0dbb856b7ea9ce2ffa.tar.gz |
Revert "internal/lsp/cache: reduce critical sections"
This reverts commit 654a14b5274602698564a5e9710c0778be664c7a.
Reason for revert: my flawed understanding of the concurrency
Change-Id: I31a35267323bb1ff4dff1d9244d3ce69c36cdda4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412694
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
-rw-r--r-- | internal/lsp/cache/check.go | 16 | ||||
-rw-r--r-- | internal/lsp/cache/parse.go | 2 | ||||
-rw-r--r-- | internal/lsp/debug/trace.go | 39 | ||||
-rw-r--r-- | internal/memoize/memoize.go | 6 |
4 files changed, 26 insertions, 37 deletions
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 51d7d1a7e..f09fc298a 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -97,6 +97,14 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so return nil, err } + // Do not close over the packageHandle or the snapshot in the Bind function. + // This creates a cycle, which causes the finalizers to never run on the handles. + // The possible cycles are: + // + // packageHandle.h.function -> packageHandle + // packageHandle.h.function -> snapshot -> packageHandle + // + m := ph.m key := ph.key @@ -113,13 +121,6 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so }(dep) } - // TODO(adonovan): opt: consider moving the Wait here, - // so that dependencies complete before we start to - // read+parse+typecheck this package. Although the - // read+parse can proceed, typechecking will block - // almost immediately until the imports are done. - // The effect is to increase contention. - data := &packageData{} data.pkg, data.err = typeCheck(ctx, snapshot, m.Metadata, mode, deps) // Make sure that the workers above have finished before we return, @@ -447,7 +448,6 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, m *Metadata, mode sour } typeparams.InitInstanceInfo(pkg.typesInfo) - // TODO(adonovan): opt: execute this loop in parallel. for _, gf := range pkg.m.GoFiles { // In the presence of line directives, we may need to report errors in // non-compiled Go files, so we need to register them on the package. diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index ab55743cc..668c437f5 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 (issue #53202). If there was + // file.Pos is the location of the package declaration. 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 bb402cfaa..ca612867a 100644 --- a/internal/lsp/debug/trace.go +++ b/internal/lsp/debug/trace.go @@ -119,6 +119,8 @@ 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 @@ -126,8 +128,11 @@ func (t *traces) ProcessEvent(ctx context.Context, ev core.Event, lm label.Map) switch { case event.IsStart(ev): - // Just starting: add it to the unfinished map. - // Allocate before the critical section. + 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 td := &traceData{ TraceID: span.ID.TraceID, SpanID: span.ID.SpanID, @@ -136,13 +141,6 @@ 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() { @@ -157,19 +155,7 @@ 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. - // 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() + // finishing, must be already in the map td, found := t.unfinished[span.ID] if !found { return ctx // if this happens we are in a bad place @@ -178,7 +164,14 @@ 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()) - td.Events = tdEvents + events := span.Events() + td.Events = make([]traceEvent, len(events)) + for i, event := range events { + td.Events[i] = traceEvent{ + Time: event.At(), + Tags: renderLabels(event), + } + } set, ok := t.sets[span.Name] if !ok { diff --git a/internal/memoize/memoize.go b/internal/memoize/memoize.go index 28d5e2c5b..dec2fff68 100644 --- a/internal/memoize/memoize.go +++ b/internal/memoize/memoize.go @@ -181,9 +181,8 @@ func (g *Generation) Bind(key interface{}, function Function, cleanup func(inter if atomic.LoadUint32(&g.destroyed) != 0 { panic("operation on generation " + g.name + " destroyed by " + g.destroyedBy) } - - // Avoid 'defer Unlock' to reduce critical section. g.store.mu.Lock() + defer g.store.mu.Unlock() h, ok := g.store.handles[key] if !ok { h := &Handle{ @@ -193,11 +192,8 @@ func (g *Generation) Bind(key interface{}, function Function, cleanup func(inter cleanup: cleanup, } g.store.handles[key] = h - g.store.mu.Unlock() return h } - g.store.mu.Unlock() - h.mu.Lock() defer h.mu.Unlock() if _, ok := h.generations[g]; !ok { |