use atomics for metrics #91
Conversation
|
I think you are right and this might be a good change. Before we merge this, have you done any benchmarking here? I am asking because of this bit:
|
|
| func (c *Cache[K, V]) Metrics() (m Metrics) { | ||
| // I imagine on most architectures this is equivalent to | ||
| // simply writing: return c.metrics | ||
| m.Insertions = atomic.LoadUint64(&c.metrics.Insertions) |
There was a problem hiding this comment.
Maybe something like this would be more expressive, but it's just question of taste, ofc:
return Metrics{
Insertions: atomic.LoadUint64(&c.metrics.Insertions),
Hits: atomic.LoadUint64(&c.metrics.Hits),
Misses: atomic.LoadUint64(&c.metrics.Misses),
Evictions: atomic.LoadUint64(&c.metrics.Evictions),
}
There was a problem hiding this comment.
That would be my suggestion as well.
|
|
||
| return c.metrics | ||
| func (c *Cache[K, V]) Metrics() (m Metrics) { | ||
| // I imagine on most architectures this is equivalent to |
There was a problem hiding this comment.
Just curious, what do you mean by this comment? Afaik it's not the best idea to do just return c.metrics on any platforms.
There was a problem hiding this comment.
I submitted this PR almost 2 months ago and no longer remember the details of the code. What I vaguely remember is that it seemed like there may be unnecessary atomic operations / locks in various places. Granted, some architectures may require atomic loads where others don't. However, I don't have time to re-read all of the code again now. I'm guessing that what I was indicating in that comment is that the atomic loads are of no value if they're already guarded by a read lock -- if they are, then returning a struct by value (plus or minus any golang quirks) is likely pretty equivalent to creating a new struct and copying the values over explicitly in your code.
The main thing is that unless you're worried about partial reads of partially written words, there's not really much point in atomic reads of individual values in the structs; and separately, if you're reading the individual values atomically, there's not much point to locking the struct to read it since those values are updated independently of each other. That is, there isn't case where value A and value B inside the struct must be updated atomically together. Therefore nobody should care if you read A then read B and an update happened in between.
Anyway, all of the above caveat emptor because it's been too long for me to remember the code. I just vaguely remember that there may be some thread safety overkill going on.
|
I also noticed this potential improvement, glad to see opened PR for this (: @swithek do you have any concerns regarding this improvement or regarding this particular PR? |
Having seen the benchmark results, I'm not entirely convinced this would be a good change. It doesn't look like an improvement at all. Perhaps the banchmark tests should be different to properly test this? |
AFAIK metricsMu is not really performing any useful function over and above the likely more efficient atomic integer operations available on most modern CPUs. In the Metric() return function, the metricsMu served to ensure all the counters are returned as of an instant in time when no other counter is incremented relative to other counters, but since the write locks are only held for the duration of an increment of a single element of the structure, even that has no utility (i.e., there are never increments of two counters at once while holding the lock across both increments, which is the only thing the read lock would protect). Finally, the atomic loads probably collapse to simple reads on many CPUs, with the atomic load possibly only forcing cpu cache coherency (i.e., you can probably just write Metrics(){return c.metrics} and be just fine).