x/tools/go/types/typeutil: gut Hasher

[adonovan]

Thinking about https://github.com/golang/go/issues/67161, I wanted to know whether the the new API should include Hasher. It complicates the API because it turns reads (lookups in typeutil.Map) into writes (memoization of hash values). Is it an effective optimization, or does it increase memory contention (and potentially the need for locking)?

I gutted its implementation, removing both memoization maps, and simplifying the implementation of Hasher.hashPtr to just reflect.ValueOf(ptr).Pointer() (which assumes a non-moving GC) and Hasher.hashTypeParam to a hash of t.Index(), making Hasher a stateless empty struct. Then I ran BenchmarkRTA, modified to analyze all of gopls, not just net/http.

The results:

before
200805483 ns/op
203918225
206312775
207155800
209421367
219960417

after
201373042
203830142
204368642
208747200
206170792
208873431

N   min     max     sum     mean        stddev
6   2.00805e+08 2.1996e+08  1.24757e+09 2.07929e+08 6.58822e+06 // before
6   2.01373e+08 2.08873e+08 1.23336e+09 2.05561e+08 2.94797e+06 // after

It is slightly faster and has lower variance. Whatever problems a stateful Hasher once solved, we have no further need for it.

Let's make it an empty struct, and disregard it when considering #67161.

@timothy-king @findleyr

[gopherbot]

Change https://go.dev/cl/612496 mentions this issue: go/types/typeutil: make Hasher stateless

[timothy-king]

54670 was accepted. Once it is available, can we use that?

[findleyr]

@timothy-king do you mean to use Comparable for hashPtr, rather than relying on a non-moving GC?

[adonovan]

@timothy-king do you mean to use Comparable for hashPtr, rather than relying on a non-moving GC?

maphash.Comparable does the same thing as our code (reflect.Value.Pointer). The only difference is that, being std code, it is morally permitted to assume a non-moving GC, but once the updated https://github.com/golang/go/issues/67161 lands, we can use types.Hash, which will have equal moral license.