From 29bd0226364516a2ab438173178f808b97f85897 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 7 Nov 2026 10:08:20 +0000 Subject: [PATCH 1/5] Cache config building --- src/parse/asp/config.go | 54 ++++++++++++++++++++++++++++-------- src/parse/asp/interpreter.go | 16 +++++++---- 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/src/parse/asp/config.go b/src/parse/asp/config.go index 5d76b79d4a..2415c99b4d 100644 --- a/src/parse/asp/config.go +++ b/src/parse/asp/config.go @@ -11,6 +11,13 @@ import ( "github.com/thought-machine/please/src/core" ) +// Used to key a cache to memoise building plugin configs. +type pluginConfigKey struct { + PluginState *core.BuildState // State for the plugin itself + State *core.BuildState // State for the receiving package (or its parent if it has no overrides) + InheritOnly bool // True if State is the parent of a state with no overrides for this plugin +} + // valueToPyObject converts a field value to a pyObject func valueToPyObject(value reflect.Value) pyObject { switch value.Kind() { @@ -135,8 +142,7 @@ func getConfigKey(aspKey, configKey string) string { } // pluginConfig loads the plugin's config into a pyDict. It will load con -func pluginConfig(pluginState *core.BuildState, pkgState *core.BuildState) pyDict { - pluginName := strings.ToLower(pluginState.RepoConfig.PluginDefinition.Name) +func pluginConfig(pluginState *core.BuildState, pkgState *core.BuildState, pluginName string) pyDict { var extraVals map[string][]string var ret pyDict if pkgState.ParentState == nil { @@ -144,7 +150,7 @@ func pluginConfig(pluginState *core.BuildState, pkgState *core.BuildState) pyDic ret = pyDict{} } else { extraVals = getExtraVals(pkgState.RepoConfig, pluginName) - ret = pluginConfig(pluginState, pkgState.ParentState) + ret = pluginConfig(pluginState, pkgState.ParentState, pluginName) } for key, definition := range pluginState.RepoConfig.PluginConfig { @@ -193,6 +199,35 @@ func pluginConfig(pluginState *core.BuildState, pkgState *core.BuildState) pyDic return ret } +// loadCachedPluginConfig loads the config for a particular plugin, caching it to minimise rebuilds. +func (i *interpreter) loadCachedPluginConfig(pluginState *core.BuildState, pkgState *core.BuildState, name string) pyDict { + key := pluginConfigKey{ + PluginState: pluginState, + State: pkgState, + } + if len(getExtraVals(pkgState.RepoConfig, name)) == 0 { + // This package doesn't have any overrides, so we can reuse the parent state's config. + key.State = pkgState.ParentState + key.InheritOnly = true + } + i.pluginConfigLock.RLock() + if cfg, present := i.pluginConfigs[key]; present { + i.pluginConfigLock.RUnlock() + return cfg + } + i.pluginConfigLock.RUnlock() + i.pluginConfigLock.Lock() + defer i.pluginConfigLock.Unlock() + // Check again since we just released the lock, we may have raced. + if cfg, present := i.pluginConfigs[key]; present { + return cfg + } + log.Debugf("Loading configuration for plugin %s", name) + cfg := pluginConfig(pluginState, pkgState, name) + i.pluginConfigs[key] = cfg + return cfg +} + func (i *interpreter) loadPluginConfig(s *scope, pluginState *core.BuildState) { if pluginState.RepoConfig == nil { return @@ -204,19 +239,14 @@ func (i *interpreter) loadPluginConfig(s *scope, pluginState *core.BuildState) { return } - log.Debugf("Loading configuration for plugin %s", pluginName) - - if s.config.overlay == nil { - s.config.overlay = pyDict{} - } - key := strings.ToUpper(pluginName) if _, ok := s.config.overlay[key]; ok { return } - - cfg := pluginConfig(pluginState, s.state) - s.config.overlay[key] = cfg + if s.config.overlay == nil { + s.config.overlay = pyDict{} + } + s.config.overlay[key] = i.loadCachedPluginConfig(pluginState, s.state, strings.ToLower(pluginName)) } func toPyObject(key, val, toType string, optional bool) pyObject { diff --git a/src/parse/asp/interpreter.go b/src/parse/asp/interpreter.go index 2f1373ea19..0382f3e733 100644 --- a/src/parse/asp/interpreter.go +++ b/src/parse/asp/interpreter.go @@ -35,6 +35,9 @@ type interpreter struct { regexCache *cmap.Map[string, *regexp.Regexp] + pluginConfigs map[pluginConfigKey]pyDict + pluginConfigLock sync.RWMutex + // packageMetadata store scope level metadata for each package. packageMetadata *cmap.Map[core.BuildLabel, scopeMetadata] @@ -52,12 +55,13 @@ func newInterpreter(state *core.BuildState, p *Parser, callbacks Callbacks) *int s.metadata = &noopScopeMetadata{} i := &interpreter{ - scope: s, - parser: p, - configs: map[*core.BuildState]*pyConfig{}, - limiter: make(semaphore, state.Config.Parse.NumThreads), - regexCache: cmap.New[string, *regexp.Regexp](cmap.SmallShardCount, cmap.XXHash), - callbacks: callbacks, + scope: s, + parser: p, + configs: map[*core.BuildState]*pyConfig{}, + limiter: make(semaphore, state.Config.Parse.NumThreads), + regexCache: cmap.New[string, *regexp.Regexp](cmap.SmallShardCount, cmap.XXHash), + pluginConfigs: map[pluginConfigKey]pyDict{}, + callbacks: callbacks, } // If we're creating an interpreter for a subrepo, we should share the subinclude cache. if p.interpreter != nil { From 016088a63997ecd0e67efd7f5827f9727f0a8234 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 7 Nov 2026 10:21:02 +0000 Subject: [PATCH 2/5] Freeze --- src/parse/asp/builtins.go | 24 ++++++++++++++---------- src/parse/asp/config.go | 4 ++-- src/parse/asp/interpreter.go | 4 ++-- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 22f4e618dc..3777c2c5fb 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -246,16 +246,9 @@ func pkg(s *scope, args []pyObject) pyObject { // Merge in the existing config for dictionaries if overrides, ok := v.(pyDict); ok { if pluginConfig, ok := configVal.(pyDict); ok { - newPluginConfig := pluginConfig.Copy() - for pluginKey, override := range overrides { - pluginKey = strings.ToUpper(pluginKey) - if _, ok := newPluginConfig[pluginKey]; !ok { - s.Error("error calling package(): %s.%s is not a known config value", k, pluginKey) - } - - newPluginConfig.IndexAssign(pyString(pluginKey), override) - } - v = newPluginConfig + v = mergePluginConfig(s, overrides, pluginConfig.Copy(), k) + } else if pluginConfig, ok := configVal.(pyFrozenDict); ok { + v = mergePluginConfig(s, overrides, pluginConfig.Copy(), k) } else { s.Error("error calling package(): can't assign a dict to %s as it's not a dict", k) } @@ -265,6 +258,17 @@ func pkg(s *scope, args []pyObject) pyObject { return None } +func mergePluginConfig(s *scope, overrides, newPluginConfig pyDict, key string) pyDict { + for pluginKey, override := range overrides { + pluginKey = strings.ToUpper(pluginKey) + if _, ok := newPluginConfig[pluginKey]; !ok { + s.Error("error calling package(): %s.%s is not a known config value", key, pluginKey) + } + newPluginConfig.IndexAssign(pyString(pluginKey), override) + } + return newPluginConfig +} + func tag(s *scope, args []pyObject) pyObject { name := args[0].String() tag := args[1].String() diff --git a/src/parse/asp/config.go b/src/parse/asp/config.go index 2415c99b4d..b9c253bc49 100644 --- a/src/parse/asp/config.go +++ b/src/parse/asp/config.go @@ -200,7 +200,7 @@ func pluginConfig(pluginState *core.BuildState, pkgState *core.BuildState, plugi } // loadCachedPluginConfig loads the config for a particular plugin, caching it to minimise rebuilds. -func (i *interpreter) loadCachedPluginConfig(pluginState *core.BuildState, pkgState *core.BuildState, name string) pyDict { +func (i *interpreter) loadCachedPluginConfig(pluginState *core.BuildState, pkgState *core.BuildState, name string) pyFrozenDict { key := pluginConfigKey{ PluginState: pluginState, State: pkgState, @@ -223,7 +223,7 @@ func (i *interpreter) loadCachedPluginConfig(pluginState *core.BuildState, pkgSt return cfg } log.Debugf("Loading configuration for plugin %s", name) - cfg := pluginConfig(pluginState, pkgState, name) + cfg := pluginConfig(pluginState, pkgState, name).Freeze().(pyFrozenDict) i.pluginConfigs[key] = cfg return cfg } diff --git a/src/parse/asp/interpreter.go b/src/parse/asp/interpreter.go index 0382f3e733..b09661418e 100644 --- a/src/parse/asp/interpreter.go +++ b/src/parse/asp/interpreter.go @@ -35,7 +35,7 @@ type interpreter struct { regexCache *cmap.Map[string, *regexp.Regexp] - pluginConfigs map[pluginConfigKey]pyDict + pluginConfigs map[pluginConfigKey]pyFrozenDict pluginConfigLock sync.RWMutex // packageMetadata store scope level metadata for each package. @@ -60,7 +60,7 @@ func newInterpreter(state *core.BuildState, p *Parser, callbacks Callbacks) *int configs: map[*core.BuildState]*pyConfig{}, limiter: make(semaphore, state.Config.Parse.NumThreads), regexCache: cmap.New[string, *regexp.Regexp](cmap.SmallShardCount, cmap.XXHash), - pluginConfigs: map[pluginConfigKey]pyDict{}, + pluginConfigs: map[pluginConfigKey]pyFrozenDict{}, callbacks: callbacks, } // If we're creating an interpreter for a subrepo, we should share the subinclude cache. From 69e8df920eff5b93a2a56ef79b9a34cfed39ff2a Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 7 Nov 2026 10:31:52 +0000 Subject: [PATCH 3/5] Handle frozen dicts --- src/parse/asp/builtins.go | 32 ++++++++++++++------------------ src/parse/asp/config.go | 2 +- src/parse/asp/objects.go | 2 +- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 3777c2c5fb..27fad373bf 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -244,11 +244,18 @@ func pkg(s *scope, args []pyObject) pyObject { s.Assert(configVal != nil, "error calling package(): %s is not a known config value", k) // Merge in the existing config for dictionaries - if overrides, ok := v.(pyDict); ok { + if overrides, ok := asDict(v); ok { if pluginConfig, ok := configVal.(pyDict); ok { - v = mergePluginConfig(s, overrides, pluginConfig.Copy(), k) - } else if pluginConfig, ok := configVal.(pyFrozenDict); ok { - v = mergePluginConfig(s, overrides, pluginConfig.Copy(), k) + newPluginConfig := pluginConfig.Copy() + for pluginKey, override := range overrides { + pluginKey = strings.ToUpper(pluginKey) + if _, ok := newPluginConfig[pluginKey]; !ok { + s.Error("error calling package(): %s.%s is not a known config value", k, pluginKey) + } + + newPluginConfig.IndexAssign(pyString(pluginKey), override) + } + v = newPluginConfig } else { s.Error("error calling package(): can't assign a dict to %s as it's not a dict", k) } @@ -258,17 +265,6 @@ func pkg(s *scope, args []pyObject) pyObject { return None } -func mergePluginConfig(s *scope, overrides, newPluginConfig pyDict, key string) pyDict { - for pluginKey, override := range overrides { - pluginKey = strings.ToUpper(pluginKey) - if _, ok := newPluginConfig[pluginKey]; !ok { - s.Error("error calling package(): %s.%s is not a known config value", key, pluginKey) - } - newPluginConfig.IndexAssign(pyString(pluginKey), override) - } - return newPluginConfig -} - func tag(s *scope, args []pyObject) pyObject { name := args[0].String() tag := args[1].String() @@ -519,11 +515,11 @@ func isType(obj pyObject, name string) bool { return name == "str" case *pyRange: return name == "range" - case pyList: + case pyList, pyFrozenList: return name == "list" - case pyDict: + case pyDict, pyFrozenDict: return name == "dict" - case *pyConfig: + case *pyConfig, *pyFrozenConfig: return name == "config" case *pyFunc: return name == "callable" diff --git a/src/parse/asp/config.go b/src/parse/asp/config.go index b9c253bc49..ce1be97fda 100644 --- a/src/parse/asp/config.go +++ b/src/parse/asp/config.go @@ -141,7 +141,7 @@ func getConfigKey(aspKey, configKey string) string { return strings.ToLower(configKey) } -// pluginConfig loads the plugin's config into a pyDict. It will load con +// pluginConfig loads the plugin's config into a pyDict. func pluginConfig(pluginState *core.BuildState, pkgState *core.BuildState, pluginName string) pyDict { var extraVals map[string][]string var ret pyDict diff --git a/src/parse/asp/objects.go b/src/parse/asp/objects.go index cd23bb3d20..66e3b9e517 100644 --- a/src/parse/asp/objects.go +++ b/src/parse/asp/objects.go @@ -510,7 +510,7 @@ func (d pyDict) Operator(operator Operator, operand pyObject) pyObject { } panic("unknown dict key: " + s.String()) case Union: - d2, ok := operand.(pyDict) + d2, ok := asDict(operand) if !ok { panic("Operator to | must be another dict, not " + operand.Type()) } From 65c7fc51eb0309afa6e49cca7504f5a0b214e3e1 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 7 Nov 2026 10:37:36 +0000 Subject: [PATCH 4/5] Add tests --- src/parse/asp/builtins.go | 2 +- src/parse/asp/config_test.go | 138 +++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 27fad373bf..e75fead102 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -245,7 +245,7 @@ func pkg(s *scope, args []pyObject) pyObject { // Merge in the existing config for dictionaries if overrides, ok := asDict(v); ok { - if pluginConfig, ok := configVal.(pyDict); ok { + if pluginConfig, ok := asDict(configVal); ok { newPluginConfig := pluginConfig.Copy() for pluginKey, override := range overrides { pluginKey = strings.ToUpper(pluginKey) diff --git a/src/parse/asp/config_test.go b/src/parse/asp/config_test.go index 2d88ab2e9b..e58c63cfca 100644 --- a/src/parse/asp/config_test.go +++ b/src/parse/asp/config_test.go @@ -1,9 +1,15 @@ package asp import ( + "context" + "reflect" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/thought-machine/please/rules" + "github.com/thought-machine/please/src/core" ) func TestResolvePluginTargetValues(t *testing.T) { @@ -19,3 +25,135 @@ func TestResolvePluginTargetValues(t *testing.T) { values = resolvePluginValue([]string{"//path/to:target"}, "") assert.Equal(t, []string{"/////path/to:target"}, values) } + +// newPluginConfigTest returns an interpreter with builtins loaded, the host state, and the state of a +// plugin named "foo" with two config fields: BAR (inherited from the host) and BAZ (not inherited). +func newPluginConfigTest(t *testing.T) (*interpreter, *core.BuildState, *core.BuildState) { + t.Helper() + state := core.NewDefaultBuildState() + parser := NewParser(state, nil) + src, err := rules.ReadAsset("builtins.build_defs") + require.NoError(t, err) + parser.MustLoadBuiltins("builtins.build_defs", src) + + pluginState := state.ForSubrepo("foo", false) + pluginState.RepoConfig = &core.Configuration{} + pluginState.RepoConfig.PluginDefinition.Name = "foo" + pluginState.RepoConfig.PluginConfig = map[string]*core.PluginConfigDefinition{ + "bar": {DefaultValue: []string{"default_bar"}, Inherit: true}, + "baz": {DefaultValue: []string{"default_baz"}, Inherit: false}, + } + return parser.interpreter, state, pluginState +} + +// subrepoState returns a subrepo state under the given parent, with the given overrides for the foo plugin. +// With no overrides it looks like a subrepo with no .plzconfig (e.g. a go_repo module). +func subrepoState(parent *core.BuildState, name string, overrides map[string][]string) *core.BuildState { + s := parent.ForSubrepo(name, false) + s.RepoConfig = &core.Configuration{} // as Initialise does + if overrides != nil { + s.RepoConfig.Plugin = map[string]*core.Plugin{"foo": {ExtraValues: overrides}} + } + return s +} + +// runWithPluginConfig interprets some code in a fresh package scope with the foo plugin's config loaded, +// the same way interpretAll sets one up. +func runWithPluginConfig(t *testing.T, i *interpreter, pluginState *core.BuildState, code string) (*scope, error) { + t.Helper() + s := i.scope.NewPackagedScope(context.Background(), core.NewPackage("test/package"), 1) + s.config = i.getConfig(s.state).Copy() + s.Set("CONFIG", s.config) + i.loadPluginConfig(s, pluginState) + stmts, err := i.parser.ParseData([]byte(code), "BUILD") + require.NoError(t, err) + _, err = i.interpretStatements(s, i.parser.optimise(stmts)) + return s, err +} + +// fooConfig returns the foo plugin's config as seen by the given scope. +func fooConfig(t *testing.T, s *scope) pyDict { + t.Helper() + d, ok := asDict(s.config.MustGet("FOO")) + require.True(t, ok, "CONFIG.FOO should be a dict") + return d +} + +func TestPluginConfigCacheSharing(t *testing.T) { + i, state, pluginState := newPluginConfigTest(t) + state.RepoConfig.Plugin = map[string]*core.Plugin{ + "foo": {ExtraValues: map[string][]string{"bar": {"host_bar"}, "baz": {"host_baz"}}}, + } + a := subrepoState(state, "a", nil) + b := subrepoState(state, "b", nil) + c := subrepoState(state, "c", map[string][]string{"baz": {"c_baz"}}) + + host := i.loadCachedPluginConfig(pluginState, state, "foo") + cfgA := i.loadCachedPluginConfig(pluginState, a, "foo") + cfgB := i.loadCachedPluginConfig(pluginState, b, "foo") + cfgC := i.loadCachedPluginConfig(pluginState, c, "foo") + + assert.Equal(t, pyString("host_bar"), host.pyDict["BAR"]) + assert.Equal(t, pyString("host_baz"), host.pyDict["BAZ"]) + // Subrepos without overrides inherit BAR, but BAZ isn't inherited so gets reset to its default. + assert.Equal(t, pyString("host_bar"), cfgA.pyDict["BAR"]) + assert.Equal(t, pyString("default_baz"), cfgA.pyDict["BAZ"]) + // A subrepo with its own override gets that. + assert.Equal(t, pyString("host_bar"), cfgC.pyDict["BAR"]) + assert.Equal(t, pyString("c_baz"), cfgC.pyDict["BAZ"]) + + // Override-free siblings share a single dict; the host and the overriding subrepo have their own. + ptr := func(d pyFrozenDict) uintptr { return reflect.ValueOf(d.pyDict).Pointer() } + assert.Equal(t, ptr(cfgA), ptr(cfgB)) + assert.NotEqual(t, ptr(cfgA), ptr(host)) + assert.NotEqual(t, ptr(cfgA), ptr(cfgC)) + // And repeated lookups for the same state come from the cache. + assert.Equal(t, ptr(host), ptr(i.loadCachedPluginConfig(pluginState, state, "foo"))) + assert.Equal(t, ptr(cfgC), ptr(i.loadCachedPluginConfig(pluginState, c, "foo"))) +} + +func TestPluginConfigPackageOverride(t *testing.T) { + i, _, pluginState := newPluginConfigTest(t) + s, err := runWithPluginConfig(t, i, pluginState, `package(foo = {"bar": "overridden"})`) + require.NoError(t, err) + assert.Equal(t, pyString("overridden"), fooConfig(t, s)["BAR"]) + assert.Equal(t, pyString("default_baz"), fooConfig(t, s)["BAZ"]) + + // The override must not leak into the cached config that other packages get. + s, err = runWithPluginConfig(t, i, pluginState, ``) + require.NoError(t, err) + assert.Equal(t, pyString("default_bar"), fooConfig(t, s)["BAR"]) +} + +func TestPluginConfigPackageOverrideUnknownKey(t *testing.T) { + i, _, pluginState := newPluginConfigTest(t) + _, err := runWithPluginConfig(t, i, pluginState, `package(foo = {"wibble": "x"})`) + require.Error(t, err) + assert.Contains(t, err.Error(), "FOO.WIBBLE is not a known config value") +} + +func TestPluginConfigIsImmutable(t *testing.T) { + i, _, pluginState := newPluginConfigTest(t) + _, err := runWithPluginConfig(t, i, pluginState, "foo = CONFIG.FOO\nfoo[\"BAR\"] = \"mutated\"") + require.Error(t, err) + assert.Contains(t, err.Error(), "dict is immutable") + + // Nothing else sees the attempted change. + s, err := runWithPluginConfig(t, i, pluginState, ``) + require.NoError(t, err) + assert.Equal(t, pyString("default_bar"), fooConfig(t, s)["BAR"]) +} + +func TestPluginConfigBehavesAsDict(t *testing.T) { + i, _, pluginState := newPluginConfigTest(t) + s, err := runWithPluginConfig(t, i, pluginState, ` +is_dict = isinstance(CONFIG.FOO, dict) +merged = {"QUX": "qux"} | CONFIG.FOO +`) + require.NoError(t, err) + assert.Equal(t, True, s.Lookup("is_dict")) + merged, ok := asDict(s.Lookup("merged")) + require.True(t, ok) + assert.Equal(t, pyString("qux"), merged["QUX"]) + assert.Equal(t, pyString("default_bar"), merged["BAR"]) +} From 49db7b29e001bd640713d4225636ae2a250e7647 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 7 Nov 2026 10:48:37 +0000 Subject: [PATCH 5/5] Add GC --- src/please.go | 1 + 1 file changed, 1 insertion(+) diff --git a/src/please.go b/src/please.go index f88fa5ebc5..728d447166 100644 --- a/src/please.go +++ b/src/please.go @@ -1638,6 +1638,7 @@ func execute(command string) int { } defer f.Close() defer pprof.WriteHeapProfile(f) + defer runtime.GC() } if opts.MutexProfile != "" { runtime.SetMutexProfileFraction(1)