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

Cache eviction for CHMCache, or another default cache #29

Closed
joewreschnig opened this issue Apr 12, 2017 · 6 comments
Closed

Cache eviction for CHMCache, or another default cache #29

joewreschnig opened this issue Apr 12, 2017 · 6 comments

Comments

@joewreschnig
Copy link

Right now CHMCache has no eviction policy - when it fills up, it never changes. But our GeoIP lookups are not just clustered (so benefiting from a cache) but also time-varying (benefiting from the cache changing over time). E.g. from around UTC 0:00 to 6:00, we switch from mostly-European to mostly-North-American IPs.

I understand that CHMCache is supposed to be mostly an example of how to write a cache, but we're dealing with applications written in JRuby, and that makes writing our own cache a little more painful and a lot slower.

Would it be possible to offer a default cache with an eviction policy?

@oschwald
Copy link
Member

As you mention, the CHMCache is primarily an example of how to implement a cache, but it does work ok for most basic used cases as things like countries, continents, and major cities end up in the cache. We would be open to alternate cache implementations or enhancements to CHMCache as long as they didn't pull in new dependencies (e.g., no Apache Commons).

@cameronkerrnz
Copy link

cameronkerrnz commented Mar 24, 2018

I'm not a Java programmer, but this looks promising, and standard to Java 7 at least: http://chriswu.me/blog/a-lru-cache-in-10-lines-of-java/ implements a LRU Cache based on a LinkedHashMap in a mode (selected by constructor) to be ordered by access time.

https://docs.oracle.com/javase/7/docs/api/java/util/LinkedHashMap.html#LinkedHashMap(int,%20float,%20boolean)

Worth noting from the docs:

Note that this implementation is not synchronized. If multiple threads access a linked hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally. This is typically accomplished by synchronizing on some object that naturally encapsulates the map. If no such object exists, the map should be "wrapped" using the Collections.synchronizedMap method. This is best done at creation time, to prevent accidental unsynchronized access to the map:

Map m = Collections.synchronizedMap(new LinkedHashMap(...));

A structural modification is any operation that adds or deletes one or more mappings or, in the case of access-ordered linked hash maps, affects iteration order. In insertion-ordered linked hash maps, merely changing the value associated with a key that is already contained in the map is not a structural modification. In access-ordered linked hash maps, merely querying the map with get is a structural modification.)

@re-thc
Copy link

re-thc commented May 30, 2018

Why not use something like https://github.com/ben-manes/caffeine ?

@oschwald
Copy link
Member

We do not want to pull in a bunch of dependencies that many users will never use. Caffeine, in particular, also requires Java 8, while we still support 7. If you would like to use Caffeine, it is very easy to implement your own cache using CHMCache as an example.

@ben-manes
Copy link

Maybe consider ConcurrentLinkedHashMap if you want a tiny dependency, or an older (JDK7) Guava if already used?

@oschwald
Copy link
Member

We tried implementing an LRU cache and the performance was strictly worse when looking up random IPs than CHMCache, at least for the default cache size and larger sizes. The reason is that even with random lookups, CHMCache ends up caching most of the relevant data, e.g., the country maps, and it has an extremely low overhead. Managing eviction, especially in a thread-safe manner, ends up being almost as costly as just decoding the data from the database.

Given this, I am going to close this issue without prejudice . Future PRs with alternate caching implementations will be considered on their merits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants