internal/memoize: delete Bind(cleanup) hook
Now that the workspace directory uses Snapshot.Destroy
to clean up (see https://go-review.googlesource.com/c/tools/+/414499)
there is no need for this feature.
Change-Id: Id5782273ce5030b4fb8f3b66a8d16a45a831ed91
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414500
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go
index 9f7a19c..847ac2d 100644
--- a/internal/lsp/cache/analysis.go
+++ b/internal/lsp/cache/analysis.go
@@ -147,7 +147,7 @@
}
}
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 c076f42..843919d 100644
--- a/internal/lsp/cache/mod.go
+++ b/internal/lsp/cache/mod.go
@@ -88,7 +88,7 @@
},
err: parseErr,
}
- }, nil)
+ })
pmh := &parseModHandle{handle: h}
s.mu.Lock()
@@ -162,7 +162,7 @@
},
err: parseErr,
}
- }, nil)
+ })
pwh := &parseWorkHandle{handle: h}
s.mu.Lock()
@@ -288,7 +288,7 @@
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 bd2ff0c..0f36249 100644
--- a/internal/lsp/cache/mod_tidy.go
+++ b/internal/lsp/cache/mod_tidy.go
@@ -138,7 +138,7 @@
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 f7b4f9c..c3eae2f 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -130,7 +130,7 @@
}
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 d56a036..f27178e 100644
--- a/internal/lsp/cache/symbols.go
+++ b/internal/lsp/cache/symbols.go
@@ -41,7 +41,7 @@
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 7fa5340..4b84410 100644
--- a/internal/memoize/memoize.go
+++ b/internal/memoize/memoize.go
@@ -34,6 +34,7 @@
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 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
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 @@
h = &Handle{
key: key,
function: function,
- cleanup: cleanup,
trackGenerations: trackGenerations,
}
if trackGenerations {
@@ -298,18 +290,6 @@
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 @@
}
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 @@
// 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 ae387b8..48bb181 100644
--- a/internal/memoize/memoize_test.go
+++ b/internal/memoize/memoize_test.go
@@ -23,7 +23,7 @@
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 @@
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 @@
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 @@
// 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")
