Use ristretto to improve find performance#849
Conversation
This patch significantly improves `v.find` performance by using ristretto cache. Ristretto was chosen because it is a very performant, thread-safe cache that does not suffer form congestion or long GC pauses. ``` % go test -bench=. . goos: darwin goarch: amd64 pkg: github.com/spf13/viper BenchmarkGetBool-8 5431267 211 ns/op BenchmarkFind/cache=false-8 289495 4060 ns/op BenchmarkFind/cache=true-8 6980976 171 ns/op BenchmarkGet-8 6332091 193 ns/op BenchmarkGetBoolFromMap-8 235071828 5.09 ns/op PASS ```
|
Wow, this is super cool. I've been meaning to try ristretto for some time, but didn't have a chance. I'll take a look at the PR soon! Thanks @aeneasr |
|
Cool! We're running race tests on our fork and apparently there is one introduced by the cache. I'll look into what that's caused by, please hold the merge until then. :) |
|
Sure! How did you find the race? Because the race detector is enabled in Viper too: |
|
With a simple |
|
Ah, I know why this isn't caught here. Enabling I think viper will have a race condition if that test was added without this patch as well, as there are concurrent read/writes to the map. |
|
Well, for now this is what it is, but for the long term I really want to separate reading and writing in Viper. IMHO fetching config from various sources is what's Viper's strong suite, writing/setting config is just there, but not really the primary use case for Viper (or at least I think it shouldn't be). So for the long term I plan to keep the current API for compatibility reasons, at least for one more major version, but after that I'd like to see writing either gone from the core, or moved a different interface entirely. Writing and Get-ing are usually done by totally different consumers, so it should be okay. |
All of this works well until there's actually reloading (e.g. on config file changed) involved, which is where viper just breaks all over the place. We're using a RWMutex to prevent stuff like that but it obviously takes away some of the benefits you get from e.g. ristretto, although RLocks are kinda fine I guess.
As long as setting values for testing is supported I agree :) |
Yeah, Viper pretty much sucks at that at the moment. I'm looking at changing that in a next version.
Obviously. Manually setting values should be possible, but that's a different consumer of Viper. Ideally "getters" should only be able to use the getter API and not set values. That would probably get rid of lots of races. |
|
Sounds good :) Let me know if there’s anything else I can do in this patch to get it merged :) |
sagikazarmark
left a comment
There was a problem hiding this comment.
@aeneasr can you please resolve the conflict? I'd like to get this merged.
As discussed in another PR,
v.findperformance deteriorates as the config map grows and more config sources come in. We can significantly improve the performance ofv.findoperations (and thus allGet*operations) with a simple cache strategy.As you can see in the benchmark below, not using a cache needs 4060 ns/op for reading all keys of a large config file, while using a cache reduces that time to 171ns/op on average.
Ristretto was chosen because it is a very performant, thread-safe
cache that does not suffer form congestion or long GC pauses.