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

Add Size method on cache #41

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Add Size method on cache #41

merged 2 commits into from
Aug 20, 2024

Conversation

nlachfr
Copy link
Contributor

@nlachfr nlachfr commented Aug 7, 2024

Closes #40

@Yiling-J
Copy link
Owner

Yiling-J commented Aug 7, 2024

@nlachfr qlen is the window size of each shard(window is a very small buffer used in W-TinyLFU, you can read the paper for more detail), not the map size. Also don't forget to add unit test in cache_test.go, you can create a new test case or update TestCost.

@nlachfr
Copy link
Contributor Author

nlachfr commented Aug 7, 2024

My bad, I only had a quick look at internals, I thought it was the overall shard cost since it is updated during insertion
Is there an overall state somewhere which would avoid iterating over every entries ?

@Yiling-J
Copy link
Owner

Yiling-J commented Aug 7, 2024

@nlachfr The good news is yes, but there are a few things to consider. Firstly, it's not 100% accurate. Secondly, you may need some knowledge of TinyLFU. Theine has two parts: a shared map for storing data, and a policy for determining what to keep and remove when the cache is full.
The policy part is the tlfu.go file, you will find a TinyLFU struct includes an SLRU field. SLRU has two linked lists, each with a 'len' field that represents the cost sum of items in the list. However, it's not 100% accurate due to two reasons:

  1. There are several window buffers (qlen).
  2. The data map communicates with the policy using a buffer, meaning an item might exist in both the map and the buffer, but not in the policy.

@nlachfr
Copy link
Contributor Author

nlachfr commented Aug 7, 2024

OK, in that case, what do you think I should try to implement ? A function that loops over every item or one that return an approximate of the current state of the cache ? Or both ?

For a bit more context, I need this info in order to provide some metrics on cache usage. I have added wrappers around Set / Delete operations for collecting this, but it would be must simpler to have it available directly. In this scenario, having an approximate of size usage would be reasonable.

@Yiling-J
Copy link
Owner

Yiling-J commented Aug 7, 2024

@nlachfr I think approximate is good enough, but the method name should show that clearly, something like EstimatedSize. And to get that value, you can sum the qlen in all shards and two linked list len. Also add a race test: main goroutine randomly read/write cache and goroutine 2 call your new method periodically.

@nlachfr nlachfr force-pushed the main branch 3 times, most recently from dbc8450 to d4f2646 Compare August 19, 2024 10:32
@nlachfr nlachfr marked this pull request as draft August 19, 2024 10:46
@nlachfr nlachfr force-pushed the main branch 4 times, most recently from 7e227b3 to 1fb8827 Compare August 19, 2024 12:42
@nlachfr nlachfr marked this pull request as ready for review August 19, 2024 12:57
@nlachfr
Copy link
Contributor Author

nlachfr commented Aug 19, 2024

@Yiling-J So, this MR adds two methods on Cache and LoadingCache:

  • Size() int which returns the size of the cache (iterates over every entry)
  • EstimatedSize() int which returns an approximate of the cache size (sum of qlen + list len)

As you guessed, the EstimatedSize one cause some race issues, so I just changed the len int field of List[K, V] to an atomic.Int64

Copy link
Owner

@Yiling-J Yiling-J left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Could you update the README to include the new APIs?

cache.go Outdated Show resolved Hide resolved
internal/store.go Outdated Show resolved Hide resolved
internal/store.go Outdated Show resolved Hide resolved
@Yiling-J
Copy link
Owner

@nlachfr Looks good, Thanks for the contribution🙏

@Yiling-J Yiling-J merged commit 05dc84d into Yiling-J:main Aug 20, 2024
34 checks passed
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.

Add Size() function for getting used cache size
3 participants