-
Notifications
You must be signed in to change notification settings - Fork 9.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Memoize Block.DecoderSpec #26577
Memoize Block.DecoderSpec #26577
Conversation
Codecov Report
|
7dd016a
to
b302869
Compare
b302869
to
b046eb3
Compare
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.
Not a great benchmark, but record it here for posterity. Practical testing with the AWS provider gives us a 98% speedup for simple plans.
b046eb3
to
dd8a8bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
Co-authored-by: Kristin Laemmert <mildwonkey@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@alisdair, yes the "mutability" is odd here, but the |
I don't have any concerns! The absolute most I'd suggest is adding your above comment about how ✅ |
5b17da9
to
e27ecba
Compare
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. |
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:
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 thatBlock
is assigned by value toNestedBlock
, 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 byspecCache
will have no impact, since the number of calls toDecoderSpec
will be significantly reduced.Fixes #25889