From b8aa1ee442921b9517b53bf079ff5b4c2c89a54d Mon Sep 17 00:00:00 2001 From: Mark Freeman Date: Thu, 23 Oct 2025 16:48:00 -0400 Subject: [PATCH] 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 Reviewed-by: Robert Griesemer Auto-Submit: Mark Freeman LUCI-TryBot-Result: Go LUCI --- src/cmd/compile/internal/types2/named.go | 33 ++++++++++++------------ src/go/types/named.go | 33 ++++++++++++------------ 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/cmd/compile/internal/types2/named.go b/src/cmd/compile/internal/types2/named.go index 06f4e75c82d..cece304925a 100644 --- 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 130919435a1..1f9e2470fa7 100644 --- 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) + } + }() } }