-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add Len() to common interface #44
Conversation
- implement Len for simple cache
@@ -23,6 +23,8 @@ type Interface[K comparable, V any] interface { | |||
Keys() []K | |||
// Delete deletes the item with provided key from the cache. | |||
Delete(key K) | |||
// Len returns the number of items in the cache. | |||
Len() int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add method of the Cache struct because this is breaking change.
I think good like below:
Len() int { return len(c.Keys()) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that this my change below:
func (c *Cache[K, V]) Len() int {
c.mu.Lock()
defer c.mu.Unlock()
return c.cache.Len()
}
should be changed to:
func (c *Cache[K, V]) Len() int {
return len(c.Keys())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@norri Yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think performance for c.cache.Len()
is better than len(c.Keys())
because of for example sorting is not needed and all policies support Len()
. However I can make the change if you think that it's better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you measure the performance?
And also please implement all cache policies not just simple 🙏
I think this discussion is required all cache policies results.
I think performance for c.cache.Len() is better than len(c.Keys())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to test the performance. All other policies implement already Len()
so no changes needed for those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added benchmark test for both implementations and using just Len() is quite match faster:
$ go test -benchmem -bench BenchmarkLenWithKeys ./policy/simple
goos: darwin
goarch: amd64
pkg: github.com/Code-Hex/go-generics-cache/policy/simple
cpu: Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz
BenchmarkLenWithKeys-8 3777514 310.7 ns/op 104 B/op 3 allocs/op
PASS
ok github.com/Code-Hex/go-generics-cache/policy/simple 1.640s
$ go test -benchmem -bench BenchmarkJustLen ./policy/simple
goos: darwin
goarch: amd64
pkg: github.com/Code-Hex/go-generics-cache/policy/simple
cpu: Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz
BenchmarkJustLen-8 1000000000 0.3075 ns/op 0 B/op 0 allocs/op
PASS
Len() method is useful in common interface and is simple to implement.
Fixes #43