core: Potential optimization of ModuleExpansionTransformer

internal/addrs/module.go

@@ -165,3 +165,11 @@ func (m Module) Ancestors() []Module {
 func (m Module) configMoveableSigil() {
 	// ModuleInstance is moveable
 }
+
+func (m Module) UniqueKey() UniqueKey {
+	return moduleUniqueKey(m.String())
+}
+
+type moduleUniqueKey string
+
+func (k moduleUniqueKey) uniqueKeySigil() {}

internal/configs/configload/testing.go

@@ -17,7 +17,7 @@ import (
 // In the case of any errors, t.Fatal (or similar) will be called to halt
 // execution of the test, so the calling test does not need to handle errors
 // itself.
-func NewLoaderForTests(t *testing.T) (*Loader, func()) {
+func NewLoaderForTests(t testing.TB) (*Loader, func()) {
 	t.Helper()
 
 	modulesDir, err := ioutil.TempDir("", "tf-configs")

internal/terraform/terraform_test.go

@@ -42,13 +42,13 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
-func testModule(t *testing.T, name string) *configs.Config {
+func testModule(t testing.TB, name string) *configs.Config {
 	t.Helper()
 	c, _ := testModuleWithSnapshot(t, name)
 	return c
 }
 
-func testModuleWithSnapshot(t *testing.T, name string) (*configs.Config, *configload.Snapshot) {
+func testModuleWithSnapshot(t testing.TB, name string) (*configs.Config, *configload.Snapshot) {
 	t.Helper()
 
 	dir := filepath.Join(fixtureDir, name)
@@ -85,7 +85,7 @@ func testModuleWithSnapshot(t *testing.T, name string) (*configs.Config, *config
 
 // testModuleInline takes a map of path -> config strings and yields a config
 // structure with those files loaded from disk
-func testModuleInline(t *testing.T, sources map[string]string) *configs.Config {
+func testModuleInline(t testing.TB, sources map[string]string) *configs.Config {
 	t.Helper()
 
 	cfgPath := t.TempDir()

internal/terraform/testdata/module-expansion-nesting/a/a/module-expansion-nesting-aa.tf

@@ -0,0 +1,2 @@
+
+resource "foo" "bar" {}

internal/terraform/testdata/module-expansion-nesting/a/b/module-expansion-nesting-ab.tf

@@ -0,0 +1,2 @@
+
+resource "foo" "bar" {}

internal/terraform/testdata/module-expansion-nesting/a/module-expansion-nesting-a.tf

@@ -0,0 +1,12 @@
+
+module "a" {
+  count  = 1
+  source = "./a"
+}
+
+module "b" {
+  count  = 1
+  source = "./b"
+}
+
+resource "foo" "bar" {}

internal/terraform/testdata/module-expansion-nesting/b/a/module-expansion-nesting-ba.tf

@@ -0,0 +1,2 @@
+
+resource "foo" "bar" {}

internal/terraform/testdata/module-expansion-nesting/b/b/module-expansion-nesting-bb.tf

@@ -0,0 +1,2 @@
+
+resource "foo" "bar" {}

internal/terraform/testdata/module-expansion-nesting/b/module-expansion-nesting-b.tf

@@ -0,0 +1,12 @@
+
+module "a" {
+  count  = 1
+  source = "./a"
+}
+
+module "b" {
+  count  = 1
+  source = "./b"
+}
+
+resource "foo" "bar" {}

internal/terraform/testdata/module-expansion-nesting/module-expansion-nesting.tf

@@ -0,0 +1,24 @@
+
+# This configuration includes a bunch of module nesting in support of
+# benchmarking ModuleExpansionTransformer in BenchmarkModuleExpansionTransformer.
+#
+# The transformer recursively visits all modules in the tree, and so the shape
+# of this tree is intended to allow the benchmark to be sensitive to
+# differences between performance costs that:
+# - scale by total number of distinct modules, regardless of tree shape
+# - scale by the depth of nesting of the module tree
+# - are fixed, regardless of number of modules or tree shape
+
+module "a" {
+  count = 1
+
+  source = "./a"
+}
+
+module "b" {
+  count = 1
+
+  source = "./b"
+}
+
+resource "foo" "bar" {}

internal/terraform/transform_module_expansion.go

@@ -23,16 +23,18 @@ type ModuleExpansionTransformer struct {
 	// to alter the evaluation behavior.
 	Concrete ConcreteModuleNodeFunc
 
-	closers map[string]*nodeCloseModule
+	closers map[addrs.UniqueKey]*nodeCloseModule
 }
 
 func (t *ModuleExpansionTransformer) Transform(g *Graph) error {
-	t.closers = make(map[string]*nodeCloseModule)
+	moduleKeys := nodeModuleKeyMap(g)
+
+	t.closers = make(map[addrs.UniqueKey]*nodeCloseModule)
 	// The root module is always a singleton and so does not need expansion
 	// processing, but any descendent modules do. We'll process them
 	// recursively using t.transform.
 	for _, cfg := range t.Config.Children {
-		err := t.transform(g, cfg, nil)
+		err := t.transform(g, cfg, nil, moduleKeys)
 		if err != nil {
 			return err
 		}
@@ -59,7 +61,7 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error {
 		if !ok {
 			continue
 		}
-		if closer, ok := t.closers[pather.ModulePath().String()]; ok {
+		if closer, ok := t.closers[moduleKeys[pather]]; ok {
 			// The module closer depends on each child resource instance, since
 			// during apply the module expansion will complete before the
 			// individual instances are applied.
@@ -80,7 +82,7 @@ func (t *ModuleExpansionTransformer) Transform(g *Graph) error {
 	return nil
 }
 
-func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, parentNode dag.Vertex) error {
+func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, parentNode dag.Vertex, moduleKeys map[GraphNodeModulePath]addrs.UniqueKey) error {
 	_, call := c.Path.Call()
 	modCall := c.Parent.Module.ModuleCalls[call.Name]
 
@@ -102,45 +104,67 @@ func (t *ModuleExpansionTransformer) transform(g *Graph, c *configs.Config, pare
 		g.Connect(dag.BasicEdge(expander, parentNode))
 	}
 
+	ourModuleKey := c.Path.UniqueKey()
+
 	// Add the closer (which acts as the root module node) to provide a
 	// single exit point for the expanded module.
 	closer := &nodeCloseModule{
 		Addr: c.Path,
 	}
 	g.Add(closer)
+	moduleKeys[GraphNodeModulePath(closer)] = ourModuleKey
 	g.Connect(dag.BasicEdge(closer, expander))
-	t.closers[c.Path.String()] = closer
+	t.closers[c.Path.UniqueKey()] = closer
 
 	for _, childV := range g.Vertices() {
 		// don't connect a node to itself
 		if childV == expander {
 			continue
 		}
 
-		var path addrs.Module
+		var childModuleKey addrs.UniqueKey
 		switch t := childV.(type) {
 		case GraphNodeDestroyer:
 			// skip destroyers, as they can only depend on other resources.
 			continue
 
 		case GraphNodeModulePath:
-			path = t.ModulePath()
+			childModuleKey = moduleKeys[t]
 		default:
 			continue
 		}
 
-		if path.Equal(c.Path) {
+		if childModuleKey == ourModuleKey {
 			log.Printf("[TRACE] ModuleExpansionTransformer: %s must wait for expansion of %s", dag.VertexName(childV), c.Path)
 			g.Connect(dag.BasicEdge(childV, expander))
 		}
 	}
 
 	// Also visit child modules, recursively.
 	for _, cc := range c.Children {
-		if err := t.transform(g, cc, expander); err != nil {
+		if err := t.transform(g, cc, expander, moduleKeys); err != nil {
 			return err
 		}
 	}
 
 	return nil
 }
+
+// nodeModuleKeyMap builds a cache data structure to allow more quickly
+// deciding whether two graph nodes belong to the same module, by caching
+// the comparable UniqueKey values of each node's module path.
+//
+// The result is a map with one entry for each graph node that reports that
+// it belongs to a module by implementing GraphNodeModulePath. The keys are
+// the nodes themselves, which assumes that our node implementations are always
+// comparable types; we typically ensure that's true by implementing
+// GraphNodeModulePath as a method on a pointer type.
+func nodeModuleKeyMap(g *Graph) map[GraphNodeModulePath]addrs.UniqueKey {
+	ret := make(map[GraphNodeModulePath]addrs.UniqueKey)
+	for _, v := range g.Vertices() {
+		if mp, ok := v.(GraphNodeModulePath); ok {
+			ret[mp] = mp.ModulePath().UniqueKey()
+		}
+	}
+	return ret
+}

internal/terraform/transform_module_expansion_test.go

@@ -0,0 +1,47 @@
+package terraform
+
+import (
+	"testing"
+)
+
+var benchmarkModuleExpansionTransformerGraph *Graph
+
+func BenchmarkModuleExpansionTransformer(b *testing.B) {
+	// We need to construct quite an elaborate set of inputs in order to
+	// make for a "realistic" run of the transformer that will generate
+	// useful benchmark results. In particular, we need to have a
+	// configuration with a relatively large number of non-root modules
+	// so that the benchmark can be sensitive to the difference between
+	// costs that scale per module or per level of nesting and costs
+	// that are fixed regardless of the configuration tree complexity.
+
+	cfg := testModule(b, "module-expansion-nesting")
+
+	for i := 0; i < b.N; i++ {
+		// We'll make sure that the graph "escapes" so that the Go compiler
+		// can't optimize it away with local optimizations.
+		benchmarkModuleExpansionTransformerGraph = func() *Graph {
+			graph := &Graph{}
+
+			// The module expansion transformer expects there to already be
+			// graph nodes representing objects within the modules in the graph,
+			// and so we'll borrow the ConfigTransformer to get an approximation
+			// of that.
+			cfgTransformer := &ConfigTransformer{
+				Config: cfg,
+			}
+			cfgTransformer.Transform(graph)
+
+			// Now we can run the module expansion transformer to add all of the
+			// expand/close nodes for the modules and the edges from the expand
+			// nodes to the resources inside.
+			modExpTransformer := &ModuleExpansionTransformer{
+				Config: cfg,
+			}
+			modExpTransformer.Transform(graph)
+
+			return graph
+		}()
+	}
+
+}