Skip to content

Commit

Permalink
Don't cache metadata store disambiguations. Fixes BUKKIT-3841
Browse files Browse the repository at this point in the history
The metadata system generates unique keys for metadata entries based on
the subject metadata is being applied to and the name of the metadata
being applied. It was assumed this would be an expensive operation so a
cache was put in place to ensure this was done as little as possible.

In reality this cache only has a benefit when you have a hit rate above
~90% and is otherwise much slower. As the implementation of the cache is
a hashmap of hashmaps it also uses a significant amount of memory which
is not worth it even for the performance increase with a high hit rate.

This commit simply removes the cache which results in speedups for most
cases and large memory savings.
  • Loading branch information
crast authored and amaranth committed Apr 4, 2013
1 parent 771c5bd commit b7a9cd9
Showing 1 changed file with 4 additions and 28 deletions.
32 changes: 4 additions & 28 deletions src/main/java/org/bukkit/metadata/MetadataStoreBase.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

public abstract class MetadataStoreBase<T> {
private Map<String, List<MetadataValue>> metadataMap = new HashMap<String, List<MetadataValue>>();
private WeakHashMap<T, Map<String, String>> disambiguationCache = new WeakHashMap<T, Map<String, String>>();

/**
* Adds a metadata value to an object. Each metadata value is owned by a specific{@link Plugin}.
Expand All @@ -28,7 +27,7 @@ public abstract class MetadataStoreBase<T> {
public synchronized void setMetadata(T subject, String metadataKey, MetadataValue newMetadataValue) {
Validate.notNull(newMetadataValue, "Value cannot be null");
Validate.notNull(newMetadataValue.getOwningPlugin(), "Plugin cannot be null");
String key = cachedDisambiguate(subject, metadataKey);
String key = disambiguate(subject, metadataKey);
if (!metadataMap.containsKey(key)) {
metadataMap.put(key, new ArrayList<MetadataValue>());
}
Expand All @@ -55,7 +54,7 @@ public synchronized void setMetadata(T subject, String metadataKey, MetadataValu
* @see MetadataStore#getMetadata(Object, String)
*/
public synchronized List<MetadataValue> getMetadata(T subject, String metadataKey) {
String key = cachedDisambiguate(subject, metadataKey);
String key = disambiguate(subject, metadataKey);
if (metadataMap.containsKey(key)) {
return Collections.unmodifiableList(metadataMap.get(key));
} else {
Expand All @@ -71,7 +70,7 @@ public synchronized List<MetadataValue> getMetadata(T subject, String metadataKe
* @return the existence of the metadataKey within subject.
*/
public synchronized boolean hasMetadata(T subject, String metadataKey) {
String key = cachedDisambiguate(subject, metadataKey);
String key = disambiguate(subject, metadataKey);
return metadataMap.containsKey(key);
}

Expand All @@ -86,7 +85,7 @@ public synchronized boolean hasMetadata(T subject, String metadataKey) {
*/
public synchronized void removeMetadata(T subject, String metadataKey, Plugin owningPlugin) {
Validate.notNull(owningPlugin, "Plugin cannot be null");
String key = cachedDisambiguate(subject, metadataKey);
String key = disambiguate(subject, metadataKey);
List<MetadataValue> metadataList = metadataMap.get(key);
if (metadataList == null) return;
for (int i = 0; i < metadataList.size(); i++) {
Expand Down Expand Up @@ -118,29 +117,6 @@ public synchronized void invalidateAll(Plugin owningPlugin) {
}
}

/**
* Caches the results of calls to {@link MetadataStoreBase#disambiguate(Object, String)} in a {@link WeakHashMap}. Doing so maintains a
* <a href="http://www.codeinstructions.com/2008/09/weakhashmap-is-not-cache-understanding.html">canonical list</a>
* of disambiguation strings for objects in memory. When those objects are garbage collected, the disambiguation string
* in the list is aggressively garbage collected as well.
*
* @param subject The object for which this key is being generated.
* @param metadataKey The name identifying the metadata value.
* @return a unique metadata key for the given subject.
*/
private String cachedDisambiguate(T subject, String metadataKey) {
if (disambiguationCache.containsKey(subject) && disambiguationCache.get(subject).containsKey(metadataKey)) {
return disambiguationCache.get(subject).get(metadataKey);
} else {
if (!disambiguationCache.containsKey(subject)) {
disambiguationCache.put(subject, new HashMap<String, String>());
}
String disambiguation = disambiguate(subject, metadataKey);
disambiguationCache.get(subject).put(metadataKey, disambiguation);
return disambiguation;
}
}

/**
* Creates a unique name for the object receiving metadata by combining unique data from the subject with a metadataKey.
* The name created must be globally unique for the given object and any two equivalent objects must generate the
Expand Down

0 comments on commit b7a9cd9

Please sign in to comment.