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

initial ttl support #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kirillDanshin
Copy link

Signed-off-by: Kirill Danshin kirill@danshin.pro

c.ttlmu.Lock()
for k, deadBySec := range c.ttl {
if !ttlValid(deadBySec) {
delete(c.ttl, k)
Copy link

Choose a reason for hiding this comment

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

Deleting from the map will cause increasing of internal overflow buckets inside a map, so it can produce huge gc pauses. Better to use a slice here.

Copy link
Author

@kirillDanshin kirillDanshin May 31, 2019

Choose a reason for hiding this comment

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

using slices tho will cause O(n) search in the locked section (in {S,G}etWithTTL methods)

func (c *Cache) ttlGCRoutine() {
go func() {
for {
c.ttlmu.Lock()
Copy link

Choose a reason for hiding this comment

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

Does this loop cause huge pauses?

Copy link
Author

Choose a reason for hiding this comment

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

it keeps map relatively small, so in practice it works pretty fast

Copy link

Choose a reason for hiding this comment

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

Is it possible to store ttl inside of buckets?

Copy link
Author

Choose a reason for hiding this comment

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

initially I was going to do exactly that, but it caused a lot of memcopies and increased implementation complexity. current implementation is simplified, allowing to at least introduce such features and optimize further if needed.

fastcache_ttl_timing_test.go Outdated Show resolved Hide resolved
Signed-off-by: Kirill Danshin <kirill@danshin.pro>
@kirillDanshin
Copy link
Author

Hmm, tests failed on travis, but works on all my environments. Checking out.

@valyala valyala mentioned this pull request Jun 4, 2019
@detailyang
Copy link

Hello
Any progress? @kirillDanshin

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.

3 participants