diff --git a/changes/unreleased/implicit-parameter-position-from-memo.fixed.md b/changes/unreleased/implicit-parameter-position-from-memo.fixed.md new file mode 100644 index 0000000000..58862a2ee5 --- /dev/null +++ b/changes/unreleased/implicit-parameter-position-from-memo.fixed.md @@ -0,0 +1 @@ +- The features a feature redefines, directly or through them, are computed once per model, and a parameter's implicit redefinitions take its position from its owner's memoized parameter list instead of rebuilding it on every query; same redefinitions, fewer allocations per query — and per execution root frame, which asks for every feature it holds. diff --git a/internal/semantic/semantics/implicit_parameter_memo_test.go b/internal/semantic/semantics/implicit_parameter_memo_test.go new file mode 100644 index 0000000000..13ed5a4f51 --- /dev/null +++ b/internal/semantic/semantics/implicit_parameter_memo_test.go @@ -0,0 +1,28 @@ +package semantics + +import "testing" + +// The redefinition closure of a feature is computed once per model, and a +// parameter's implicit redefinitions take its position from the owner's +// memoized parameter list instead of rebuilding it: repeated queries allocate +// nothing beyond what the owner's implied generals cost. +func TestRedefinedFeaturesAreComputedOncePerSymbol(t *testing.T) { + m, root := buildModel(t, `package P { + action def Base { in x; in y; out z; } + action def Proc :> Base { in a; in b; out c; } + }`) + p := sym(t, root, "P") + proc := nested(t, p.Scope, "Proc") + a := nested(t, p.Scope, "Proc", "a") + wantX := nested(t, p.Scope, "Base", "x") + if got := m.AllRedefinedFeatures(a); len(got) != 1 || got[0] != wantX { + t.Fatalf("AllRedefinedFeatures(Proc::a) = %v, want [Base::x]", got) + } + if got := testing.AllocsPerRun(20, func() { m.AllRedefinedFeatures(a) }); got != 0 { + t.Fatalf("a repeated AllRedefinedFeatures query allocates %.0f times, want 0", got) + } + generals := testing.AllocsPerRun(20, func() { m.ImplicitGenerals(proc) }) + if got := testing.AllocsPerRun(20, func() { m.ImplicitParameterRedefinitions(a) }); got > generals+1 { + t.Fatalf("a repeated ImplicitParameterRedefinitions query allocates %.0f times, want at most its result beyond the owner's implied generals (%.0f)", got, generals) + } +} diff --git a/internal/semantic/semantics/masking.go b/internal/semantic/semantics/masking.go index d02c4dc117..45c223f47c 100644 --- a/internal/semantic/semantics/masking.go +++ b/internal/semantic/semantics/masking.go @@ -74,6 +74,11 @@ func (m *Model) AllRedefinedFeatures(sym *symbols.Symbol) []*symbols.Symbol { if m == nil || sym == nil { return nil } + defer m.own(sym).LeaveDoc() + if cached, ok := m.allRedefined[sym]; ok { + return cached + } + m.resolver.Enter() var out []*symbols.Symbol seen := map[*symbols.Symbol]bool{sym: true} var visit func(*symbols.Symbol) @@ -88,6 +93,13 @@ func (m *Model) AllRedefinedFeatures(sym *symbols.Symbol) []*symbols.Symbol { } } visit(sym) + // A closure walked while a guard cut a query short may lack a target, so it + // is recomputed on the next query, not memoized. + if !m.resolver.Leave() { + return out + } + journal(m, m.allRedefined, sym, sym.Decl) + m.allRedefined[sym] = out return out } diff --git a/internal/semantic/semantics/model.go b/internal/semantic/semantics/model.go index bfdf8e7558..37c8cb049a 100644 --- a/internal/semantic/semantics/model.go +++ b/internal/semantic/semantics/model.go @@ -34,20 +34,22 @@ type Model struct { computingSupers map[*symbols.Symbol]int // valuing holds the features whose typing value is being judged, so a value // that leads back to its own feature is not followed again. - valuing map[*symbols.Symbol]bool - referenced map[*symbols.Symbol]*symbols.Symbol - resolvingRef map[*symbols.Symbol]bool - memberSources map[*symbols.Symbol][]*symbols.Symbol - lookupOrder map[*symbols.Symbol][]lookupSource // name-lookup order - contributed map[*symbols.Symbol][]*symbols.Symbol // memoized contributors - nestedRedefs map[*symbols.Symbol][]NestedRedefinition - primTypes map[*symbols.Symbol]PrimType - scalars map[*symbols.Symbol]PrimType // stdlib scalar symbols, resolved once - params map[*symbols.Symbol]behaviorParameters - invocations map[invocationKey]*InvocationSelection - arguments ArgumentTyper // the checker's argument typing, nil when no checker runs - sourceText source.Lookup // notation of the loaded documents, nil when unavailable - sourceFile source.Locate // the files the documents were read from, nil for source.FileNamed + valuing map[*symbols.Symbol]bool + referenced map[*symbols.Symbol]*symbols.Symbol + resolvingRef map[*symbols.Symbol]bool + memberSources map[*symbols.Symbol][]*symbols.Symbol + lookupOrder map[*symbols.Symbol][]lookupSource // name-lookup order + contributed map[*symbols.Symbol][]*symbols.Symbol // memoized contributors + nestedRedefs map[*symbols.Symbol][]NestedRedefinition + allRedefined map[*symbols.Symbol][]*symbols.Symbol // memoized redefinition closure + primTypes map[*symbols.Symbol]PrimType + scalars map[*symbols.Symbol]PrimType // stdlib scalar symbols, resolved once + params map[*symbols.Symbol]behaviorParameters + computingParams map[*symbols.Symbol]int // lookup depth of a params entry being computed + invocations map[invocationKey]*InvocationSelection + arguments ArgumentTyper // the checker's argument typing, nil when no checker runs + sourceText source.Lookup // notation of the loaded documents, nil when unavailable + sourceFile source.Locate // the files the documents were read from, nil for source.FileNamed // typingArgs holds the calls whose arguments are being typed, so an argument // whose type leads back to its own call is not typed again. typingArgs map[*ast.InvocationExpr]bool @@ -168,8 +170,10 @@ func NewModel(resolver *resolve.Resolver) *Model { lookupOrder: make(map[*symbols.Symbol][]lookupSource), contributed: make(map[*symbols.Symbol][]*symbols.Symbol), nestedRedefs: make(map[*symbols.Symbol][]NestedRedefinition), + allRedefined: make(map[*symbols.Symbol][]*symbols.Symbol), primTypes: make(map[*symbols.Symbol]PrimType), params: make(map[*symbols.Symbol]behaviorParameters), + computingParams: make(map[*symbols.Symbol]int), invocations: make(map[invocationKey]*InvocationSelection), typingArgs: make(map[*ast.InvocationExpr]bool), composed: make(map[composedKey][]*symbols.Symbol), diff --git a/internal/semantic/semantics/parameters_guard_test.go b/internal/semantic/semantics/parameters_guard_test.go new file mode 100644 index 0000000000..9271fd59d8 --- /dev/null +++ b/internal/semantic/semantics/parameters_guard_test.go @@ -0,0 +1,28 @@ +package semantics + +import "testing" + +// A parameter list read while its owner's list is still being computed cuts +// the reading query short, so a closure built on it is not memoized either. +func TestParameterListGuardCutsDependentQueriesShort(t *testing.T) { + m, root := buildModel(t, `package P { + action def Base { in x; out z; } + action def Proc :> Base { in a; out c; } + }`) + p := sym(t, root, "P") + proc := nested(t, p.Scope, "Proc") + a := nested(t, p.Scope, "Proc", "a") + wantX := nested(t, p.Scope, "Base", "x") + // As parametersOf stands mid-computation for the owner. + m.params[proc] = behaviorParameters{} + m.computingParams[proc] = m.resolver.Enter() + if got := m.AllRedefinedFeatures(a); len(got) != 0 { + t.Fatalf("AllRedefinedFeatures(Proc::a) under the guard = %v, want the guard's empty answer", got) + } + m.resolver.Leave() + delete(m.params, proc) + delete(m.computingParams, proc) + if got := m.AllRedefinedFeatures(a); len(got) != 1 || got[0] != wantX { + t.Fatalf("AllRedefinedFeatures(Proc::a) after the guard = %v, want [Base::x]", got) + } +} diff --git a/internal/semantic/semantics/redefinition.go b/internal/semantic/semantics/redefinition.go index f5ab8143dc..e76c0cb421 100644 --- a/internal/semantic/semantics/redefinition.go +++ b/internal/semantic/semantics/redefinition.go @@ -159,11 +159,17 @@ func (m *Model) BehaviorParametersOf(sym *symbols.Symbol) []BehaviorParameter { func (m *Model) parametersOf(sym *symbols.Symbol) behaviorParameters { defer m.own(sym).LeaveDoc() if cached, ok := m.params[sym]; ok { + // The seed answers a re-entrant query with nothing, cutting that query short. + if depth := m.computingParams[sym]; depth != 0 { + m.resolver.CutShort(depth) + } return cached } // Guard against re-entrancy on cyclic specialization graphs. journal(m, m.params, sym, sym.Decl) m.params[sym] = behaviorParameters{} + m.computingParams[sym] = m.resolver.Enter() + defer delete(m.computingParams, sym) owned := ownedParameters(sym) out := behaviorParameters{positional: positionalParameters(owned), result: resultParameter(owned)} @@ -194,6 +200,12 @@ func (m *Model) parametersOf(sym *symbols.Symbol) behaviorParameters { } } + // A list derived while a guard cut a query short may have missed a + // parameter, so it is recomputed on the next query, not memoized. + if !m.resolver.Leave() { + delete(m.params, sym) + return out + } m.params[sym] = out return out } @@ -391,7 +403,8 @@ func (m *Model) implicitParameterTargets(sym *symbols.Symbol, matchDirection boo return nil } position := -1 - for i, p := range positionalParameters(ownedParameters(owner)) { + // The owner's memoized parameters list its owned ones first, in order. + for i, p := range m.parametersOf(owner).positional { if p.sym == sym { position = i break diff --git a/internal/semantic/semantics/redefinition_closure_guard_test.go b/internal/semantic/semantics/redefinition_closure_guard_test.go new file mode 100644 index 0000000000..4cc09d6479 --- /dev/null +++ b/internal/semantic/semantics/redefinition_closure_guard_test.go @@ -0,0 +1,28 @@ +package semantics + +import "testing" + +// A redefinition closure walked while a guard cuts a query short is answered +// but not memoized: once the guard lifts, the next query sees the target. +func TestRedefinitionClosureIsNotCachedWhileAGuardCutsItShort(t *testing.T) { + m, root := buildModel(t, `package P { + part def W { attribute x; } + part def A { part w : W; } + part def B :> A { attribute z :>> w.x; } + }`) + p := sym(t, root, "P") + wantX := nested(t, p.Scope, "W", "x") + bz := nested(t, p.Scope, "B", "z") + // As RedefinedFeatures stands mid-computation: seeded, with its lookup open. + m.redefined[bz] = nil + m.computingRedefined[bz] = m.resolver.Enter() + if got := m.AllRedefinedFeatures(bz); len(got) != 0 { + t.Fatalf("AllRedefinedFeatures(B::z) under the guard = %v, want the guard's empty answer", got) + } + m.resolver.Leave() + delete(m.redefined, bz) + delete(m.computingRedefined, bz) + if got := m.AllRedefinedFeatures(bz); len(got) != 1 || got[0] != wantX { + t.Fatalf("AllRedefinedFeatures(B::z) after the guard = %v, want [W::x]", got) + } +}