Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use atomics for metrics #91

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

use atomics for metrics #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5841df5
use atomics for metrics
Nov 24, 2022
a19390c
missing file
Nov 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 15 additions & 22 deletions cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"fmt"
"sync"
"sync/atomic"
"time"

"golang.org/x/sync/singleflight"
Expand Down Expand Up @@ -37,8 +38,7 @@ type Cache[K comparable, V any] struct {
timerCh chan time.Duration
}

metricsMu sync.RWMutex
metrics Metrics
metrics Metrics

events struct {
insertion struct {
Expand Down Expand Up @@ -152,9 +152,7 @@ func (c *Cache[K, V]) set(key K, value V, ttl time.Duration) *Item[K, V] {
c.items.values[key] = elem
c.updateExpirations(true, elem)

c.metricsMu.Lock()
c.metrics.Insertions++
c.metricsMu.Unlock()
atomic.AddUint64(&c.metrics.Insertions, 1)

c.events.insertion.mu.RLock()
for _, fn := range c.events.insertion.fns {
Expand Down Expand Up @@ -196,9 +194,7 @@ func (c *Cache[K, V]) get(key K, touch bool) *list.Element {
// Not concurrently safe.
func (c *Cache[K, V]) evict(reason EvictionReason, elems ...*list.Element) {
if len(elems) > 0 {
c.metricsMu.Lock()
c.metrics.Evictions += uint64(len(elems))
c.metricsMu.Unlock()
atomic.AddUint64(&c.metrics.Evictions, uint64(len(elems)))

c.events.eviction.mu.RLock()
for i := range elems {
Expand All @@ -216,9 +212,7 @@ func (c *Cache[K, V]) evict(reason EvictionReason, elems ...*list.Element) {
return
}

c.metricsMu.Lock()
c.metrics.Evictions += uint64(len(c.items.values))
c.metricsMu.Unlock()
atomic.AddUint64(&c.metrics.Evictions, uint64(len(c.items.values)))

c.events.eviction.mu.RLock()
for _, elem := range c.items.values {
Expand Down Expand Up @@ -262,9 +256,7 @@ func (c *Cache[K, V]) Get(key K, opts ...Option[K, V]) *Item[K, V] {
c.items.mu.Unlock()

if elem == nil {
c.metricsMu.Lock()
c.metrics.Misses++
c.metricsMu.Unlock()
atomic.AddUint64(&c.metrics.Misses, 1)

if getOpts.loader != nil {
return getOpts.loader.Load(c, key)
Expand All @@ -273,9 +265,7 @@ func (c *Cache[K, V]) Get(key K, opts ...Option[K, V]) *Item[K, V] {
return nil
}

c.metricsMu.Lock()
c.metrics.Hits++
c.metricsMu.Unlock()
atomic.AddUint64(&c.metrics.Hits, 1)

return elem.Value.(*Item[K, V])
}
Expand Down Expand Up @@ -371,11 +361,14 @@ func (c *Cache[K, V]) Items() map[K]*Item[K, V] {
}

// Metrics returns the metrics of the cache.
func (c *Cache[K, V]) Metrics() Metrics {
c.metricsMu.RLock()
defer c.metricsMu.RUnlock()

return c.metrics
func (c *Cache[K, V]) Metrics() (m Metrics) {
// I imagine on most architectures this is equivalent to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@aathan aathan Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// simply writing: return c.metrics
m.Insertions = atomic.LoadUint64(&c.metrics.Insertions)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be my suggestion as well.

m.Hits = atomic.LoadUint64(&c.metrics.Hits)
m.Misses = atomic.LoadUint64(&c.metrics.Misses)
m.Evictions = atomic.LoadUint64(&c.metrics.Evictions)
return
}

// Start starts an automatic cleanup process that
Expand Down
5 changes: 2 additions & 3 deletions cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"fmt"
"sync"
"sync/atomic"
"testing"
"time"

Expand Down Expand Up @@ -730,9 +731,7 @@ func Test_Cache_Start(t *testing.T) {
go func() {
assert.Equal(t, EvictionReasonExpired, r)

cache.metricsMu.RLock()
v := cache.metrics.Evictions
cache.metricsMu.RUnlock()
v := atomic.LoadUint64(&cache.metrics.Evictions)

switch v {
case 1:
Expand Down