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

GC will remove incorrect item, espically for multiple reset sall cache case #96

Open
ceclinux opened this issue Dec 6, 2024 · 1 comment

Comments

@ceclinux
Copy link

ceclinux commented Dec 6, 2024

GC will remove incorrect items, especially for multiple resets in small cache cases.

Please check the following test case for the expected cache value.Get("1").value should be "900" for each iteration. However, sometimes cache.Get("1") will return nil.

func Test_SmallCacheGetGC(t *testing.T) {
	for i := 0; i < 10; i++ {
		cache := New(Configure[string]().MaxSize(2).ItemsToPrune(1).DeleteBuffer(20).PromoteBuffer(1).GetsPerPromote(2))
		cache.Set("1", "1000", 10*time.Minute)
		cache.Set("2", "1000", 10*time.Minute)
		cache.Set("3", "1000", 10*time.Minute)
		cache.Set("1", "900", 10*time.Minute)

		time.Sleep(100 * time.Millisecond)
		assert.Equal(t, cache.Get("1").value, "900")
	}
}

The reason is the bucket updating of items and list updating of items are not synchronized. Let's say '1 2 3' has already been added to the bucket, and "1" has been updated to "900". However, these updated changes have not been sent to channels. Meanwhile, the worker's goroutine is trying to gc the '1' as the cache size is 2. This can be wrong because calling c.bucket(item.key).delete(item.key) will delete the newly updated entry for "1" in the bucket.

Not sure how to fix this. It would be nice if you give me some idea :)

@karlseguin
Copy link
Owner

I'm not sure it is fixable.

I think the entire async approach that ccache uses should be re-evaluated. It has a few negative consequences, such as this issue and the fact that the cache can grow much larger than the specified maximum under heavy insert load.

Does having a single "worker" goroutine really improve performance? If it doesn't, then ccache probably shouldn't be used (or, if it is used, should be changed to simply lock the recency list on every get/set operation directly).

I think more people use ccache because of its fairly unique features. Things like the layered cache and and cache item tracking. Not because of its novel caching algorithm or implementation. Keeping the same functionality while removing the async worker, is 100% doable. I also think the implementation would be much simpler.

If it does bring meaningful performance benefits, then I think some of these are just going to be tradeoff users are going to have to put up with. I'm not spending much time doing Go these days, but if I was going to invest time in making ccache better, I would start by benchmarking it against a simpler and more traditional implementation where the linked list is made thread-safe. Hard to pick a direction without first knowing that. I am willing to look at it, but maybe not until the new year. If you're interested in doing more research, I'd love to know your findings.

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

No branches or pull requests

2 participants