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>

diff --git a/src/cmd/compile/internal/types2/named.go b/src/cmd/compile/internal/types2/named.go
index 06f4e75c82d38e9a3eb989493e940e4d85404d05..cece304925a2917b6ab5482e4a02f7f815cd61be 100644 (file)
--- a/src/cmd/compile/internal/types2/named.go
+++ b/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,
-                               //       hence n.check != nil.
+                               // only called during type checking, 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,16 +663,18 @@ func (n *Named) resolveUnderlying() {
                }
        }
 
-       // set underlying for all Named types in the chain
        for t := range seen {
-               // 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) {
-                       continue
-               }
-               t.underlying = u
-               t.setState(hasUnder)
+               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) {
+                               t.underlying = u
+                               t.setState(hasUnder)
+                       }
+               }()
        }
 }
 

diff --git a/src/go/types/named.go b/src/go/types/named.go
index 130919435a171d2df8609a2a3fd2b2408c5bcfac..1f9e2470fa7411e21c1a749e6b9cf38d4a4795af 100644 (file)
--- a/src/go/types/named.go
+++ b/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,
-                               //       hence n.check != nil.
+                               // only called during type checking, 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,16 +666,18 @@ func (n *Named) resolveUnderlying() {
                }
        }
 
-       // set underlying for all Named types in the chain
        for t := range seen {
-               // 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) {
-                       continue
-               }
-               t.underlying = u
-               t.setState(hasUnder)
+               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) {
+                               t.underlying = u
+                               t.setState(hasUnder)
+                       }
+               }()
        }
 }