GetWithExpirationUpdate - atomic implementation#126
GetWithExpirationUpdate - atomic implementation#126paddlesteamer wants to merge 3 commits intopatrickmn:masterfrom
Conversation
…-new-map-design GetWithExpirationUpdate method and items are converted to pointers
|
I just did a quick test, but it looks like this change is quite a big performance regression: The pointers also make the GC work harder, so there's probably some indirect performance regression there too (which may already be accounted for in the above benchmarks, didn't look in-depth). A simpler way to fix this would be to just lock everything for the entire function instead of switching from |
|
Hi again, I removed the |
|
I included new benchmarks at the bottom, as you can see it doesn't make that much of a difference. The basic issue is that locks are not free, for example take this simple benchmark: // BenchmarkMain-2 46259625 24.3 ns/op
func BenchmarkMain(b *testing.B) {
var s struct{ sync.Mutex }
for n := 0; n < b.N; n++ {
s.Lock()
s.Unlock()
}
}So that's the minimum amount of performance regression that can be expected for operations that add a new lock. The other issue is that pointers are not free either; I included a benchmark below which changes just For reference, here's what I did in my fork: // Touch replaces the expiry of a key and returns the current value.
func (c *cache) Touch(k string, d time.Duration) (interface{}, bool) {
if d == DefaultExpiration {
d = c.defaultExpiration
}
c.mu.Lock()
defer c.mu.Unlock()
item, ok := c.items[k]
if !ok {
return nil, false
}
item.Expiration = time.Now().Add(d).UnixNano()
c.items[k] = item
return item.Object, true
}And the benchmark for this compared to your version: This is a simple benchmark of course; I tried to make a benchmark which made your version look better in cases where you're also doing other stuff in goroutines, for example: // BenchmarkTouch-2 6014732 200 ns/op My Touch
// BenchmarkTouch-2 4213928 284 ns/op Your GetWithExpirationUpdate
func BenchmarkTouch(b *testing.B) {
tc := New(DefaultExpiration, 0)
d := 5 * time.Second
tc.Set("foo", 0, DefaultExpiration)
go func() {
for {
tc.Get("foo")
time.Sleep(1 * time.Millisecond)
}
}()
b.ResetTimer()
for i := 0; i < b.N; i++ {
tc.GetWithExpirationUpdate("foo", d)
}
}I tried a bunch of different variations of this, but I couldn't come up with anything where your version had better performance. I don't know ... maybe I missed something – benchmarking realistic workloads of hard – but from what I can see just doing For your latest commit, here's the comparison with the original PR: And here's the comparison with master: master vs. just changing |
|
Thank you for your long, detailed response. I just had time to do benchmarks myself and you're right, it is obvious that using a single mutex is faster. Converting the map value type to pointer was also a bad idea and cause regression. So, I will switch to your version of Thanks again and have a good day! |
|
You're welcome @paddlesteamer; I mostly just wanted to know this for my own reasons :-) I put up the fork over here the other day by the way: https://github.com/zgoat/zcache – I'm not 100% decided yet on some things I added so I didn't release a v1 yet, but all the existing stuff should be compatible, FYI :-) |
2 participants
This PR is fixed version of #96. Main changes are:
cache.itemsare converted frommap[string]Itemtomap[string]*Item. I needed to do it because, inGetWithExpirationUpdate, it is the only way to modify theExpirationfield of anItem. The other way around (re-setting the item) needs a write lock, therefore blocks all reads/writes toitems. Not convenient for 'cache-get's.Itemhas its ownRWLock. This way, we don't need a write lock inGetWithExpirationUpdate.Supersedes #125