Cache statistics #34

uncle-lv · 2023-10-12T13:13:47Z

Usage

1.Turn on statistics collection

Set stats to True to turn on statistics collection.

cache = Cache(stats=True)

2.Get statistics snapshot

Call get_stats() to get a Stats object, what represents a snapshot of statistics.

stats = cache.get_stats()

3.Get statistics

stats.hits
stats.misses
stats.total
stats.access
stats.hit_rate
stats.miss_rate
stats.eviction_rate

coveralls · 2023-10-12T13:16:32Z

coverage: 100.0%. remained the same when pulling e8d8f01 on uncle-lv:statistics into d9c6d8b on dgilland:master.

uncle-lv · 2023-10-12T13:19:29Z

This pr is for #32

dgilland · 2023-10-20T00:59:26Z

src/cacheout/cache.py

+                if value == default:
+                    self._stats_counter.record_misses(1)


This won't work when self.default is set and the value was set to it (i.e. key is set but happens to be the default value).

I'm wondering if this would be easier to do within _get() since it will be more explicit when the key isn't set.

The problem is has() get_ttl() _add() calls _has() then _has() calls _get(). Should these calls be counted?

Yes, ok to count those.

dgilland · 2023-10-20T01:08:51Z

src/cacheout/cache.py

+        if self._stats_counter:
+            self._stats_counter.reset()


I don't think the stats should necessarily be cleared when clearing the cache (could be a use-case for wiping the keys but keeping the stats going). Instead, we can have an explicit method to reset the stats (e.g. reset_stats()).

dgilland · 2023-10-20T01:16:28Z

src/cacheout/cache.py

+            if self._stats_counter:
+                self._stats_counter.record_total(1)


Not sure we need to manually inc/dec the total key count since that can be easily determined with len(self). I'm worried we may miss counting somewhere which would throw off the total.

dgilland · 2023-10-20T01:18:08Z

src/cacheout/cache.py

+        with self._lock:
+            return Stats(counter=self._stats_counter) if self._stats_counter else None


Avoid locking if not needed:

Suggested change

with self._lock:

return Stats(counter=self._stats_counter) if self._stats_counter else None

if not self._stats_counter:

return None

with self._lock:

return Stats(counter=self._stats_counter)

dgilland · 2023-10-20T01:18:46Z

src/cacheout/cache.py

@@ -85,12 +88,14 @@ def __init__(
        timer: t.Callable[[], T_TTL] = time.time,
        default: t.Any = None,
        on_delete: t.Optional[t.Callable[[t.Hashable, t.Any, RemovalCause], None]] = None,
+        stats: bool = False,


Suggested change

stats: bool = False,

enable_stats: bool = False,

dgilland · 2023-10-20T01:49:43Z

src/cacheout/stats.py

+        return self._total
+
+    @property
+    def access(self) -> int:


Suggested change

def access(self) -> int:

def accesses(self) -> int:

dgilland · 2023-10-20T01:51:55Z

src/cacheout/stats.py

+        return self._misses
+
+    @property
+    def total(self) -> int:


Just total might be a little ambiguous. Maybe total_entries instead.

dgilland · 2023-10-20T01:54:12Z

src/cacheout/stats.py

+
+        Return 1.0 when ``access_count`` == 0.
+        """
+        return 1.0 if self.access == 0 else self._hits / self.access


Prefer without the ternary so that it's a little easier to grok:

Suggested change

return 1.0 if self.access == 0 else self._hits / self.access

if self.accesses == 0:

return 1.0

return self.hits / self.access

Also, stay consistent with either self._* attribute or without the leading underscore for all.

dgilland · 2023-10-20T01:54:26Z

src/cacheout/stats.py

+
+        Return 0.0 when ``access_count`` == 0.
+        """
+        return 0.0 if self.access == 0 else self._misses / self.access


Prefer no ternary. See comment on hit_rate().

dgilland · 2023-10-20T01:54:31Z

src/cacheout/stats.py

+
+        Return 1.0 when ``access_count`` == 0.
+        """
+        return 1.0 if self.access == 0 else self._evictions / self.access


Prefer no ternary. See comment on hit_rate().

uncle-lv · 2023-10-20T08:20:26Z

I almost agree.

But if we move hits/misses count into _get(), not only get() calls will be counted but also other methods(has() get_ttl _add()) which call _get() will be counted. It maybe taint statistics.

uncle-lv · 2023-10-25T13:11:31Z

Usage

1.Turn on statistics collection

~~Set enable_stats to True to turn on statistics collection.~~

cache = Cache(enable_stats=True)

1.Enable statistics collection

Call cache.stats.enable() to enable statistics collection.

cache = Cache()
cache.stats.enable()

2.Control statistics

Disable statistics.

cache.stats.disable()

⚠️Statistics will be cleared if you call disable()!

Enable statistics.

cache.stats.enable()

Pause statistics.

cache.stats.pause()

Resume statistics.

cache.stats.resume()

Clear statistics.

cache.stats.reset()

3.Get statistics

Call cache.stats.info() to get a statistics snapshot.

stats = cache.stats.info()

Get details.

stats.hits
stats.misses
stats.total_entries
stats.accesses
stats.hit_rate
stats.miss_rate
stats.eviction_rate

dgilland · 2023-10-30T15:53:12Z

src/cacheout/cache.py

+    @property
+    def stats(self) -> StatsTracker:
+        return self._stats_tracker


Drop the @property and just have self.stats.

It's to prevent the user from modifying the reference of _stats_tracker(make it read-only).

I'd like for it to to be overridable.

Could you offer me a case? It seems one Cache should hold only one and same StatsTracker in one lifetime.

I haven't quite decided what the exact approach will be but don't want it behind a property.

dgilland · 2023-10-30T15:54:08Z

src/cacheout/cache.py

-            return self._get(key, default=default)
+            value = self._get(key, default=default)
+
+            return value


This change doesn't seem necessary; it's the same either way (maybe leftover from refactor?).

Sorry! I forgot it.

dgilland · 2023-10-30T15:58:26Z

src/cacheout/cache.py

    ):
        self.maxsize = maxsize
        self.ttl = ttl
        self.timer = timer
        self.default = default
        self.on_delete = on_delete
+        self._enable_stats = enable_stats


Looks like StatsTracker is already managing the enablement and checking whether stats are enabled or not before incrementing/decrementing; can have Cache call the stats methods unconditionally instead. Also, don't need this attribute since we can do something like if enable_stats: self.stats.enable().

I'll remove it.

dgilland · 2023-10-30T16:01:21Z

src/cacheout/stats.py

+        self._hit_count = 0
+        self._miss_count = 0
+        self._evicted_count = 0
+        self._total_count = 0


Prefer not to define the individual counter attributes on both StatsTracker and Stats. Can just have a reference to a Stats instance on StatsTracker that's modified instead.

dgilland · 2023-10-30T16:05:29Z

src/cacheout/stats.py

+        self._enabled = True
+        self._paused = False
+
+    def _inc_hits(self, count: int) -> None:


Let's go with public methods for these in case users want to override them/add their own.

dgilland · 2023-10-30T16:12:06Z

src/cacheout/stats.py

+    def __init__(self, hits: int, misses: int, evictions: int, total: int) -> None:
+        self._hits = hits
+        self._misses = misses
+        self._evictions = evictions
+        self._total = total


Match naming with def total_entries:

Suggested change

def __init__(self, hits: int, misses: int, evictions: int, total: int) -> None:

self._hits = hits

self._misses = misses

self._evictions = evictions

self._total = total

def __init__(self, hits: int, misses: int, evictions: int, total_entries: int) -> None:

self._hits = hits

self._misses = misses

self._evictions = evictions

self._total_entries = total_entries

dgilland · 2023-10-30T16:14:52Z

src/cacheout/cache.py

+        if self._enable_stats:
+            self._stats_tracker._total_count = len(self)


Wondering if instead of doing this, we pass the Cache instance to StatsTracker and then do return Stats(..., total_entries=len(self._cache)) so that the total is always accurate and we don't have to worry about syncing up cache key addition/removal events.

It will lead to a circular import if StatsTracker holds the Cache instance.

circular import

Because of cross-typing? That can be bypassed by checking typing.TYPE_CHECKING and only importing when True in stats module.

This blog as a simple example: https://adamj.eu/tech/2021/05/13/python-type-hints-how-to-fix-circular-imports/

Got it! Thanks.

dgilland

Awesome work! Thanks for taking the time to add this!

feat: cache statistics

d46137c

dgilland requested changes Oct 20, 2023

View reviewed changes

refactor: redesign APIs

c01939b

perf: remove unnecessary locks

d53cc5c

dgilland requested changes Oct 30, 2023

View reviewed changes

refactor stats

e8d8f01

dgilland approved these changes Oct 31, 2023

View reviewed changes

dgilland merged commit 53d55c6 into dgilland:master Oct 31, 2023
12 checks passed

uncle-lv deleted the statistics branch November 1, 2023 10:03

uncle-lv mentioned this pull request Nov 4, 2023

Cache statistics #32

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache statistics #34

Cache statistics #34

uncle-lv commented Oct 12, 2023

coveralls commented Oct 12, 2023 •

edited

Loading

uncle-lv commented Oct 12, 2023

dgilland Oct 20, 2023

uncle-lv Oct 20, 2023

dgilland Oct 21, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

dgilland Oct 20, 2023

uncle-lv commented Oct 20, 2023

uncle-lv commented Oct 25, 2023 •

edited

Loading

dgilland Oct 30, 2023

uncle-lv Oct 31, 2023 •

edited

Loading

dgilland Oct 31, 2023

uncle-lv Oct 31, 2023

dgilland Oct 31, 2023

dgilland Oct 30, 2023

uncle-lv Oct 31, 2023

dgilland Oct 30, 2023

uncle-lv Oct 31, 2023

dgilland Oct 30, 2023

dgilland Oct 30, 2023

dgilland Oct 30, 2023

dgilland Oct 30, 2023

uncle-lv Oct 31, 2023

dgilland Oct 31, 2023

uncle-lv Oct 31, 2023

dgilland left a comment

		with self._lock:
		return Stats(counter=self._stats_counter) if self._stats_counter else None

		if self._enable_stats:
		self._stats_tracker._total_count = len(self)

Cache statistics #34

Cache statistics #34

Conversation

uncle-lv commented Oct 12, 2023

Usage

1.Turn on statistics collection

2.Get statistics snapshot

3.Get statistics

coveralls commented Oct 12, 2023 • edited Loading

uncle-lv commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uncle-lv commented Oct 20, 2023

uncle-lv commented Oct 25, 2023 • edited Loading

Usage

1.Turn on statistics collection

1.Enable statistics collection

2.Control statistics

3.Get statistics

Choose a reason for hiding this comment

uncle-lv Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgilland left a comment

Choose a reason for hiding this comment

coveralls commented Oct 12, 2023 •

edited

Loading

uncle-lv commented Oct 25, 2023 •

edited

Loading

uncle-lv Oct 31, 2023 •

edited

Loading