diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index 9f7a19c5c6..847ac2db8d 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -147,7 +147,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A } } return runAnalysis(ctx, snapshot, a, pkg, results) - }, nil) + }) act.handle = h act = s.addActionHandle(act) diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index c076f424dc..843919d7b3 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -88,7 +88,7 @@ func (s *snapshot) ParseMod(ctx context.Context, modFH source.FileHandle) (*sour }, err: parseErr, } - }, nil) + }) pmh := &parseModHandle{handle: h} s.mu.Lock() @@ -162,7 +162,7 @@ func (s *snapshot) ParseWork(ctx context.Context, modFH source.FileHandle) (*sou }, err: parseErr, } - }, nil) + }) pwh := &parseWorkHandle{handle: h} s.mu.Lock() @@ -288,7 +288,7 @@ func (s *snapshot) ModWhy(ctx context.Context, fh source.FileHandle) (map[string why[req.Mod.Path] = whyList[i] } return &modWhyData{why: why} - }, nil) + }) mwh := &modWhyHandle{handle: h} s.mu.Lock() diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index bd2ff0c5f8..0f36249e05 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -138,7 +138,7 @@ func (s *snapshot) ModTidy(ctx context.Context, pm *source.ParsedModule) (*sourc TidiedContent: tempContents, }, } - }, nil) + }) mth := &modTidyHandle{handle: h} s.mu.Lock() diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index f7b4f9c703..c3eae2f764 100644 --- a/internal/lsp/cache/parse.go +++ b/internal/lsp/cache/parse.go @@ -130,7 +130,7 @@ func (s *snapshot) astCacheData(ctx context.Context, spkg source.Package, pos to } astHandle := s.generation.Bind(astCacheKey{pkgHandle.key, pgf.URI}, func(ctx context.Context, arg memoize.Arg) interface{} { return buildASTCache(pgf) - }, nil) + }) d, err := astHandle.Get(ctx, s.generation, s) if err != nil { diff --git a/internal/lsp/cache/symbols.go b/internal/lsp/cache/symbols.go index d56a036ff6..f27178e2e7 100644 --- a/internal/lsp/cache/symbols.go +++ b/internal/lsp/cache/symbols.go @@ -41,7 +41,7 @@ func (s *snapshot) buildSymbolHandle(ctx context.Context, fh source.FileHandle) snapshot := arg.(*snapshot) symbols, err := symbolize(snapshot, fh) return &symbolData{symbols, err} - }, nil) + }) sh := &symbolHandle{ handle: handle, diff --git a/internal/memoize/memoize.go b/internal/memoize/memoize.go index 7fa5340c5a..4b84410d50 100644 --- a/internal/memoize/memoize.go +++ b/internal/memoize/memoize.go @@ -34,6 +34,7 @@ type Store struct { handlesMu sync.Mutex // lock ordering: Store.handlesMu before Handle.mu handles map[interface{}]*Handle // handles which are bound to generations for GC purposes. + // (It is the subset of values of 'handles' with trackGenerations enabled.) boundHandles map[*Handle]struct{} } @@ -78,7 +79,11 @@ func (g *Generation) Destroy(destroyedBy string) { if _, ok := h.generations[g]; ok { delete(h.generations, g) // delete even if it's dead, in case of dangling references to the entry. if len(h.generations) == 0 { - h.destroy(g.store) + h.state = stateDestroyed + delete(g.store.handles, h.key) + if h.trackGenerations { + delete(g.store.boundHandles, h) + } } } h.mu.Unlock() @@ -155,14 +160,6 @@ type Handle struct { function Function // value is set in completed state. value interface{} - // cleanup, if non-nil, is used to perform any necessary clean-up on values - // produced by function. - // - // cleanup is never set for reference counted handles. - // - // TODO(rfindley): remove this field once workspace folders no longer need to - // be tracked. - cleanup func(interface{}) // If trackGenerations is set, this handle tracks generations in which it // is valid, via the generations field. Otherwise, it is explicitly reference @@ -171,7 +168,7 @@ type Handle struct { refCounter int32 } -// Bind returns a handle for the given key and function. +// Bind returns a "generational" handle for the given key and function. // // Each call to bind will return the same handle if it is already bound. Bind // will always return a valid handle, creating one if needed. Each key can @@ -179,22 +176,18 @@ type Handle struct { // until the associated generation is destroyed. Bind does not cause the value // to be generated. // -// If cleanup is non-nil, it will be called on any non-nil values produced by -// function when they are no longer referenced. -// // It is responsibility of the caller to call Inherit on the handler whenever // it should still be accessible by a next generation. -func (g *Generation) Bind(key interface{}, function Function, cleanup func(interface{})) *Handle { - return g.getHandle(key, function, cleanup, true) +func (g *Generation) Bind(key interface{}, function Function) *Handle { + return g.getHandle(key, function, true) } -// GetHandle returns a handle for the given key and function with similar -// properties and behavior as Bind. -// -// As in opposite to Bind it returns a release callback which has to be called -// once this reference to handle is not needed anymore. +// GetHandle returns a "reference-counted" handle for the given key +// and function with similar properties and behavior as Bind. Unlike +// Bind, it returns a release callback which must be called once the +// handle is no longer needed. func (g *Generation) GetHandle(key interface{}, function Function) (*Handle, func()) { - h := g.getHandle(key, function, nil, false) + h := g.getHandle(key, function, false) store := g.store release := func() { // Acquire store.handlesMu before mutating refCounter @@ -206,7 +199,7 @@ func (g *Generation) GetHandle(key interface{}, function Function) (*Handle, fun h.refCounter-- if h.refCounter == 0 { - // Don't call h.destroy: for reference counted handles we can't know when + // Don't mark destroyed: for reference counted handles we can't know when // they are no longer reachable from runnable goroutines. For example, // gopls could have a current operation that is using a packageHandle. // Destroying the handle here would cause that operation to hang. @@ -216,7 +209,7 @@ func (g *Generation) GetHandle(key interface{}, function Function) (*Handle, fun return h, release } -func (g *Generation) getHandle(key interface{}, function Function, cleanup func(interface{}), trackGenerations bool) *Handle { +func (g *Generation) getHandle(key interface{}, function Function, trackGenerations bool) *Handle { // panic early if the function is nil // it would panic later anyway, but in a way that was much harder to debug if function == nil { @@ -232,7 +225,6 @@ func (g *Generation) getHandle(key interface{}, function Function, cleanup func( h = &Handle{ key: key, function: function, - cleanup: cleanup, trackGenerations: trackGenerations, } if trackGenerations { @@ -298,18 +290,6 @@ func (g *Generation) Inherit(h *Handle) { h.incrementRef(g) } -// destroy marks h as destroyed. h.mu and store.handlesMu must be held. -func (h *Handle) destroy(store *Store) { - h.state = stateDestroyed - if h.cleanup != nil && h.value != nil { - h.cleanup(h.value) - } - delete(store.handles, h.key) - if h.trackGenerations { - delete(store.boundHandles, h) - } -} - func (h *Handle) incrementRef(g *Generation) { h.mu.Lock() defer h.mu.Unlock() @@ -412,11 +392,6 @@ func (h *Handle) run(ctx context.Context, g *Generation, arg Arg) (interface{}, } v := function(childCtx, arg) if childCtx.Err() != nil { - // It's possible that v was computed despite the context cancellation. In - // this case we should ensure that it is cleaned up. - if h.cleanup != nil && v != nil { - h.cleanup(v) - } return } @@ -427,19 +402,9 @@ func (h *Handle) run(ctx context.Context, g *Generation, arg Arg) (interface{}, // checked childCtx above. Even so, that should be harmless, since each // run should produce the same results. if h.state != stateRunning { - // v will never be used, so ensure that it is cleaned up. - if h.cleanup != nil && v != nil { - h.cleanup(v) - } return } - if h.cleanup != nil && h.value != nil { - // Clean up before overwriting an existing value. - h.cleanup(h.value) - } - - // At this point v will be cleaned up whenever h is destroyed. h.value = v h.function = nil h.state = stateCompleted diff --git a/internal/memoize/memoize_test.go b/internal/memoize/memoize_test.go index ae387b8d04..48bb181173 100644 --- a/internal/memoize/memoize_test.go +++ b/internal/memoize/memoize_test.go @@ -23,7 +23,7 @@ func TestGet(t *testing.T) { h := g.Bind("key", func(context.Context, memoize.Arg) interface{} { evaled++ return "res" - }, nil) + }) expectGet(t, h, g, "res") expectGet(t, h, g, "res") if evaled != 1 { @@ -50,7 +50,7 @@ func TestGenerations(t *testing.T) { s := &memoize.Store{} // Evaluate key in g1. g1 := s.Generation("g1") - h1 := g1.Bind("key", func(context.Context, memoize.Arg) interface{} { return "res" }, nil) + h1 := g1.Bind("key", func(context.Context, memoize.Arg) interface{} { return "res" }) expectGet(t, h1, g1, "res") // Get key in g2. It should inherit the value from g1. @@ -58,7 +58,7 @@ func TestGenerations(t *testing.T) { h2 := g2.Bind("key", func(context.Context, memoize.Arg) interface{} { t.Fatal("h2 should not need evaluation") return "error" - }, nil) + }) expectGet(t, h2, g2, "res") // With g1 destroyed, g2 should still work. @@ -68,47 +68,10 @@ func TestGenerations(t *testing.T) { // With all generations destroyed, key should be re-evaluated. g2.Destroy("TestGenerations") g3 := s.Generation("g3") - h3 := g3.Bind("key", func(context.Context, memoize.Arg) interface{} { return "new res" }, nil) + h3 := g3.Bind("key", func(context.Context, memoize.Arg) interface{} { return "new res" }) expectGet(t, h3, g3, "new res") } -func TestCleanup(t *testing.T) { - s := &memoize.Store{} - g1 := s.Generation("g1") - v1 := false - v2 := false - cleanup := func(v interface{}) { - *(v.(*bool)) = true - } - h1 := g1.Bind("key1", func(context.Context, memoize.Arg) interface{} { - return &v1 - }, nil) - h2 := g1.Bind("key2", func(context.Context, memoize.Arg) interface{} { - return &v2 - }, cleanup) - expectGet(t, h1, g1, &v1) - expectGet(t, h2, g1, &v2) - g2 := s.Generation("g2") - g2.Inherit(h1) - g2.Inherit(h2) - - g1.Destroy("TestCleanup") - expectGet(t, h1, g2, &v1) - expectGet(t, h2, g2, &v2) - for k, v := range map[string]*bool{"key1": &v1, "key2": &v2} { - if got, want := *v, false; got != want { - t.Errorf("after destroying g1, bound value %q is cleaned up", k) - } - } - g2.Destroy("TestCleanup") - if got, want := v1, false; got != want { - t.Error("after destroying g2, v1 is cleaned up") - } - if got, want := v2, true; got != want { - t.Error("after destroying g2, v2 is not cleaned up") - } -} - func TestHandleRefCounting(t *testing.T) { s := &memoize.Store{} g1 := s.Generation("g1")