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.

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

Fix cache key eviction #231

Merged
merged 2 commits into from
May 24, 2023
Merged

Fix cache key eviction #231

merged 2 commits into from
May 24, 2023

Conversation

novln
Copy link
Contributor

@novln novln commented May 24, 2023

This pull request tries to fix the issue reported by #230

I don't know if it wasn't working from the start or something changed internally in Go runtime, but using internal bytebuffer to perform string concatenation without allocation is not working as intended.

On the internal memory cache at a given time (it seems deterministic) the key get evicted resetting the value.
https://github.com/ulule/limiter/blob/master/drivers/store/memory/cache.go#L191-L194

I don't have the energy and motivation to dig this far inside go runtime to understand why.
(So if you want to contribute to the project and you're reading this, don't hesitate)
But I'm wondering if the key is not garbage collected inside the sync.Map (or something along those lines)
resulting in this behavior because I've check the produced slice and string and it doesn't seem to differ (content, length, capacity).

With this fix, we now have a performance penalty since we do an allocation for the key.

On the previous version:

➤ go test -run=XXX -bench=MemoryStore -benchmem -benchtime=30s .
goos: linux
goarch: amd64
pkg: github.com/ulule/limiter/v3/drivers/store/memory
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
BenchmarkMemoryStoreSequentialAccess-8   	220604082	      165.1 ns/op	      0 B/op	      0 allocs/op
BenchmarkMemoryStoreConcurrentAccess-8   	205891585	      171.8 ns/op	      0 B/op	      0 allocs/op
PASS
ok  	github.com/ulule/limiter/v3/drivers/store/memory	105.946s

On the newer version with this fix:

➤ go test -run=XXX -bench=MemoryStore -benchmem -benchtime=30s .
goos: linux
goarch: amd64
pkg: github.com/ulule/limiter/v3/drivers/store/memory
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
BenchmarkMemoryStoreSequentialAccess-8   	200741551	      187.1 ns/op	     48 B/op	      1 allocs/op
BenchmarkMemoryStoreConcurrentAccess-8   	148522920	      248.3 ns/op	     48 B/op	      1 allocs/op
PASS
ok  	github.com/ulule/limiter/v3/drivers/store/memory	117.020s

@novln novln merged commit 6b0fc1a into master May 24, 2023
@novln novln deleted the fix-cache-key-eviction branch May 30, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant