Closed kentquirk closed 11 months ago
I also tried the
new
algorithm but replaced a random value in the map instead of recreating it when it was full. This was not a winning strategy, so I didn't include it in the benchmark (but the code is there).
With new
+ EjectRandom disqualified, do you think it is worth trying the recommended sync
algorithm + EjectRandom to constrain map size?
I was going to say no, because sync.Map doesn't even have a Size() or Len() function, so we'd have to implement that separately. And then I just remembered that this cache is global to the process and thus it's possible to grow very large. I'm going to implement that and try it. Thanks for the poke.
Update -- I spent a while trying to limit the size of the caches in various ways, and all of them ended up causing significant performance bottlenecks when we do so safely (behind locks).
I no longer think that we should do this at all. More in the main issue
So I wanted to compare some ideas for improving this, and ended up spending more than an hour writing benchmarks and trying ideas.
All that is in here, but the summary is that we get a lot more predictable performance by using sync.Map instead of Go's native map. The alternative concurrent-map doesn't actually do better in this case.
The function
getPrefixedFieldNameSync
in this PR is the best-performing choice, IMO.Below are the results of the benchmark.
orig
is the original code (prepending app unconditionally),new
is Mike's implementation,sync
is using sync.Map,conc
is using concurrent-map.f50
means that there are 50 different field names,g10
means there are 10 concurrent goroutines accessing the cache. Sof2000-g300
is the most stressful.I also tried the
new
algorithm but replaced a random value in the map instead of recreating it when it was full. This was not a winning strategy, so I didn't include it in the benchmark (but the code is there).My recommendation is that we replace the current design in the PR with the contents of getPrefixedFieldNameSync and call it a day.