Factor out the index cache implementation, add ristretto #849
Conversation
GiedriusS
changed the title
|
Nice! I did not look at this much, but definitely huge area for improvments here. Can we think of / run some benchmarks to assess if this change is valuable? It makes sense in theory (: |
|
Yes, I will rework this into making it a (hidden) option and will make some automatic benchmarks, just like with the other ones that we have already. |
|
@GiedriusS have you seen? https://blog.dgraph.io/post/caching-in-go/ |
|
I haven't seen that article but I am aware of TinyLFU. It seems to me that it is the new best LRU cache policy. I started this PR with 2Q since there is a mature implementation of it. On the other hand, I have only seen one experimental implementation of TinyLFU in Go however it lacks some very important features that are mentioned in that article as well. So I guess we should wait until they will implement it or I will try it if I will have the time. Or maybe someone else wants to do that (: |
|
https://github.com/dgraph-io/ristretto seems like that new library is here :) hopefully the implementation will come soon |
|
The library has been finally implemented so now we could resume the work on this! 🎉 |
GiedriusS
changed the title
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
GiedriusS
changed the title
|
@bwplotka do you think we should make this configurable? Myself I would lean towards a one time switch since the performance should be miles ahead with this library 😃 |
|
Unfortunately but ristretto has some problems:
|
|
@GiedriusS Collision checking is being finalized. We're investigating the memory leak issue; however, it appears to only occur under rare circumstances. Thanks for checking out Ristretto! |
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Encode the key's type inside of the value itself in the tinylfu case. This is needed so that we would not lose any information in the metrics about the different types of keys. 1 byte for this seems like a good enough trade-off for me. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Add an implementation for Purge() which calls Clear(). Fix KeyData() return value to return `false` which is the true value. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Two uint64 hashes are used. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
|
@bwplotka how do you think the performance of this could be tested? WDYT about the PR in general? I have been running it for a bit on one node and it seems to work well. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Indeed this is not relevant anymore since these things should probably be pushed to some outer provider like |
SimpleLRU is a nice and simple algorithm for storing items in a cache. However, it is not the most efficient and it doesn't work well in our use-case under high-pressure situations since users tend to execute ad-hoc queries and, in addition, refresh Grafana dashboards quite often which leads to the same items being loaded over and over again.
I have factored out the index cache implementation into another file. This lets us change it to another one on a whim. In addition, I have added an implementation which uses the
ristrettolibrary, which uses TinyLFU underneath - a relatively new algorithm which espouses very good surprising performance (see the benchmarks here: https://github.com/dgraph-io/ristretto).This current implementation kind of works.
Verification:
go test -v.