Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.