cr

Change-Id: If949bf9aac8f60f09d7969feaf6e9f67030400af

internal/lsp/cache/parse.go

@@ -58,7 +58,7 @@ func (s *snapshot) parseGoHandle(ctx context.Context, fh source.FileHandle, mode
 	if pgh := s.getGoFile(key); pgh != nil {
 		return pgh
 	}
-	parseHandle, release := s.generation.NewHandle(key, func(ctx context.Context, arg memoize.Arg) interface{} {
+	parseHandle, release := s.generation.GetHandle(key, func(ctx context.Context, arg memoize.Arg) interface{} {
 		snapshot := arg.(*snapshot)
 		return parseGo(ctx, snapshot.FileSet(), fh, mode)
 	}, nil)

internal/memoize/memoize.go

@@ -84,9 +84,17 @@ func (g *Generation) Destroy(destroyedBy string) {
 	g.store.mu.Lock()
 	defer g.store.mu.Unlock()
 	for _, e := range g.store.handles {
-		if e.trackGenerations {
-			e.decrementRef(g, g.store)
+		if !e.trackGenerations {
+			continue
 		}
+		e.mu.Lock()
+		if _, ok := e.generations[g]; ok {
+			delete(e.generations, g) // delete even if it's dead, in case of dangling references to the entry.
+			if len(e.generations) == 0 {
+				e.destroy(g.store)
+			}
+		}
+		e.mu.Unlock()
 	}
 	delete(g.store.generations, g)
 }
@@ -153,6 +161,9 @@ type Handle struct {
 	// produced by function.
 	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
+	// counted via the refCounter field.
 	trackGenerations bool
 	refCounter       int32
 }
@@ -171,27 +182,30 @@ type Handle struct {
 // 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.newHandle(key, function, cleanup, true)
+	return g.getHandle(key, function, cleanup, true)
 }
 
-// NewHandle returns a handle for the given key and function with similar
+// 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.
-func (g *Generation) NewHandle(key interface{}, function Function, cleanup func(interface{})) (*Handle, func()) {
-	handle := g.newHandle(key, function, cleanup, false)
+func (g *Generation) GetHandle(key interface{}, function Function, cleanup func(interface{})) (*Handle, func()) {
+	handle := g.getHandle(key, function, cleanup, false)
 	store := g.store
 	release := func() {
 		store.mu.Lock()
 		defer store.mu.Unlock()
 
-		handle.decrementRef(nil, store)
+		handle.refCounter--
+		if handle.refCounter == 0 {
+			handle.destroy(store)
+		}
 	}
 	return handle, release
 }
 
-func (g *Generation) newHandle(key interface{}, function Function, cleanup func(interface{}), trackGenerations bool) *Handle {
+func (g *Generation) getHandle(key interface{}, function Function, cleanup func(interface{}), 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 {
@@ -287,31 +301,7 @@ func (h *Handle) incrementRef(g *Generation) {
 	}
 }
 
-func (h *Handle) decrementRef(g *Generation, store *Store) {
-	h.mu.Lock()
-	defer h.mu.Unlock()
-
-	if h.trackGenerations {
-		if g == nil {
-			panic("passed nil generation to Handle.decrementRef")
-		}
-		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(store)
-			}
-		}
-	} else {
-		if g != nil {
-			panic(fmt.Sprintf("passed non-generation to Handle.decrementRef: %v", g))
-		}
-		h.refCounter--
-		if h.refCounter == 0 {
-			h.destroy(store)
-		}
-	}
-}
-
+// hasRefLocked reports whether h is valid in generation g. h.mu must be held.
 func (h *Handle) hasRefLocked(g *Generation) bool {
 	if !h.trackGenerations {
 		return true

internal/memoize/memoize_test.go

@@ -115,10 +115,10 @@ func TestHandleRefCounting(t *testing.T) {
 	cleanup := func(v interface{}) {
 		*(v.(*bool)) = true
 	}
-	h1, release1 := g1.NewHandle("key1", func(context.Context, memoize.Arg) interface{} {
+	h1, release1 := g1.GetHandle("key1", func(context.Context, memoize.Arg) interface{} {
 		return &v1
 	}, nil)
-	h2, release2 := g1.NewHandle("key2", func(context.Context, memoize.Arg) interface{} {
+	h2, release2 := g1.GetHandle("key2", func(context.Context, memoize.Arg) interface{} {
 		return &v2
 	}, cleanup)
 	expectGet(t, h1, g1, &v1)
@@ -129,7 +129,7 @@ func TestHandleRefCounting(t *testing.T) {
 	g1.Destroy("by test")
 	expectGet(t, h2, g2, &v2)
 
-	h2Copy, release2Copy := g2.NewHandle("key2", func(context.Context, memoize.Arg) interface{} {
+	h2Copy, release2Copy := g2.GetHandle("key2", func(context.Context, memoize.Arg) interface{} {
 		return &v1
 	}, nil)
 	if h2 != h2Copy {
@@ -152,7 +152,7 @@ func TestHandleRefCounting(t *testing.T) {
 	release1()
 
 	g3 := s.Generation("g3")
-	h2Copy, release2Copy = g3.NewHandle("key2", func(context.Context, memoize.Arg) interface{} {
+	h2Copy, release2Copy = g3.GetHandle("key2", func(context.Context, memoize.Arg) interface{} {
 		return &v2
 	}, cleanup)
 	if h2 == h2Copy {

internal/persistent/map.go

@@ -12,6 +12,17 @@ import (
 	"sync/atomic"
 )
 
+// Implementation details:
+// * Each value is reference counted by nodes which hold it.
+// * Each node is reference counted by its parent nodes.
+// * Each map is considered a top-level parent node from reference counting perspective.
+// * Each change does always effectivelly produce a new top level node.
+//
+// Functions which operate directly with nodes do have a notation in form of
+// `foo(arg1:+n1, arg2:+n2) (ret1:+n3)`.
+// Each argument is followed by a delta change to its reference counter.
+// In case if no change is expected, the delta will be `-0`.
+
 // Map is an associative mapping from keys to values, both represented as
 // interface{}. Key comparison and iteration order is defined by a
 // client-provided function that implements a strict weak order.
@@ -159,9 +170,9 @@ func (pm *Map) Store(key, value interface{}, release func(key, value interface{}
 // union returns a new tree which is a union of first and second one.
 // If overwrite is set to true, second one would override a value for any duplicate keys.
 //
-// union(left:-0, right:-0) (result:+1)
+// union(first:-0, second:-0) (result:+1)
 // Union borrows both subtrees without affecting their refcount and returns a
-// new reference that the caller is expected to dispose of.
+// new reference that the caller is expected to call decref.
 func union(first, second *mapNode, less func(a, b interface{}) bool, overwrite bool) *mapNode {
 	if first == nil {
 		return second.incref()
@@ -197,7 +208,7 @@ func union(first, second *mapNode, less func(a, b interface{}) bool, overwrite b
 //
 // split(n:-0) (left:+1, mid:+1, right:+1)
 // Split borrows n without affecting its refcount, and returns three
-// new references that that caller is expected to dispose of.
+// new references that that caller is expected to call decref.
 func split(n *mapNode, key interface{}, less func(a, b interface{}) bool) (left, mid, right *mapNode) {
 	if n == nil {
 		return nil, nil, nil
@@ -236,7 +247,7 @@ func (pm *Map) Delete(key interface{}) {
 //
 // merge(left:-0, right:-0) (result:+1)
 // Merge borrows its arguments without affecting their refcount
-// and returns a new reference that the caller is expected to dispose of.
+// and returns a new reference that the caller is expected to call decref.
 func merge(left, right *mapNode) *mapNode {
 	switch {
 	case left == nil: