From fa4499b65f1a03cbf386cc6641141023ac94f9ef Mon Sep 17 00:00:00 2001 From: md_5 Date: Sat, 23 Mar 2013 10:30:53 +1100 Subject: [PATCH] Add a TON of metadata speed / memory fixes and improvements, courtesy of @crast --- ...ions-from-LazyMetadataValue-into-abs.patch | 258 ++++++++++++++++++ Bukkit-Patches/0007-Clean-up-whitespace.patch | 42 +++ ...tially-more-comprehensive-unit-tests.patch | 119 ++++++++ ...der-leakage-in-metadata-system.-Fixe.patch | 139 ++++++++++ 4 files changed, 558 insertions(+) create mode 100644 Bukkit-Patches/0006-Refactor-conversions-from-LazyMetadataValue-into-abs.patch create mode 100644 Bukkit-Patches/0007-Clean-up-whitespace.patch create mode 100644 Bukkit-Patches/0008-Substantially-more-comprehensive-unit-tests.patch create mode 100644 Bukkit-Patches/0009-Prevent-classloader-leakage-in-metadata-system.-Fixe.patch diff --git a/Bukkit-Patches/0006-Refactor-conversions-from-LazyMetadataValue-into-abs.patch b/Bukkit-Patches/0006-Refactor-conversions-from-LazyMetadataValue-into-abs.patch new file mode 100644 index 0000000..0015784 --- /dev/null +++ b/Bukkit-Patches/0006-Refactor-conversions-from-LazyMetadataValue-into-abs.patch @@ -0,0 +1,258 @@ +From 0d2be83dcf857b4b29c304bfb4b5d67dde23a13c Mon Sep 17 00:00:00 2001 +From: crast +Date: Sat, 16 Feb 2013 14:33:24 -0700 +Subject: [PATCH] Refactor conversions from LazyMetadataValue into abstract + base class + +Implementing MetadataValue interface is significant work due to having +to provide a large amount of conversion stub method. This adds a new +optional abstract base class to aid in implementation. + +Includes comprehensive unit tests including a sample adapter class, and +all existing metadata tests pass. +--- + .../org/bukkit/metadata/LazyMetadataValue.java | 60 +---------------- + .../org/bukkit/metadata/MetadataValueAdapter.java | 77 ++++++++++++++++++++++ + .../bukkit/metadata/MetadataValueAdapterTest.java | 44 +++++++++++++ + 3 files changed, 123 insertions(+), 58 deletions(-) + create mode 100644 src/main/java/org/bukkit/metadata/MetadataValueAdapter.java + create mode 100644 src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java + +diff --git a/src/main/java/org/bukkit/metadata/LazyMetadataValue.java b/src/main/java/org/bukkit/metadata/LazyMetadataValue.java +index cc0ba50..9b66da9 100644 +--- a/src/main/java/org/bukkit/metadata/LazyMetadataValue.java ++++ b/src/main/java/org/bukkit/metadata/LazyMetadataValue.java +@@ -5,7 +5,6 @@ import java.util.concurrent.Callable; + + import org.apache.commons.lang.Validate; + import org.bukkit.plugin.Plugin; +-import org.bukkit.util.NumberConversions; + + /** + * The LazyMetadataValue class implements a type of metadata that is not computed until another plugin asks for it. +@@ -14,11 +13,10 @@ import org.bukkit.util.NumberConversions; + * or invalidated at the individual or plugin level. Once invalidated, the LazyMetadataValue will recompute its value + * when asked. + */ +-public class LazyMetadataValue implements MetadataValue { ++public class LazyMetadataValue extends MetadataValueAdapter implements MetadataValue { + private Callable lazyValue; + private CacheStrategy cacheStrategy; + private SoftReference internalValue = new SoftReference(null); +- private Plugin owningPlugin; + private static final Object ACTUALLY_NULL = new Object(); + + /** +@@ -39,19 +37,14 @@ public class LazyMetadataValue implements MetadataValue { + * @param lazyValue the lazy value assigned to this metadata value. + */ + public LazyMetadataValue(Plugin owningPlugin, CacheStrategy cacheStrategy, Callable lazyValue) { +- Validate.notNull(owningPlugin, "owningPlugin cannot be null"); ++ super(owningPlugin); + Validate.notNull(cacheStrategy, "cacheStrategy cannot be null"); + Validate.notNull(lazyValue, "lazyValue cannot be null"); + + this.lazyValue = lazyValue; +- this.owningPlugin = owningPlugin; + this.cacheStrategy = cacheStrategy; + } + +- public Plugin getOwningPlugin() { +- return owningPlugin; +- } +- + public Object value() { + eval(); + Object value = internalValue.get(); +@@ -61,55 +54,6 @@ public class LazyMetadataValue implements MetadataValue { + return value; + } + +- public int asInt() { +- return NumberConversions.toInt(value()); +- } +- +- public float asFloat() { +- return NumberConversions.toFloat(value()); +- } +- +- public double asDouble() { +- return NumberConversions.toDouble(value()); +- } +- +- public long asLong() { +- return NumberConversions.toLong(value()); +- } +- +- public short asShort() { +- return NumberConversions.toShort(value()); +- } +- +- public byte asByte() { +- return NumberConversions.toByte(value()); +- } +- +- public boolean asBoolean() { +- Object value = value(); +- if (value instanceof Boolean) { +- return (Boolean) value; +- } +- +- if (value instanceof Number) { +- return ((Number) value).intValue() != 0; +- } +- +- if (value instanceof String) { +- return Boolean.parseBoolean((String) value); +- } +- +- return value != null; +- } +- +- public String asString() { +- Object value = value(); +- +- if (value == null) { +- return ""; +- } +- return value.toString(); +- } + + /** + * Lazily evaluates the value of this metadata item. +diff --git a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +new file mode 100644 +index 0000000..9ec7e61 +--- /dev/null ++++ b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +@@ -0,0 +1,77 @@ ++package org.bukkit.metadata; ++ ++import org.apache.commons.lang.Validate; ++import org.bukkit.plugin.Plugin; ++import org.bukkit.util.NumberConversions; ++ ++/** ++ * Optional base class for facilitating MetadataValue implementations. ++ * ++ * This provides all the conversion functions for MetadataValue ++ * so that writing an implementation of MetadataValue is as simple ++ * as implementing value() and invalidate() ++ * ++ */ ++public abstract class MetadataValueAdapter implements MetadataValue { ++ protected final Plugin owningPlugin; ++ ++ protected MetadataValueAdapter(Plugin owningPlugin) { ++ Validate.notNull(owningPlugin, "owningPlugin cannot be null"); ++ this.owningPlugin = owningPlugin; ++ } ++ ++ public Plugin getOwningPlugin() { ++ return owningPlugin; ++ } ++ ++ public int asInt() { ++ return NumberConversions.toInt(value()); ++ } ++ ++ public float asFloat() { ++ return NumberConversions.toFloat(value()); ++ } ++ ++ public double asDouble() { ++ return NumberConversions.toDouble(value()); ++ } ++ ++ public long asLong() { ++ return NumberConversions.toLong(value()); ++ } ++ ++ public short asShort() { ++ return NumberConversions.toShort(value()); ++ } ++ ++ public byte asByte() { ++ return NumberConversions.toByte(value()); ++ } ++ ++ public boolean asBoolean() { ++ Object value = value(); ++ if (value instanceof Boolean) { ++ return (Boolean) value; ++ } ++ ++ if (value instanceof Number) { ++ return ((Number) value).intValue() != 0; ++ } ++ ++ if (value instanceof String) { ++ return Boolean.parseBoolean((String) value); ++ } ++ ++ return value != null; ++ } ++ ++ public String asString() { ++ Object value = value(); ++ ++ if (value == null) { ++ return ""; ++ } ++ return value.toString(); ++ } ++ ++} +diff --git a/src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java b/src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java +new file mode 100644 +index 0000000..5ae7df4 +--- /dev/null ++++ b/src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java +@@ -0,0 +1,44 @@ ++package org.bukkit.metadata; ++ ++import static org.junit.Assert.assertEquals; ++ ++import org.bukkit.plugin.Plugin; ++import org.bukkit.plugin.TestPlugin; ++import org.junit.Test; ++ ++public class MetadataValueAdapterTest { ++ private TestPlugin plugin = new TestPlugin("x"); ++ ++ @Test ++ public void testIncrementingAdapter() { ++ IncrementingMetaValue mv = new IncrementingMetaValue(plugin); ++ // check getOwningPlugin ++ assertEquals(mv.getOwningPlugin(), this.plugin); ++ ++ // check the various value-making methods ++ assertEquals(mv.asInt(), 1); ++ assertEquals(mv.asLong(), 2L); ++ assertEquals(mv.asFloat(), 3.0, 0.001); ++ assertEquals(mv.asByte(), 4); ++ assertEquals(mv.asDouble(), 5.0, 0.001); ++ assertEquals(mv.asShort(), 6); ++ assertEquals(mv.asString(), "7"); ++ } ++ ++ /** Silly Metadata implementation that increments every time value() is called */ ++ class IncrementingMetaValue extends MetadataValueAdapter { ++ private int internalValue = 0; ++ ++ protected IncrementingMetaValue(Plugin owningPlugin) { ++ super(owningPlugin); ++ } ++ ++ public Object value() { ++ return ++internalValue; ++ } ++ ++ public void invalidate() { ++ internalValue = 0; ++ } ++ } ++} +-- +1.8.1-rc2 + diff --git a/Bukkit-Patches/0007-Clean-up-whitespace.patch b/Bukkit-Patches/0007-Clean-up-whitespace.patch new file mode 100644 index 0000000..e63c72a --- /dev/null +++ b/Bukkit-Patches/0007-Clean-up-whitespace.patch @@ -0,0 +1,42 @@ +From c072f0a7250ce40674b9bfbcf8fe52fd28b25716 Mon Sep 17 00:00:00 2001 +From: crast +Date: Wed, 20 Mar 2013 15:24:12 -0600 +Subject: [PATCH] Clean up whitespace. + +--- + src/main/java/org/bukkit/metadata/LazyMetadataValue.java | 1 - + src/main/java/org/bukkit/metadata/MetadataValueAdapter.java | 6 +++--- + 2 files changed, 3 insertions(+), 4 deletions(-) + +diff --git a/src/main/java/org/bukkit/metadata/LazyMetadataValue.java b/src/main/java/org/bukkit/metadata/LazyMetadataValue.java +index 9b66da9..57fdc50 100644 +--- a/src/main/java/org/bukkit/metadata/LazyMetadataValue.java ++++ b/src/main/java/org/bukkit/metadata/LazyMetadataValue.java +@@ -54,7 +54,6 @@ public class LazyMetadataValue extends MetadataValueAdapter implements MetadataV + return value; + } + +- + /** + * Lazily evaluates the value of this metadata item. + * +diff --git a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +index 9ec7e61..354b6dc 100644 +--- a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java ++++ b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +@@ -6,9 +6,9 @@ import org.bukkit.util.NumberConversions; + + /** + * Optional base class for facilitating MetadataValue implementations. +- * +- * This provides all the conversion functions for MetadataValue +- * so that writing an implementation of MetadataValue is as simple ++ * ++ * This provides all the conversion functions for MetadataValue ++ * so that writing an implementation of MetadataValue is as simple + * as implementing value() and invalidate() + * + */ +-- +1.8.1-rc2 + diff --git a/Bukkit-Patches/0008-Substantially-more-comprehensive-unit-tests.patch b/Bukkit-Patches/0008-Substantially-more-comprehensive-unit-tests.patch new file mode 100644 index 0000000..adb55de --- /dev/null +++ b/Bukkit-Patches/0008-Substantially-more-comprehensive-unit-tests.patch @@ -0,0 +1,119 @@ +From 634c75e521140bc33af968f8b5249123eee5aebc Mon Sep 17 00:00:00 2001 +From: crast +Date: Wed, 20 Mar 2013 15:59:03 -0600 +Subject: [PATCH] Substantially more comprehensive unit tests. + +Check all the interesting implementation details in metadatavalues +which were never tested before, as well as making sure we document +things thoroughly. +--- + .../org/bukkit/metadata/MetadataValueAdapter.java | 2 +- + .../bukkit/metadata/MetadataValueAdapterTest.java | 73 +++++++++++++++++++--- + 2 files changed, 64 insertions(+), 11 deletions(-) + +diff --git a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +index 354b6dc..c4b8b39 100644 +--- a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java ++++ b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +@@ -9,7 +9,7 @@ import org.bukkit.util.NumberConversions; + * + * This provides all the conversion functions for MetadataValue + * so that writing an implementation of MetadataValue is as simple +- * as implementing value() and invalidate() ++ * as implementing value() and invalidate(). + * + */ + public abstract class MetadataValueAdapter implements MetadataValue { +diff --git a/src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java b/src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java +index 5ae7df4..7d8a17f 100644 +--- a/src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java ++++ b/src/test/java/org/bukkit/metadata/MetadataValueAdapterTest.java +@@ -10,22 +10,75 @@ public class MetadataValueAdapterTest { + private TestPlugin plugin = new TestPlugin("x"); + + @Test +- public void testIncrementingAdapter() { ++ public void testAdapterBasics() { + IncrementingMetaValue mv = new IncrementingMetaValue(plugin); + // check getOwningPlugin + assertEquals(mv.getOwningPlugin(), this.plugin); + +- // check the various value-making methods +- assertEquals(mv.asInt(), 1); +- assertEquals(mv.asLong(), 2L); +- assertEquals(mv.asFloat(), 3.0, 0.001); +- assertEquals(mv.asByte(), 4); +- assertEquals(mv.asDouble(), 5.0, 0.001); +- assertEquals(mv.asShort(), 6); +- assertEquals(mv.asString(), "7"); ++ // Check value-getting and invalidation. ++ assertEquals(new Integer(1), mv.value()); ++ assertEquals(new Integer(2), mv.value()); ++ mv.invalidate(); ++ assertEquals(new Integer(1), mv.value()); + } + +- /** Silly Metadata implementation that increments every time value() is called */ ++ @Test ++ public void testAdapterConversions() { ++ IncrementingMetaValue mv = new IncrementingMetaValue(plugin); ++ ++ assertEquals(1, mv.asInt()); ++ assertEquals(2L, mv.asLong()); ++ assertEquals(3.0, mv.asFloat(), 0.001); ++ assertEquals(4, mv.asByte()); ++ assertEquals(5.0, mv.asDouble(), 0.001); ++ assertEquals(6, mv.asShort()); ++ assertEquals("7", mv.asString()); ++ } ++ ++ /** Boolean conversion is non-trivial, we want to test it thoroughly. */ ++ @Test ++ public void testBooleanConversion() { ++ // null is False. ++ assertEquals(false, simpleValue(null).asBoolean()); ++ ++ // String to boolean. ++ assertEquals(true, simpleValue("True").asBoolean()); ++ assertEquals(true, simpleValue("TRUE").asBoolean()); ++ assertEquals(false, simpleValue("false").asBoolean()); ++ ++ // Number to boolean. ++ assertEquals(true, simpleValue(1).asBoolean()); ++ assertEquals(true, simpleValue(5.0).asBoolean()); ++ assertEquals(false, simpleValue(0).asBoolean()); ++ assertEquals(false, simpleValue(0.1).asBoolean()); ++ ++ // Boolean as boolean, of course. ++ assertEquals(true, simpleValue(Boolean.TRUE).asBoolean()); ++ assertEquals(false, simpleValue(Boolean.FALSE).asBoolean()); ++ ++ // any object that is not null and not a Boolean, String, or Number is true. ++ assertEquals(true, simpleValue(new Object()).asBoolean()); ++ } ++ ++ /** Test String conversions return an empty string when given null. */ ++ @Test ++ public void testStringConversionNull() { ++ assertEquals("", simpleValue(null).asString()); ++ } ++ ++ /** Get a fixed value MetadataValue. */ ++ private MetadataValue simpleValue(Object value) { ++ return new FixedMetadataValue(plugin, value); ++ } ++ ++ /** ++ * A sample non-trivial MetadataValueAdapter implementation. ++ * ++ * The rationale for implementing an incrementing value is to have a value ++ * which changes with every call to value(). This is important for testing ++ * because we want to make sure all the tested conversions are calling the ++ * value() method exactly once and no caching is going on. ++ */ + class IncrementingMetaValue extends MetadataValueAdapter { + private int internalValue = 0; + +-- +1.8.1-rc2 + diff --git a/Bukkit-Patches/0009-Prevent-classloader-leakage-in-metadata-system.-Fixe.patch b/Bukkit-Patches/0009-Prevent-classloader-leakage-in-metadata-system.-Fixe.patch new file mode 100644 index 0000000..aac6f6b --- /dev/null +++ b/Bukkit-Patches/0009-Prevent-classloader-leakage-in-metadata-system.-Fixe.patch @@ -0,0 +1,139 @@ +From 7080e3e4d864249328686c7d04ffba43881a36ed Mon Sep 17 00:00:00 2001 +From: crast +Date: Thu, 21 Mar 2013 18:13:20 -0600 +Subject: [PATCH] Prevent classloader leakage in metadata system. Fixes + Bukkit-3854 + +Metadata values keep strong reference to plugins, and they're not +cleared out when plugins are unloaded. This system adds weak reference +logic to allow these values to fall out of scope. In addition we get +some operations turning to O(1) "for free." +--- + .../org/bukkit/metadata/MetadataStoreBase.java | 48 ++++++++-------------- + .../org/bukkit/metadata/MetadataValueAdapter.java | 8 ++-- + 2 files changed, 23 insertions(+), 33 deletions(-) + +diff --git a/src/main/java/org/bukkit/metadata/MetadataStoreBase.java b/src/main/java/org/bukkit/metadata/MetadataStoreBase.java +index 7febbd4..8b7aabc 100644 +--- a/src/main/java/org/bukkit/metadata/MetadataStoreBase.java ++++ b/src/main/java/org/bukkit/metadata/MetadataStoreBase.java +@@ -6,7 +6,7 @@ import org.bukkit.plugin.Plugin; + import java.util.*; + + public abstract class MetadataStoreBase { +- private Map> metadataMap = new HashMap>(); ++ private Map> metadataMap = new HashMap>(); + private WeakHashMap> disambiguationCache = new WeakHashMap>(); + + /** +@@ -26,23 +26,16 @@ public abstract class MetadataStoreBase { + * @throws IllegalArgumentException If value is null, or the owning plugin is null + */ + public synchronized void setMetadata(T subject, String metadataKey, MetadataValue newMetadataValue) { ++ Plugin owningPlugin = newMetadataValue.getOwningPlugin(); + Validate.notNull(newMetadataValue, "Value cannot be null"); +- Validate.notNull(newMetadataValue.getOwningPlugin(), "Plugin cannot be null"); ++ Validate.notNull(owningPlugin, "Plugin cannot be null"); + String key = cachedDisambiguate(subject, metadataKey); +- if (!metadataMap.containsKey(key)) { +- metadataMap.put(key, new ArrayList()); +- } +- // we now have a list of subject's metadata for the given metadata key. If newMetadataValue's owningPlugin +- // is found in this list, replace the value rather than add a new one. +- List metadataList = metadataMap.get(key); +- for (int i = 0; i < metadataList.size(); i++) { +- if (metadataList.get(i).getOwningPlugin().equals(newMetadataValue.getOwningPlugin())) { +- metadataList.set(i, newMetadataValue); +- return; +- } ++ Map entry = metadataMap.get(key); ++ if (entry == null) { ++ entry = new WeakHashMap(1); ++ metadataMap.put(key, entry); + } +- // we didn't find a duplicate...add the new metadata value +- metadataList.add(newMetadataValue); ++ entry.put(owningPlugin, newMetadataValue); + } + + /** +@@ -57,7 +50,8 @@ public abstract class MetadataStoreBase { + public synchronized List getMetadata(T subject, String metadataKey) { + String key = cachedDisambiguate(subject, metadataKey); + if (metadataMap.containsKey(key)) { +- return Collections.unmodifiableList(metadataMap.get(key)); ++ Collection values = metadataMap.get(key).values(); ++ return Collections.unmodifiableList(new ArrayList(values)); + } else { + return Collections.emptyList(); + } +@@ -87,15 +81,11 @@ public abstract class MetadataStoreBase { + public synchronized void removeMetadata(T subject, String metadataKey, Plugin owningPlugin) { + Validate.notNull(owningPlugin, "Plugin cannot be null"); + String key = cachedDisambiguate(subject, metadataKey); +- List metadataList = metadataMap.get(key); +- if (metadataList == null) return; +- for (int i = 0; i < metadataList.size(); i++) { +- if (metadataList.get(i).getOwningPlugin().equals(owningPlugin)) { +- metadataList.remove(i); +- if (metadataList.isEmpty()) { +- metadataMap.remove(key); +- } +- } ++ Map entry = metadataMap.get(key); ++ if (entry == null) return; ++ entry.remove(owningPlugin); ++ if (entry.isEmpty()) { ++ metadataMap.remove(key); + } + } + +@@ -109,11 +99,9 @@ public abstract class MetadataStoreBase { + */ + public synchronized void invalidateAll(Plugin owningPlugin) { + Validate.notNull(owningPlugin, "Plugin cannot be null"); +- for (List values : metadataMap.values()) { +- for (MetadataValue value : values) { +- if (value.getOwningPlugin().equals(owningPlugin)) { +- value.invalidate(); +- } ++ for (Map values : metadataMap.values()) { ++ if (values.containsKey(owningPlugin)) { ++ values.get(owningPlugin).invalidate(); + } + } + } +diff --git a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +index c4b8b39..3b83380 100644 +--- a/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java ++++ b/src/main/java/org/bukkit/metadata/MetadataValueAdapter.java +@@ -1,5 +1,7 @@ + package org.bukkit.metadata; + ++import java.lang.ref.WeakReference; ++ + import org.apache.commons.lang.Validate; + import org.bukkit.plugin.Plugin; + import org.bukkit.util.NumberConversions; +@@ -13,15 +15,15 @@ import org.bukkit.util.NumberConversions; + * + */ + public abstract class MetadataValueAdapter implements MetadataValue { +- protected final Plugin owningPlugin; ++ protected final WeakReference owningPlugin; + + protected MetadataValueAdapter(Plugin owningPlugin) { + Validate.notNull(owningPlugin, "owningPlugin cannot be null"); +- this.owningPlugin = owningPlugin; ++ this.owningPlugin = new WeakReference(owningPlugin); + } + + public Plugin getOwningPlugin() { +- return owningPlugin; ++ return owningPlugin.get(); + } + + public int asInt() { +-- +1.8.1-rc2 +