From a43b035a51d2cedefeff22954e1b31173bde97d5 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 5 Mar 2021 12:07:31 -0500 Subject: [PATCH] core: Reduce string allocations for addrs Equal Generating strings and comparing them to implement Equal is a quick and easy solution. Unfortunately when this code is in the hot path, it becomes very expensive, so this commit changes some of those instances to compare the values directly. Combined with using addr.Equal instead of checking for string equality, this makes Terraform dramatically faster for some operations, such as generating large JSON plans. --- addrs/module.go | 10 +- addrs/module_instance.go | 11 +- addrs/module_instance_test.go | 79 ++++++++++ addrs/module_test.go | 57 +++++++ addrs/output_value.go | 4 + addrs/output_value_test.go | 65 ++++++++ addrs/resource.go | 10 +- addrs/resource_test.go | 276 ++++++++++++++++++++++++++++++++++ plans/changes.go | 9 +- 9 files changed, 508 insertions(+), 13 deletions(-) create mode 100644 addrs/module_instance_test.go create mode 100644 addrs/module_test.go create mode 100644 addrs/output_value_test.go create mode 100644 addrs/resource_test.go diff --git a/addrs/module.go b/addrs/module.go index 2a3ffe3fc..18f3dbec8 100644 --- a/addrs/module.go +++ b/addrs/module.go @@ -41,7 +41,15 @@ func (m Module) String() string { } func (m Module) Equal(other Module) bool { - return m.String() == other.String() + if len(m) != len(other) { + return false + } + for i := range m { + if m[i] != other[i] { + return false + } + } + return true } func (m Module) targetableSigil() { diff --git a/addrs/module_instance.go b/addrs/module_instance.go index f3efa7eaf..7a5c03a6e 100644 --- a/addrs/module_instance.go +++ b/addrs/module_instance.go @@ -268,7 +268,16 @@ func (m ModuleInstance) String() string { // Equal returns true if the receiver and the given other value // contains the exact same parts. func (m ModuleInstance) Equal(o ModuleInstance) bool { - return m.String() == o.String() + if len(m) != len(o) { + return false + } + + for i := range m { + if m[i] != o[i] { + return false + } + } + return true } // Less returns true if the receiver should sort before the given other value diff --git a/addrs/module_instance_test.go b/addrs/module_instance_test.go new file mode 100644 index 000000000..c36e9952d --- /dev/null +++ b/addrs/module_instance_test.go @@ -0,0 +1,79 @@ +package addrs + +import ( + "fmt" + "testing" +) + +func TestModuleInstanceEqual_true(t *testing.T) { + addrs := []string{ + "module.foo", + "module.foo.module.bar", + "module.foo[1].module.bar", + `module.foo["a"].module.bar["b"]`, + `module.foo["a"].module.bar.module.baz[3]`, + } + for _, m := range addrs { + t.Run(m, func(t *testing.T) { + addr, diags := ParseModuleInstanceStr(m) + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + if !addr.Equal(addr) { + t.Fatalf("expected %#v to be equal to itself", addr) + } + }) + } +} + +func TestModuleInstanceEqual_false(t *testing.T) { + testCases := []struct { + left string + right string + }{ + { + "module.foo", + "module.bar", + }, + { + "module.foo", + "module.foo.module.bar", + }, + { + "module.foo[1]", + "module.bar[1]", + }, + { + `module.foo[1]`, + `module.foo["1"]`, + }, + { + "module.foo.module.bar", + "module.foo[1].module.bar", + }, + { + `module.foo.module.bar`, + `module.foo["a"].module.bar`, + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s = %s", tc.left, tc.right), func(t *testing.T) { + left, diags := ParseModuleInstanceStr(tc.left) + if len(diags) > 0 { + t.Fatalf("unexpected diags parsing %s: %s", tc.left, diags.Err()) + } + right, diags := ParseModuleInstanceStr(tc.right) + if len(diags) > 0 { + t.Fatalf("unexpected diags parsing %s: %s", tc.right, diags.Err()) + } + + if left.Equal(right) { + t.Fatalf("expected %#v not to be equal to %#v", left, right) + } + + if right.Equal(left) { + t.Fatalf("expected %#v not to be equal to %#v", right, left) + } + }) + } +} diff --git a/addrs/module_test.go b/addrs/module_test.go new file mode 100644 index 000000000..829704faf --- /dev/null +++ b/addrs/module_test.go @@ -0,0 +1,57 @@ +package addrs + +import ( + "fmt" + "testing" +) + +func TestModuleEqual_true(t *testing.T) { + modules := []Module{ + RootModule, + {"a"}, + {"a", "b"}, + {"a", "b", "c"}, + } + for _, m := range modules { + t.Run(m.String(), func(t *testing.T) { + if !m.Equal(m) { + t.Fatalf("expected %#v to be equal to itself", m) + } + }) + } +} + +func TestModuleEqual_false(t *testing.T) { + testCases := []struct { + left Module + right Module + }{ + { + RootModule, + Module{"a"}, + }, + { + Module{"a"}, + Module{"b"}, + }, + { + Module{"a"}, + Module{"a", "a"}, + }, + { + Module{"a", "b"}, + Module{"a", "B"}, + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s = %s", tc.left, tc.right), func(t *testing.T) { + if tc.left.Equal(tc.right) { + t.Fatalf("expected %#v not to be equal to %#v", tc.left, tc.right) + } + + if tc.right.Equal(tc.left) { + t.Fatalf("expected %#v not to be equal to %#v", tc.right, tc.left) + } + }) + } +} diff --git a/addrs/output_value.go b/addrs/output_value.go index f97eca19e..d0dfca56c 100644 --- a/addrs/output_value.go +++ b/addrs/output_value.go @@ -56,6 +56,10 @@ func (v AbsOutputValue) String() string { return fmt.Sprintf("%s.%s", v.Module.String(), v.OutputValue.String()) } +func (v AbsOutputValue) Equal(o AbsOutputValue) bool { + return v.OutputValue == o.OutputValue && v.Module.Equal(o.Module) +} + // ModuleCallOutput converts an AbsModuleOutput into a ModuleCallOutput, // returning also the module instance that the ModuleCallOutput is relative // to. diff --git a/addrs/output_value_test.go b/addrs/output_value_test.go new file mode 100644 index 000000000..e56d4ca3a --- /dev/null +++ b/addrs/output_value_test.go @@ -0,0 +1,65 @@ +package addrs + +import ( + "fmt" + "testing" +) + +func TestAbsOutputValueInstanceEqual_true(t *testing.T) { + foo, diags := ParseModuleInstanceStr("module.foo") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + foobar, diags := ParseModuleInstanceStr("module.foo[1].module.bar") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + + ovs := []AbsOutputValue{ + foo.OutputValue("a"), + foobar.OutputValue("b"), + } + for _, r := range ovs { + t.Run(r.String(), func(t *testing.T) { + if !r.Equal(r) { + t.Fatalf("expected %#v to be equal to itself", r) + } + }) + } +} + +func TestAbsOutputValueInstanceEqual_false(t *testing.T) { + foo, diags := ParseModuleInstanceStr("module.foo") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + foobar, diags := ParseModuleInstanceStr("module.foo[1].module.bar") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + + testCases := []struct { + left AbsOutputValue + right AbsOutputValue + }{ + { + foo.OutputValue("a"), + foo.OutputValue("b"), + }, + { + foo.OutputValue("a"), + foobar.OutputValue("a"), + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s = %s", tc.left, tc.right), func(t *testing.T) { + if tc.left.Equal(tc.right) { + t.Fatalf("expected %#v not to be equal to %#v", tc.left, tc.right) + } + + if tc.right.Equal(tc.left) { + t.Fatalf("expected %#v not to be equal to %#v", tc.right, tc.left) + } + }) + } +} diff --git a/addrs/resource.go b/addrs/resource.go index f0a4f79e1..8aa0efc05 100644 --- a/addrs/resource.go +++ b/addrs/resource.go @@ -29,7 +29,7 @@ func (r Resource) String() string { } func (r Resource) Equal(o Resource) bool { - return r.String() == o.String() + return r.Mode == o.Mode && r.Name == o.Name && r.Type == o.Type } // Instance produces the address for a specific instance of the receiver @@ -91,7 +91,7 @@ func (r ResourceInstance) String() string { } func (r ResourceInstance) Equal(o ResourceInstance) bool { - return r.String() == o.String() + return r.Key == o.Key && r.Resource.Equal(o.Resource) } // Absolute returns an AbsResourceInstance from the receiver and the given module @@ -171,7 +171,7 @@ func (r AbsResource) String() string { } func (r AbsResource) Equal(o AbsResource) bool { - return r.String() == o.String() + return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } // AbsResourceInstance is an absolute address for a resource instance under a @@ -236,7 +236,7 @@ func (r AbsResourceInstance) String() string { } func (r AbsResourceInstance) Equal(o AbsResourceInstance) bool { - return r.String() == o.String() + return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } // Less returns true if the receiver should sort before the given other value @@ -320,7 +320,7 @@ func (r ConfigResource) String() string { } func (r ConfigResource) Equal(o ConfigResource) bool { - return r.String() == o.String() + return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } // ResourceMode defines which lifecycle applies to a given resource. Each diff --git a/addrs/resource_test.go b/addrs/resource_test.go new file mode 100644 index 000000000..fbaa981f4 --- /dev/null +++ b/addrs/resource_test.go @@ -0,0 +1,276 @@ +package addrs + +import ( + "fmt" + "testing" +) + +func TestResourceEqual_true(t *testing.T) { + resources := []Resource{ + { + Mode: ManagedResourceMode, + Type: "a", + Name: "b", + }, + { + Mode: DataResourceMode, + Type: "a", + Name: "b", + }, + } + for _, r := range resources { + t.Run(r.String(), func(t *testing.T) { + if !r.Equal(r) { + t.Fatalf("expected %#v to be equal to itself", r) + } + }) + } +} + +func TestResourceEqual_false(t *testing.T) { + testCases := []struct { + left Resource + right Resource + }{ + { + Resource{Mode: DataResourceMode, Type: "a", Name: "b"}, + Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + }, + { + Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + Resource{Mode: ManagedResourceMode, Type: "b", Name: "b"}, + }, + { + Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + Resource{Mode: ManagedResourceMode, Type: "a", Name: "c"}, + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s = %s", tc.left, tc.right), func(t *testing.T) { + if tc.left.Equal(tc.right) { + t.Fatalf("expected %#v not to be equal to %#v", tc.left, tc.right) + } + + if tc.right.Equal(tc.left) { + t.Fatalf("expected %#v not to be equal to %#v", tc.right, tc.left) + } + }) + } +} + +func TestResourceInstanceEqual_true(t *testing.T) { + resources := []ResourceInstance{ + { + Resource: Resource{ + Mode: ManagedResourceMode, + Type: "a", + Name: "b", + }, + Key: IntKey(0), + }, + { + Resource: Resource{ + Mode: DataResourceMode, + Type: "a", + Name: "b", + }, + Key: StringKey("x"), + }, + } + for _, r := range resources { + t.Run(r.String(), func(t *testing.T) { + if !r.Equal(r) { + t.Fatalf("expected %#v to be equal to itself", r) + } + }) + } +} + +func TestResourceInstanceEqual_false(t *testing.T) { + testCases := []struct { + left ResourceInstance + right ResourceInstance + }{ + { + ResourceInstance{ + Resource: Resource{Mode: DataResourceMode, Type: "a", Name: "b"}, + Key: IntKey(0), + }, + ResourceInstance{ + Resource: Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + Key: IntKey(0), + }, + }, + { + ResourceInstance{ + Resource: Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + Key: IntKey(0), + }, + ResourceInstance{ + Resource: Resource{Mode: ManagedResourceMode, Type: "b", Name: "b"}, + Key: IntKey(0), + }, + }, + { + ResourceInstance{ + Resource: Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + Key: IntKey(0), + }, + ResourceInstance{ + Resource: Resource{Mode: ManagedResourceMode, Type: "a", Name: "c"}, + Key: IntKey(0), + }, + }, + { + ResourceInstance{ + Resource: Resource{Mode: DataResourceMode, Type: "a", Name: "b"}, + Key: IntKey(0), + }, + ResourceInstance{ + Resource: Resource{Mode: DataResourceMode, Type: "a", Name: "b"}, + Key: StringKey("0"), + }, + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s = %s", tc.left, tc.right), func(t *testing.T) { + if tc.left.Equal(tc.right) { + t.Fatalf("expected %#v not to be equal to %#v", tc.left, tc.right) + } + + if tc.right.Equal(tc.left) { + t.Fatalf("expected %#v not to be equal to %#v", tc.right, tc.left) + } + }) + } +} + +func TestAbsResourceInstanceEqual_true(t *testing.T) { + managed := Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"} + data := Resource{Mode: DataResourceMode, Type: "a", Name: "b"} + + foo, diags := ParseModuleInstanceStr("module.foo") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + foobar, diags := ParseModuleInstanceStr("module.foo[1].module.bar") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + + instances := []AbsResourceInstance{ + managed.Instance(IntKey(0)).Absolute(foo), + data.Instance(IntKey(0)).Absolute(foo), + managed.Instance(StringKey("a")).Absolute(foobar), + } + for _, r := range instances { + t.Run(r.String(), func(t *testing.T) { + if !r.Equal(r) { + t.Fatalf("expected %#v to be equal to itself", r) + } + }) + } +} + +func TestAbsResourceInstanceEqual_false(t *testing.T) { + managed := Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"} + data := Resource{Mode: DataResourceMode, Type: "a", Name: "b"} + + foo, diags := ParseModuleInstanceStr("module.foo") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + foobar, diags := ParseModuleInstanceStr("module.foo[1].module.bar") + if len(diags) > 0 { + t.Fatalf("unexpected diags: %s", diags.Err()) + } + + testCases := []struct { + left AbsResourceInstance + right AbsResourceInstance + }{ + { + managed.Instance(IntKey(0)).Absolute(foo), + data.Instance(IntKey(0)).Absolute(foo), + }, + { + managed.Instance(IntKey(0)).Absolute(foo), + managed.Instance(IntKey(0)).Absolute(foobar), + }, + { + managed.Instance(IntKey(0)).Absolute(foo), + managed.Instance(StringKey("0")).Absolute(foo), + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s = %s", tc.left, tc.right), func(t *testing.T) { + if tc.left.Equal(tc.right) { + t.Fatalf("expected %#v not to be equal to %#v", tc.left, tc.right) + } + + if tc.right.Equal(tc.left) { + t.Fatalf("expected %#v not to be equal to %#v", tc.right, tc.left) + } + }) + } +} + +func TestConfigResourceEqual_true(t *testing.T) { + resources := []ConfigResource{ + { + Resource: Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + Module: RootModule, + }, + { + Resource: Resource{Mode: DataResourceMode, Type: "a", Name: "b"}, + Module: RootModule, + }, + { + Resource: Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"}, + Module: Module{"foo"}, + }, + { + Resource: Resource{Mode: DataResourceMode, Type: "a", Name: "b"}, + Module: Module{"foo"}, + }, + } + for _, r := range resources { + t.Run(r.String(), func(t *testing.T) { + if !r.Equal(r) { + t.Fatalf("expected %#v to be equal to itself", r) + } + }) + } +} + +func TestConfigResourceEqual_false(t *testing.T) { + managed := Resource{Mode: ManagedResourceMode, Type: "a", Name: "b"} + data := Resource{Mode: DataResourceMode, Type: "a", Name: "b"} + + foo := Module{"foo"} + foobar := Module{"foobar"} + testCases := []struct { + left ConfigResource + right ConfigResource + }{ + { + ConfigResource{Resource: managed, Module: foo}, + ConfigResource{Resource: data, Module: foo}, + }, + { + ConfigResource{Resource: managed, Module: foo}, + ConfigResource{Resource: managed, Module: foobar}, + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("%s = %s", tc.left, tc.right), func(t *testing.T) { + if tc.left.Equal(tc.right) { + t.Fatalf("expected %#v not to be equal to %#v", tc.left, tc.right) + } + + if tc.right.Equal(tc.left) { + t.Fatalf("expected %#v not to be equal to %#v", tc.right, tc.left) + } + }) + } +} diff --git a/plans/changes.go b/plans/changes.go index 72b6a8938..1b4a9677f 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -51,9 +51,8 @@ func (c *Changes) Empty() bool { // resource instance of the given address, if any. Returns nil if no change is // planned. func (c *Changes) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInstanceChangeSrc { - addrStr := addr.String() for _, rc := range c.Resources { - if rc.Addr.String() == addrStr && rc.DeposedKey == states.NotDeposed { + if rc.Addr.Equal(addr) && rc.DeposedKey == states.NotDeposed { return rc } } @@ -81,9 +80,8 @@ func (c *Changes) InstancesForConfigResource(addr addrs.ConfigResource) []*Resou // the resource instance of the given address, if any. Returns nil if no change // is planned. func (c *Changes) ResourceInstanceDeposed(addr addrs.AbsResourceInstance, key states.DeposedKey) *ResourceInstanceChangeSrc { - addrStr := addr.String() for _, rc := range c.Resources { - if rc.Addr.String() == addrStr && rc.DeposedKey == key { + if rc.Addr.Equal(addr) && rc.DeposedKey == key { return rc } } @@ -94,9 +92,8 @@ func (c *Changes) ResourceInstanceDeposed(addr addrs.AbsResourceInstance, key st // OutputValue returns the planned change for the output value with the // given address, if any. Returns nil if no change is planned. func (c *Changes) OutputValue(addr addrs.AbsOutputValue) *OutputChangeSrc { - addrStr := addr.String() for _, oc := range c.Outputs { - if oc.Addr.String() == addrStr { + if oc.Addr.Equal(addr) { return oc } }