-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Performance: string builder speedup for Module.String() #31293
Conversation
making code more similar to https://github.com/hashicorp/terraform/blob/main/internal/addrs/module_instance.go
see similar idea in #28246 |
Thanks for this submission. Although I cannot commit to having this PR reviewed at this time, we acknowledge your contribution and appreciate it! |
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.
Thanks for this improvement! I added some basic tests of Module.String
, as I think it was only tested implicitly until now, and this faster implementation is a little harder to be sure about correctness.
I'm a bit surprised that you're seeing significant GC pressure from this. Do you have any more information about what in the call stack is calling Module.String
enough times to have this effect? I ask because in the past we've seen calls to String
as part of equality checks, which we should avoid by calling Equal
instead.
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
I noticed that in 0.13 and a particularly big configuration, apparently that didn't include
|
@alisdair I'm not sure if there's a connection here that prompted working on this or if it was just a coincidence, but FWIW just yesterday I happened to spot a place where we were using In that PR I was trying a different strategy for potentially improving it -- effectively, doing the |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
After the change:
before the change:
About 5x speedup for time, and only single allocation.
Inspiration for this change is perf profiling on a particularly large config which points to memory churn: