chore: use otter as the primary cache implementation and get rid of alternative implementations#3112
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3112 +/- ##
==========================================
+ Coverage 75.66% 75.69% +0.03%
==========================================
Files 505 502 -3
Lines 62111 61989 -122
==========================================
- Hits 46989 46914 -75
+ Misses 11701 11660 -41
+ Partials 3421 3415 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 3a395e3 | Previous: f7fda69 | Ratio |
|---|---|---|---|
BenchmarkDatastoreDriver/cockroachdb-overlap-static/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) |
34826186 ns/op 175010 B/op 20202 allocs/op |
6959651 ns/op 172710 B/op 20195 allocs/op |
5.00 |
BenchmarkDatastoreDriver/cockroachdb-overlap-static/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) - ns/op |
34826186 ns/op |
6959651 ns/op |
5.00 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) |
32581099 ns/op 174731 B/op 20200 allocs/op |
7535757 ns/op 172724 B/op 20195 allocs/op |
4.32 |
BenchmarkDatastoreDriver/cockroachdb-overlap-insecure/TestTuple/SnapshotReverseRead (github.com/authzed/spicedb/internal/datastore/benchmark) - ns/op |
32581099 ns/op |
7535757 ns/op |
4.32 |
This comment was automatically generated by workflow using github-action-benchmark.
39dad92 to
d8c6070
Compare
| // DatastoreProxyTestCache returns a cache used for testing. | ||
| func DatastoreProxyTestCache(t testing.TB) cache.Cache[cache.StringKey, CacheEntry] { | ||
| cache, err := cache.NewStandardCache[cache.StringKey, CacheEntry](&cache.Config{ | ||
| NumCounters: 1000, |
There was a problem hiding this comment.
This configuration value isn't used by otter, so I deprecated it and removed places where it's set.
| } | ||
| } | ||
|
|
||
| func TestComputeOnlyStableHash(t *testing.T) { |
There was a problem hiding this comment.
This is related to removing ristretto - there was a process-specific hash that was a part of its key computation, but that isn't used by the other cache implementations, so I got rid of its computation and got rid of logic and tests around that.
|
|
||
| func (dck DispatchCacheKey) KeyString() string { | ||
| firstBytes := binary.AppendUvarint(make([]byte, 0, 8), dck.stableSum) | ||
| secondBytes := binary.AppendUvarint(make([]byte, 0, 8), dck.processSpecificSum) |
There was a problem hiding this comment.
This value is always 0 unless it's been computed in the ristretto logic.
| // AsUInt64s returns the cache key in the form of two uint64's. This method returns uint64s created | ||
| // from two distinct hashing algorithms, which should make the risk of key overlap incredibly | ||
| // unlikely. | ||
| func (dck DispatchCacheKey) AsUInt64s() (uint64, uint64) { |
There was a problem hiding this comment.
This was only used in the ristretto implementation.
| if h.computeOption == computeBothHashes { | ||
| h.processSpecificSum = runMemHash(h.processSpecificSum, []byte(value)) | ||
| } | ||
| } | ||
|
|
||
| // From: https://github.com/outcaste-io/ristretto/blob/master/z/rtutil.go | ||
| type stringStruct struct { | ||
| str unsafe.Pointer | ||
| len int | ||
| } | ||
|
|
||
| //go:noescape | ||
| //go:linkname memhash runtime.memhash | ||
| func memhash(p unsafe.Pointer, h, s uintptr) uintptr | ||
|
|
||
| func runMemHash(seed uint64, data []byte) uint64 { | ||
| ss := (*stringStruct)(unsafe.Pointer(&data)) | ||
| return uint64(memhash(ss.str, uintptr(seed), uintptr(ss.len))) //nolint:gosec | ||
| } |
There was a problem hiding this comment.
This is the part that I removed - this is the only place where computeOption is actually referenced, and the processSpecificSum is only referenced in the key logic in the ristretto implementation.
There was a problem hiding this comment.
These changes are just flattening the test and testing Otter directly.
| // TODO: check name here | ||
| return NewOtterCache[K, V]("", config) |
There was a problem hiding this comment.
Should there be a name here? I wasn't sure.
There was a problem hiding this comment.
func mustRegisterCache(name string, c withMetrics) {
if _, loaded := caches.LoadOrStore(name, c); loaded {
panic("two caches with the same name: " + name)
}
}
func unregisterCache(name string) {
caches.Delete(name)
}how does that work if the key is empty?
why do we have _?
should we not panic if _ is actually false? so many questions..
|
|
||
| // CompleteCache translates the CLI cache config into a cache config. | ||
| func CompleteCache[K cache.KeyString, V any](cc *CacheConfig) (cache.Cache[K, V], error) { | ||
| if !cc.Enabled || cc.MaxCost == "" || cc.MaxCost == "0%" || cc.NumCounters == 0 { |
There was a problem hiding this comment.
NumCounters is deprecated, so we don't switch on it anymore.
| return nil, errors.New("could not cast max cost to int64") | ||
| } | ||
|
|
||
| if cc.CacheKindForTesting != "" { |
There was a problem hiding this comment.
Otter is now the standard, so we get rid of this switching.
| flags.BoolVar(&config.Enabled, flagPrefix+"-enabled", defaults.Enabled, "enable caching of "+flagDescription) | ||
|
|
||
| // Hidden flags. | ||
| flags.StringVar(&config.CacheKindForTesting, flagPrefix+"-kind-for-testing", defaults.CacheKindForTesting, "choose a different kind of cache, for testing") |
There was a problem hiding this comment.
This flag was already hidden, so I'm comfortable with deleting it.
5b87fe4 to
2691223
Compare
|
Hmm... seems Otter is still incompatible with wasm. I'll reinstate the wasm build flags. |
2691223 to
0a785e8
Compare
0a785e8 to
5a2bf18
Compare
|
Okay, this is ready for review. |
| } | ||
| } | ||
|
|
||
| if cc.Metrics { |
There was a problem hiding this comment.
why do we have this if here? the config that gets sent in the constructor is exactly the same, and so is the underlying code.
Looking at the implementation, it looks like we're always emmitting metrics 🤔 😬 (which is fine by me, i don't know why we expose --cache-metrics as a flag?)
|
An LLM note about why otter isn't compatible with WASM:
|
c04ccb9 to
3a395e3
Compare
Description
We've had three alternative implementations for a while now, with
ristrettobeing the one that is available by default, with two other implementations behind flags. We've load tested this internally and it appears that all three of the implementations are interchangeable from a performance perspective.otteris the most modern of the three and it supports generics, which means that we don't have to muck around with calculating our own keys.This PR makes otter the primary cache, removes the other implementations, and simplifies where possible.
Changes
Will annotate.
Testing
Review. See that benchmarks are the same.