From d40e2fb8d1092217ed7f96374d1038a03d2c9c78 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Oct 2020 15:22:37 -0400 Subject: [PATCH 1/4] cache DecoderSpec calls DecoderSpec may be called many times, and deeply recursive calls are expensive. Since we cannot synchronize the Blocks themselves due to them being copied in parts of the code, we use a separate cache to store the generated Specs. --- configs/configschema/decoder_spec.go | 62 ++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/configs/configschema/decoder_spec.go b/configs/configschema/decoder_spec.go index 2c21ca5e5..6dbdce21e 100644 --- a/configs/configschema/decoder_spec.go +++ b/configs/configschema/decoder_spec.go @@ -1,11 +1,68 @@ package configschema import ( + "runtime" + "sync" + "unsafe" + "github.com/hashicorp/hcl/v2/hcldec" ) var mapLabelNames = []string{"key"} +// specCache is a global cache of all the generated hcldec.Spec values for +// Blocks. This cache is used by the Block.DecoderSpec method to memoize calls +// and prevent unnecessary regeneration of the spec, especially when they are +// large and deeply nested. +// Caching these externally rather than within the struct is required because +// Blocks are used by value and copied when working with NestedBlocks, and the +// copying of the value prevents any safe synchronisation of the struct itself. +type specCache struct { + sync.Mutex + specs map[uintptr]hcldec.Spec +} + +var decoderSpecCache = specCache{ + specs: map[uintptr]hcldec.Spec{}, +} + +// get returns the Spec associated with eth given Block, or nil if non is +// found. +func (s *specCache) get(b *Block) hcldec.Spec { + s.Lock() + defer s.Unlock() + k := uintptr(unsafe.Pointer(b)) + return s.specs[k] +} + +// set stores the given Spec as being the result of b.DecoderSpec(). +func (s *specCache) set(b *Block, spec hcldec.Spec) { + s.Lock() + defer s.Unlock() + + // the uintptr value gets us a unique identifier for each block, without + // tying this to the block value itself. + k := uintptr(unsafe.Pointer(b)) + if _, ok := s.specs[k]; ok { + return + } + + s.specs[k] = spec + + // This must use a finalizer tied to the Block, otherwise we'll continue to + // build up Spec values as the Blocks are recycled. + runtime.SetFinalizer(b, s.delete) +} + +// delete removes the spec associated with the given Block. +func (s *specCache) delete(b *Block) { + s.Lock() + defer s.Unlock() + + k := uintptr(unsafe.Pointer(b)) + delete(s.specs, k) +} + // DecoderSpec returns a hcldec.Spec that can be used to decode a HCL Body // using the facilities in the hcldec package. // @@ -18,6 +75,10 @@ func (b *Block) DecoderSpec() hcldec.Spec { return ret } + if spec := decoderSpecCache.get(b); spec != nil { + return spec + } + for name, attrS := range b.Attributes { ret[name] = attrS.decoderSpec(name) } @@ -111,6 +172,7 @@ func (b *Block) DecoderSpec() hcldec.Spec { } } + decoderSpecCache.set(b, ret) return ret } From dd8a8bdea1809f1969bff17636d839d50fae59a5 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Oct 2020 16:30:55 -0400 Subject: [PATCH 2/4] add benchmark used for DecoderSpec Not a great benchmark, but record it here for posterity. Practical testing with the AWS provider gives us a 98% speedup for simple plans. --- lang/blocktoattr/fixup_bench_test.go | 97 ++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 lang/blocktoattr/fixup_bench_test.go diff --git a/lang/blocktoattr/fixup_bench_test.go b/lang/blocktoattr/fixup_bench_test.go new file mode 100644 index 000000000..1515d2eff --- /dev/null +++ b/lang/blocktoattr/fixup_bench_test.go @@ -0,0 +1,97 @@ +package blocktoattr + +import ( + "testing" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hcldec" + "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/terraform/configs/configschema" + "github.com/zclconf/go-cty/cty" +) + +func ambiguousNestedBlock(nesting int) *configschema.NestedBlock { + ret := &configschema.NestedBlock{ + Nesting: configschema.NestingList, + Block: configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "a": {Type: cty.String, Required: true}, + "b": {Type: cty.String, Optional: true}, + }, + }, + } + if nesting > 0 { + ret.BlockTypes = map[string]*configschema.NestedBlock{ + "nested0": ambiguousNestedBlock(nesting - 1), + "nested1": ambiguousNestedBlock(nesting - 1), + "nested2": ambiguousNestedBlock(nesting - 1), + "nested3": ambiguousNestedBlock(nesting - 1), + "nested4": ambiguousNestedBlock(nesting - 1), + "nested5": ambiguousNestedBlock(nesting - 1), + "nested6": ambiguousNestedBlock(nesting - 1), + "nested7": ambiguousNestedBlock(nesting - 1), + "nested8": ambiguousNestedBlock(nesting - 1), + "nested9": ambiguousNestedBlock(nesting - 1), + } + } + return ret +} + +func schemaWithAmbiguousNestedBlock(nesting int) *configschema.Block { + return &configschema.Block{ + BlockTypes: map[string]*configschema.NestedBlock{ + "maybe_block": ambiguousNestedBlock(nesting), + }, + } +} + +const configForFixupBlockAttrsBenchmark = ` +maybe_block { + a = "hello" + b = "world" + nested0 { + a = "the" + nested1 { + a = "deeper" + nested2 { + a = "we" + nested3 { + a = "go" + b = "inside" + } + } + } + } +} +` + +func configBodyForFixupBlockAttrsBenchmark() hcl.Body { + f, diags := hclsyntax.ParseConfig([]byte(configForFixupBlockAttrsBenchmark), "", hcl.Pos{Line: 1, Column: 1}) + if diags.HasErrors() { + panic("test configuration is invalid") + } + return f.Body +} + +func BenchmarkFixUpBlockAttrs(b *testing.B) { + for i := 0; i < b.N; i++ { + b.StopTimer() + body := configBodyForFixupBlockAttrsBenchmark() + schema := schemaWithAmbiguousNestedBlock(5) + b.StartTimer() + + spec := schema.DecoderSpec() + fixedBody := FixUpBlockAttrs(body, schema) + val, diags := hcldec.Decode(fixedBody, spec, nil) + if diags.HasErrors() { + b.Fatal("diagnostics during decoding", diags) + } + if !val.Type().IsObjectType() { + b.Fatal("result is not an object") + } + blockVal := val.GetAttr("maybe_block") + if !blockVal.Type().IsListType() || blockVal.LengthInt() != 1 { + b.Fatal("result has wrong value for 'maybe_block'") + } + } +} From bb76c3b50c39e575f785c21fef634269eb586f08 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 14 Oct 2020 10:33:44 -0400 Subject: [PATCH 3/4] Update configs/configschema/decoder_spec.go Co-authored-by: Kristin Laemmert --- configs/configschema/decoder_spec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configs/configschema/decoder_spec.go b/configs/configschema/decoder_spec.go index 6dbdce21e..5357db815 100644 --- a/configs/configschema/decoder_spec.go +++ b/configs/configschema/decoder_spec.go @@ -26,7 +26,7 @@ var decoderSpecCache = specCache{ specs: map[uintptr]hcldec.Spec{}, } -// get returns the Spec associated with eth given Block, or nil if non is +// get returns the Spec associated with the given Block, or nil if none is // found. func (s *specCache) get(b *Block) hcldec.Spec { s.Lock() From e27ecba6e4f380cc066f0e760fba807f82ed6063 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 14 Oct 2020 12:28:06 -0400 Subject: [PATCH 4/4] extended cache comments --- configs/configschema/decoder_spec.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/configs/configschema/decoder_spec.go b/configs/configschema/decoder_spec.go index 5357db815..f7193ec07 100644 --- a/configs/configschema/decoder_spec.go +++ b/configs/configschema/decoder_spec.go @@ -17,6 +17,12 @@ var mapLabelNames = []string{"key"} // Caching these externally rather than within the struct is required because // Blocks are used by value and copied when working with NestedBlocks, and the // copying of the value prevents any safe synchronisation of the struct itself. +// +// While we are using the *Block pointer as the cache key, and the Block +// contents are mutable, once a Block is created it is treated as immutable for +// the duration of its life. Because a Block is a representation of a logical +// schema, which cannot change while it's being used, any modifications to the +// schema during execution would be an error. type specCache struct { sync.Mutex specs map[uintptr]hcldec.Spec @@ -26,7 +32,7 @@ var decoderSpecCache = specCache{ specs: map[uintptr]hcldec.Spec{}, } -// get returns the Spec associated with the given Block, or nil if none is +// get returns the Spec associated with eth given Block, or nil if non is // found. func (s *specCache) get(b *Block) hcldec.Spec { s.Lock()