go/types, types2: reduce locks held at once in resolveUnderlying

There is no need to hold locks for the entire chain of Named types in
resolveUnderlying. This change moves the locking / unlocking right to
where t.underlying is set.

This change consolidates logic into resolveUnderlying where possible
and makes minor stylistic / documentation adjustments.

Change-Id: Ic5ec5a7e9a0da8bc34954bf456e4e23a28df296d
Reviewed-on: https://go-review.googlesource.com/c/go/+/714403
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Mark Freeman <markfreeman@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/cmd/compile/internal/types2/named.go

@@ -590,7 +590,7 @@ func (n *Named) Underlying() Type {
 		}
 	}
 
-	if !n.stateHas(hasUnder) {
+	if !n.stateHas(hasUnder) { // minor performance optimization
 		n.resolveUnderlying()
 	}
 
@@ -605,7 +605,8 @@ func (t *Named) String() string { return TypeString(t, nil) }
 // TODO(rfindley): reorganize the loading and expansion methods under this
 // heading.
 
-// resolveUnderlying computes the underlying type of n.
+// resolveUnderlying computes the underlying type of n. If n already has an
+// underlying type, nothing happens.
 //
 // It does so by following RHS type chains for alias and named types. If any
 // other type T is found, each named type in the chain has its underlying
@@ -636,14 +637,13 @@ func (n *Named) resolveUnderlying() {
 					path[j] = t.obj
 				}
 				path = path[i:]
-				// Note: This code may only be called during type checking,
+				// only called during type checking, hence n.check != nil
-				//       hence n.check != nil.
 				n.check.cycleError(path, firstInSrc(path))
 				u = Typ[Invalid]
 				break
 			}
 
-			// avoid acquiring the lock if we can
+			// don't recalculate the underlying
 			if t.stateHas(hasUnder) {
 				u = t.underlying
 				break
@@ -655,9 +655,6 @@ func (n *Named) resolveUnderlying() {
 			seen[t] = len(seen)
 
 			t.unpack()
-			t.mu.Lock()
-			defer t.mu.Unlock()
-
 			assert(t.rhs() != nil || t.allowNilRHS)
 			rhs = t.rhs()
 
@@ -666,17 +663,19 @@ func (n *Named) resolveUnderlying() {
 		}
 	}
 
-	// set underlying for all Named types in the chain
 	for t := range seen {
+		func() {
+			t.mu.Lock()
+			defer t.mu.Unlock()
 			// Careful, t.underlying has lock-free readers. Since we might be racing
 			// another call to resolveUnderlying, we have to avoid overwriting
 			// t.underlying. Otherwise, the race detector will be tripped.
-		if t.stateHas(hasUnder) {
+			if !t.stateHas(hasUnder) {
-			continue
-		}
 				t.underlying = u
 				t.setState(hasUnder)
 			}
+		}()
+	}
 }
 
 func (n *Named) lookupMethod(pkg *Package, name string, foldCase bool) (int, *Func) {

src/go/types/named.go

@@ -593,7 +593,7 @@ func (n *Named) Underlying() Type {
 		}
 	}
 
-	if !n.stateHas(hasUnder) {
+	if !n.stateHas(hasUnder) { // minor performance optimization
 		n.resolveUnderlying()
 	}
 
@@ -608,7 +608,8 @@ func (t *Named) String() string { return TypeString(t, nil) }
 // TODO(rfindley): reorganize the loading and expansion methods under this
 // heading.
 
-// resolveUnderlying computes the underlying type of n.
+// resolveUnderlying computes the underlying type of n. If n already has an
+// underlying type, nothing happens.
 //
 // It does so by following RHS type chains for alias and named types. If any
 // other type T is found, each named type in the chain has its underlying
@@ -639,14 +640,13 @@ func (n *Named) resolveUnderlying() {
 					path[j] = t.obj
 				}
 				path = path[i:]
-				// Note: This code may only be called during type checking,
+				// only called during type checking, hence n.check != nil
-				//       hence n.check != nil.
 				n.check.cycleError(path, firstInSrc(path))
 				u = Typ[Invalid]
 				break
 			}
 
-			// avoid acquiring the lock if we can
+			// don't recalculate the underlying
 			if t.stateHas(hasUnder) {
 				u = t.underlying
 				break
@@ -658,9 +658,6 @@ func (n *Named) resolveUnderlying() {
 			seen[t] = len(seen)
 
 			t.unpack()
-			t.mu.Lock()
-			defer t.mu.Unlock()
-
 			assert(t.rhs() != nil || t.allowNilRHS)
 			rhs = t.rhs()
 
@@ -669,17 +666,19 @@ func (n *Named) resolveUnderlying() {
 		}
 	}
 
-	// set underlying for all Named types in the chain
 	for t := range seen {
+		func() {
+			t.mu.Lock()
+			defer t.mu.Unlock()
 			// Careful, t.underlying has lock-free readers. Since we might be racing
 			// another call to resolveUnderlying, we have to avoid overwriting
 			// t.underlying. Otherwise, the race detector will be tripped.
-		if t.stateHas(hasUnder) {
+			if !t.stateHas(hasUnder) {
-			continue
-		}
 				t.underlying = u
 				t.setState(hasUnder)
 			}
+		}()
+	}
 }
 
 func (n *Named) lookupMethod(pkg *Package, name string, foldCase bool) (int, *Func) {