Memoize Block.DecoderSpec

[jbardin]

DecoderSpec is called many times throughout terraform, and in the process of checking for BlocktoAttr fixups, it may be called n^2 times recursively. While this is not normally a problem for most common resources, with deeply nested schemas this can cause serious performance issues.

Because of the delicate nature of the BlockToAttr fixup code in dealing with legacy provider schemas, it would be preferable to leave that code as-is for he time being if at all possible. Rather than refactor the BlockToAttr code ((if that's possible at all without subtle breaking changes), we have an easy performance gain here by caching the Block.DecoderSpec calls.

The benchmark shown here produced the following results:

name               old time/op    new time/op    delta
FixUpBlockAttrs-4     9.13s ± 1%     0.84s ± 3%  -90.78%  (p=0.000 n=10+8)

name               old alloc/op   new alloc/op   delta
FixUpBlockAttrs-4    6.02GB ± 0%    0.41GB ± 0%  -93.24%  (p=0.000 n=10+9)

name               old allocs/op  new allocs/op  delta
FixUpBlockAttrs-4     54.7M ± 0%      3.7M ± 0%  -93.21%  (p=0.000 n=10+9)

and testing a simple configuration with a aws_wafv2_web_acl resource in the CLI produced roughly a 98% percent speedup.

The less attractive part of this change is the addition of the package-level specCache. Normally one would pair the volatile field in the struct with its own mutex, but due to the fact that Block is assigned by value to NestedBlock, and often copied by assignment, there is no way to insert a mutex without introducing breaking changes in the public API. The package-level mutex used by specCache will have no impact, since the number of calls to DecoderSpec will be significantly reduced.

Fixes #25889

[codecov]

Codecov Report

Merging #26577 into master will increase coverage by 0.01%.
The diff coverage is 66.66%.

Impacted Files	Coverage Δ
configs/configschema/decoder_spec.go	78.37% <66.66%> (-3.77%)	⬇️
terraform/eval_apply.go	72.22% <0.00%> (-0.62%)	⬇️
terraform/node_resource_apply_instance.go	75.79% <0.00%> (+0.79%)	⬆️
terraform/eval_diff.go	68.45% <0.00%> (+0.93%)	⬆️

[mildwonkey]

This looks great to me, and I love the benchmarks, but the implementation is out of my golang comfort zone (garbage collection?! unsafe pointers?! WIZARDRY) so I'd like to see a second 👍

[alisdair]

This makes sense to me! As someone with no context for when DecoderSpec is used, it feels a bit spooky to memoize using a pointer to a mutable struct as the key. But I don't think there's a good alternative, and as far as I can tell we never mutate a Block struct anyway—any transformations (like NoneRequired) always return a new value.

[jbardin]

@alisdair, yes the "mutability" is odd here, but the Block type should be used as if it were an immutable value once the full tree is constructed. It's a representation of a schema, which cannot change while it's being used, so any modifications during execution would be an error. It didn't seem worth the effort to generate a recursive hash for the block tree, but that is technically possible if there are still concerns.

[alisdair]

I don't have any concerns! The absolute most I'd suggest is adding your above comment about how Block is used to the source near the cache struct for any future readers who worry needlessly about the same thing. But that's very optional.

✅

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.