Cache plugin configs

src/parse/asp/builtins.go

@@ -244,8 +244,8 @@ 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 pluginConfig, ok := configVal.(pyDict); ok {
+		if overrides, ok := asDict(v); ok {
+			if pluginConfig, ok := asDict(configVal); ok {
 				newPluginConfig := pluginConfig.Copy()
 				for pluginKey, override := range overrides {
 					pluginKey = strings.ToUpper(pluginKey)
@@ -515,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"

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() {
@@ -134,17 +141,16 @@ func getConfigKey(aspKey, configKey string) string {
 	return strings.ToLower(configKey)
 }
 
-// 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)
+// 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
 	if pkgState.ParentState == nil {
 		extraVals = getExtraVals(pkgState.RepoConfig, pluginName)
 		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) pyFrozenDict {
+	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).Freeze().(pyFrozenDict)
+	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 {

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"])
+}

src/parse/asp/interpreter.go

@@ -35,6 +35,9 @@ type interpreter struct {
 
 	regexCache *cmap.Map[string, *regexp.Regexp]
 
+	pluginConfigs    map[pluginConfigKey]pyFrozenDict
+	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]pyFrozenDict{},
+		callbacks:     callbacks,
 	}
 	// If we're creating an interpreter for a subrepo, we should share the subinclude cache.
 	if p.interpreter != nil {

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())
 		}

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)