ben-manes / concurrentlinkedhashmap

A ConcurrentLinkedHashMap for Java
Apache License 2.0
470 stars 113 forks source link

Re-evaluate weight api (nice defaults; zero weights) #18

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
A Weigher allows values to consume more than one slot in the cache, so that 
users have additional flexibility than only bounding by the number of key-value 
pairs. This allows multi-map or memory-bound caches, for example.

This was implemented, but I left the semantics around collections unresolved as 
it wasn't clear what the expected behavior should be if empty. This may be used 
for negative caching to avoid lookups when the value is known to not exist. If 
a zero weight is allowed, is there a concern of the cache growing unbounded due 
to too much negative caching and no eviction being triggered? Ideally 
soft-references would always be used as a fall-back if memory becomes tight, so 
zero weights wouldn't cause an issue.

This was left unresolved since I wasn't sure what was least surprising and 
expected user feedback to drive a resolution. The choices seem to be:
 * Allow weighted value of zero
 * Use Math.max(list.size(), 1) in Weighers
 * Use list.size() + 1 in Weighers

I'm leaning towards allowing zero.

---
Issue raised by Grails: http://jira.codehaus.org/browse/GRAILS-6622

Original issue reported on code.google.com by Ben.Manes@gmail.com on 2 Sep 2010 at 6:07

GoogleCodeExporter commented 9 years ago
Even though zero values for empty collections might help with the negative 
cacheing, I still feel that it would be more realistic for the collection 
itself to be weighted. After all, it is still an object, and still takes up 
heap space. If you had a huge map filled with empty lists, it should still have 
weight.

Hence, my vote would go for list.size() + 1

Original comment by vermeule...@gmail.com on 3 Sep 2010 at 2:23

GoogleCodeExporter commented 9 years ago
Thinking about this, I could argue that the current behavior is correct. The 
intent was to allow someone to implement a Google Guava Multimap abstraction.

A Multimap replaces all null cases with empty collections and never stores 
empty collections in the backing map. Thus, there would never be a value with 
weight zero.

This failure is partially due to not having a Multimap abstraction, which would 
remove the need for the Weighers class, and that the negative caching is being 
used. Thus, null and empty collection have distinctly different meanings.

Perhaps the best resolution would be to implement a Multimap cache and remove 
the Weighers utility class. Grails would still need a custom weigher, but the 
behavior wouldn't be surprising.

Original comment by Ben.Manes@gmail.com on 15 Sep 2010 at 5:37

GoogleCodeExporter commented 9 years ago
Clarified JavaDoc in r475 to warn clients of the behavior if the weight is 
zero. I've concluded that the default behavior is the most appropriate, with 
the action taken the responsibility of the client. If a zero-weight is not 
allowed, then it should be decorated in a Multimap form to automatically remove 
the value instead of updating it. If a zero weight is allowed, then a custom 
weigher should be used to account for the negative caching. As the default 
behavior can't assume what is expected, the JavaDoc tries to clarify the 
failure case so clients can determine if it may occur in their usage.

Original comment by Ben.Manes@gmail.com on 1 Nov 2010 at 2:33