Performance: string builder speedup for Module.String()

[dennygursky]

After the change:

terraform/internal/addrs$ go test -bench=Module . -benchmem
BenchmarkModuleStringShort-12           15489140                77.80 ns/op           24 B/op          1 allocs/op          
BenchmarkModuleStringLong-12            10807070               111.8 ns/op            80 B/op          1 allocs/op   

before the change:

BenchmarkModuleStringShort-12            3290968               384.2 ns/op           120 B/op          3 allocs/op          
BenchmarkModuleStringLong-12             1859038               668.4 ns/op           304 B/op          4 allocs/op  

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:

  29.34%  terraform  terraform         [.] runtime.scanobject                                                               
  12.31%  terraform  terraform         [.] runtime.findObject                                                               
  11.94%  terraform  terraform         [.] runtime.greyobject                                                               
   8.35%  terraform  terraform         [.] runtime.heapBitsSetType                                                          
   7.25%  terraform  terraform         [.] runtime.mallocgc                                                                 
   5.10%  terraform  terraform         [.] strings.Join                                                                     
   3.12%  terraform  terraform         [.] runtime.memmove                                                                  
   3.02%  terraform  terraform         [.] runtime.gcDrain                                                                  
   2.85%  terraform  terraform         [.] google3/third_party/golang/hashicorp/terraform/addrs/addrs.Module.String         
   2.13%  terraform  terraform         [.] runtime.growslice                                                                
   1.65%  terraform  terraform         [.] google3/third_party/golang/hashicorp/terraform/terraform/terraform.(*nodeExpandMo
   1.02%  terraform  terraform         [.] runtime.sweepone                                                                 
   0.99%  terraform  terraform         [.] runtime.(*sweepLocked).sweep                                                     
   0.77%  terraform  terraform         [.] runtime.(*spanSet).pop                                                           
   0.68%  terraform  terraform         [.] runtime.memclrNoHeapPointers     

[dennygursky]

see similar idea in #28246

[crw]

Thanks for this submission. Although I cannot commit to having this PR reviewed at this time, we acknowledge your contribution and appreciate it!

[alisdair]

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.

[github-actions]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[dennygursky]

I noticed that in 0.13 and a particularly big configuration, apparently that didn't include Equal fix yet:

func (m Module) Equal(other Module) bool {
        return m.String() == other.String()
}

[apparentlymart]

@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 addrs.Module.String to produce map keys for each module in the configuration, over in #31302.

In that PR I was trying a different strategy for potentially improving it -- effectively, doing the String calls all up front at the start and comparing their results instead -- but as I noted over there I ran out of time before I was able to write up a benchmark to see how productive the optimization is, and now I see this PR I guess I should make sure to rebase against this change so I'm measuring against the optimized String implementation!

[github-actions]

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.