google / guava

Google core libraries for Java
Apache License 2.0
50.14k stars 10.89k forks source link

Cache: complex keys causing very low hit rate and large number of evictions #1666

Closed gissuebot closed 9 years ago

gissuebot commented 9 years ago

Original issue created by jacek99 on 2014-02-11 at 05:13 PM


We have a complex key consisting of a POJO with 4 string fields, an Integer and a DateMidnight (from Joda Time). 2 out of the 4 String fields can be null at any time.

It seems with a simple key (e.g. a single String) the cache is very fast,

However, with a complex key like this the hit rate and number of evictions goes up dramatically, e.g.:

Hit rate: 33.30 % Miss rate: 66.69 % Average load penalty: 5.44 ms Eviction count: 15002 Request count: 24402 Load exception rate: 0.00 %

if we convert all these keys into a single String using a StringBuilder is starts being super fast again with a hit rate of ~99.9%.

The problem with that is in the loading method we have to parse the String to break out the original 6 pieces that made it. Not preferable for an application with near real-time SLAS.

Set up:

cache = CacheBuilder.newBuilder() .maximumSize(100_000) .expireAfterWrite(5, TimeUnit.MINUTES) .concurrencyLevel(100) .recordStats() .build( new CacheLoader<ItineraryResultCacheKey, ItineraryResult>() {                             @Override                             public ItineraryResult load(ItineraryResultCacheKey key) throws Exception {

.... cache loading code....

                    });
}

On OpenJDK 7. Tested this with both 14.01 and 16.01, same results.

I've tried multiple things (using Date with all hours/minutes/seconds set to 0 to ensure key equality), converting all values to a single string and coding the equals() and hashCode() to use that instead of the other fields.

To no avail.

It seems that if you put in a complex POJO as a key, something goes wrong in the Cache internals and we consistently get a hit rate of ~33%, never more. Very weird that it is always so consistent.

I admit, it is a bizarre issue. Any suggestions on what we may be doing wrong are welcome.

gissuebot commented 9 years ago

Original comment posted by jacek99 on 2014-02-11 at 05:15 PM


I've also played around with the concurrencyLevel, anywhere from 1 to exact number of threads in the HTTP server. Made no discernible difference either.

gissuebot commented 9 years ago

Original comment posted by kak@google.com on 2014-02-11 at 05:18 PM


Curious what your equals/hashCode methods look like on your ItineraryResultCacheKey POJO...

gissuebot commented 9 years ago

Original comment posted by jacek99 on 2014-02-11 at 05:30 PM


At first I had a default one generated via Lombok. That is how we found the original issues.

Since that was my first suspicion I overrode it manually to just use the aforementioned single String representation of all the parts of the POJO, e.g.

private String id;

@Override
public boolean equals(Object obj) {
    if (obj instanceof ItineraryResultCacheKey) {
        return this.id.equals(((ItineraryResultCacheKey) obj).getId());
    } else {
        return false;
    }
}

@Override
public int hashCode() {
    return this.id.hashCode();
}

where "id" gets created in the Constructor by appending all the 6 fields into a single string using a StringBuilder (with "" put if string is null and the explicit year/month/day integers put into it for the date part (so no actual Date.toString() where we could get millsecond/second differences).,

Also, we found this in simple perf testing where we are basically sending the same 10 requests over and over (using multi-mechanize) so the range of keys being sent is actually very small. It's not real-life data.

Is there some part of code you could recommend for me to trace or debug manually (a particular Class/method)?

Unfortunately due to the proprietary nature of the application I am very limited in what type of code I can post.

gissuebot commented 9 years ago

Original comment posted by jacek99 on 2014-02-11 at 05:56 PM


I hang my head in shame. Equals()/hashCode() are not the problem.

We had previously existing code in a totally different section of the app that was actually mutating this key during multiple parts of the processing (one of the fields). This was done as part of a naive attempt to reduce GC pressure by reusing object instances (instead of instantiating them multiple times with minor variations).

Once I removed that and made it properly instantiate a new instance every time every started running properly.

Please close as invalid.

gissuebot commented 9 years ago

Original comment posted by lowasser@google.com on 2014-02-11 at 05:57 PM


(No comment entered for this change.)


Status: Invalid

gajduk commented 3 years ago

Original comment posted by jacek99 on 2014-02-11 at 05:56 PM

I hang my head in shame. Equals()/hashCode() are not the problem.

We had previously existing code in a totally different section of the app that was actually mutating this key during multiple parts of the processing (one of the fields). This was done as part of a naive attempt to reduce GC pressure by reusing object instances (instead of instantiating them multiple times with minor variations).

Once I removed that and made it properly instantiate a new instance every time every started running properly.

Please close as invalid.

Thanks for sharing this, helped me with a similar issue. Praise!