[tools] gopls/internal/cache: store analyzeSummary fields on analysisNode

[Robert Findley (Gerrit)]

Robert Findley has uploaded the change for review

Commit message

gopls/internal/cache: store analyzeSummary fields on analysisNode

Replace analysisNode.summary with separate compiles and actions fields,
copied from the *analyzeSummary returned by runCached. The summary
itself is no longer retained, so it can be safely shared via filecache
or inFlightAnalyses without a defensive copy, and decrefPreds now nils
the node-local actions field rather than mutating a shared struct.

This lets runCached use filecache.Get with analyzeSummaryCodec.Decode
directly, caching the decoded summary in memCache like the other kinds.

Change-Id: I1c890053fef709ebe2f7da7543a0e7280b4d0b8a

Change diff

diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go
index 5402163..3215bff 100644
--- a/gopls/internal/cache/analysis.go
+++ b/gopls/internal/cache/analysis.go
@@ -343,7 +343,11 @@
 			}
 
 			maybeReport(completed.Add(1))
-			an.summary = summary
+			// Copy fields onto the node. The summary itself (which may
+			// be shared via filecache or inFlightAnalyses) is not
+			// retained, so it is never mutated.
+			an.compiles = summary.Compiles
+			an.actions = summary.Actions
 
 			// Notify each waiting predecessor,
 			// and enqueue it when it becomes a leaf.
@@ -399,7 +403,7 @@
 				// cause #60909 since none of the analyzers currently added for
 				// requirements (e.g. ctrlflow, inspect, buildssa)
 				// is capable of reporting diagnostics.
-				if summary := root.summary.Actions[stableNames[a]]; summary != nil {
+				if summary := root.actions[stableNames[a]]; summary != nil {
 					if n := len(summary.Diagnostics); n > 0 {
 						bug.Reportf("Internal error: got %d unexpected diagnostics from analyzer %s. This analyzer was added only to fulfil the requirements of the requested set of analyzers, and it is not expected that such analyzers report diagnostics. Please report this in issue #60909.", n, a)
 					}
@@ -407,11 +411,11 @@
 				continue
 			}
 
-			// Inv: root.summary is the successful result of run (via runCached).
-			summary, ok := root.summary.Actions[stableNames[a]]
+			// Inv: root.actions is the successful result of run (via runCached).
+			summary, ok := root.actions[stableNames[a]]
 			if summary == nil {
 				panic(fmt.Sprintf("analyzeSummary.Actions[%q] = (nil, %t); got %v (#60551)",
-					stableNames[a], ok, root.summary.Actions))
+					stableNames[a], ok, root.actions))
 			}
 			if summary.Err != "" {
 				continue // action failed
@@ -426,7 +430,7 @@
 
 func (an *analysisNode) decrefPreds() {
 	if an.unfinishedPreds.Add(-1) == 0 {
-		an.summary.Actions = nil
+		an.actions = nil
 	}
 }
 
@@ -453,8 +457,9 @@
 	preds           []*analysisNode             // graph edges:
 	succs           map[PackageID]*analysisNode //   (preds -> self -> succs)
 	unfinishedSuccs atomic.Int32
-	unfinishedPreds atomic.Int32                  // effectively a summary.Actions refcount
-	summary         *analyzeSummary               // serializable result of analyzing this package
+	unfinishedPreds atomic.Int32                  // effectively an actions refcount
+	compiles        bool                          // copied from analyzeSummary
+	actions         actionMap                     // copied from analyzeSummary; nilled by decrefPreds
 	stableNames     map[*analysis.Analyzer]string // cross-process stable names for Analyzers
 
 	summaryHashOnce sync.Once
@@ -472,10 +477,10 @@
 	an.summaryHashOnce.Do(func() {
 		hasher := sha256.New()
 		fmt.Fprintf(hasher, "dep: %s\n", an.ph.mp.PkgPath)
-		fmt.Fprintf(hasher, "compiles: %t\n", an.summary.Compiles)
+		fmt.Fprintf(hasher, "compiles: %t\n", an.compiles)
 
 		// action results: errors and facts
-		for name, summary := range moremaps.Sorted(an.summary.Actions) {
+		for name, summary := range moremaps.Sorted(an.actions) {
 			fmt.Fprintf(hasher, "action %s\n", name)
 			if summary.Err != "" {
 				fmt.Fprintf(hasher, "error %s\n", summary.Err)
@@ -574,46 +579,36 @@
 	// We now consult a global cache of promised results. If nothing material has
 	// changed, we'll make a hit in the shared cache.
 
-	// Access the cache.
-	var summary *analyzeSummary
+	// Access the cache. The returned summary is shared (via memCache or
+	// inFlightAnalyses) and must be treated as immutable; the caller
+	// copies its fields onto the analysisNode rather than retaining it.
 	const cacheKind = "analysis"
-	if data, err := filecache.Get(cacheKind, key, filecache.Bytes); err == nil {
-		// cache hit
-		summary = analyzeSummaryCodec.Decode(data)
+	if summary, err := filecache.Get(cacheKind, key, analyzeSummaryCodec.Decode); err == nil {
+		return summary, nil // cache hit
 	} else if err != filecache.ErrNotFound {
 		return nil, bug.Errorf("internal error reading shared cache: %v", err)
-	} else {
-		// Cache miss: do the work.
-		cachedSummary, err := inFlightAnalyses.get(ctx, key, func(ctx context.Context) (*analyzeSummary, error) {
-			summary, err := an.run(ctx)
-			if err != nil {
-				return nil, err
-			}
-			go func() {
-				cacheLimit <- unit{}            // acquire token
-				defer func() { <-cacheLimit }() // release token
+	}
 
-				data := analyzeSummaryCodec.Encode(summary)
-				if false {
-					log.Printf("Set key=%d value=%d id=%s\n", len(key), len(data), an.ph.mp.ID)
-				}
-				if err := filecache.Set(cacheKind, key, data); err != nil {
-					event.Error(ctx, "internal error updating analysis shared cache", err)
-				}
-			}()
-			return summary, nil
-		})
+	// Cache miss: do the work.
+	return inFlightAnalyses.get(ctx, key, func(ctx context.Context) (*analyzeSummary, error) {
+		summary, err := an.run(ctx)
 		if err != nil {
 			return nil, err
 		}
+		go func() {
+			cacheLimit <- unit{}            // acquire token
+			defer func() { <-cacheLimit }() // release token
 
-		// Copy the computed summary. In decrefPreds, we may zero out
-		// summary.actions, but can't mutate a shared result.
-		copy := *cachedSummary
-		summary = &copy
-	}
-
-	return summary, nil
+			data := analyzeSummaryCodec.Encode(summary)
+			if false {
+				log.Printf("Set key=%d value=%d id=%s\n", len(key), len(data), an.ph.mp.ID)
+			}
+			if err := filecache.Set(cacheKind, key, data); err != nil {
+				event.Error(ctx, "internal error updating analysis shared cache", err)
+			}
+		}()
+		return summary, nil
+	})
 }
 
 // cacheKey returns a cache key that is a cryptographic digest
@@ -770,7 +765,7 @@
 	}
 
 	for _, vdep := range an.succs {
-		if !vdep.summary.Compiles {
+		if !vdep.compiles {
 			compiles = false // transitive error
 		}
 	}
@@ -917,7 +912,7 @@
 	if hasFacts {
 		// TODO(adonovan): use deterministic order.
 		for _, vdep := range act.vdeps {
-			if summ := vdep.summary.Actions[act.stableName]; summ.Err != "" {
+			if summ := vdep.actions[act.stableName]; summ.Err != "" {
 				return nil, nil, errors.New(summ.Err)
 			}
 		}
@@ -991,7 +986,7 @@
 			return nil, bug.Errorf("internal error in %s: missing vdep for id=%s", apkg.pkg.Types().Path(), id)
 		}
 
-		return vdep.summary.Actions[act.stableName].Facts, nil
+		return vdep.actions[act.stableName].Facts, nil
 	})
 	if err != nil {
 		return nil, nil, fmt.Errorf("internal error decoding analysis facts: %w", err)

Change information

Files:

• M gopls/internal/cache/analysis.go

Change size: M

Delta: 1 file changed, 40 insertions(+), 45 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newchange

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I1c890053fef709ebe2f7da7543a0e7280b4d0b8a

Gerrit-Change-Number: 764320

Gerrit-PatchSet: 1

Gerrit-Owner: Robert Findley <rfin...@golang.org>