TakeV/Paper
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Add a TON of metadata speed / memory fixes and improvements, courtesy of @crast

Browse source
This commit is contained in:
md_5 2013-03-23 10:30:53 +11:00
parent 4d4d0ef929
commit fa4499b65f
4 changed files with 558 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

258
Bukkit-Patches/0006-Refactor-conversions-from-LazyMetadataValue-into-abs.patch Normal file
View file

@ -0,0 +1,258 @@
From 0d2be83dcf857b4b29c304bfb4b5d67dde23a13c Mon Sep 17 00:00:00 2001
From: crast <contact@jamescrasta.com>
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<Object> lazyValue;
private CacheStrategy cacheStrategy;
private SoftReference<Object> internalValue = new SoftReference<Object>(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<Object> 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

42
Bukkit-Patches/0007-Clean-up-whitespace.patch Normal file
View file

@ -0,0 +1,42 @@
From c072f0a7250ce40674b9bfbcf8fe52fd28b25716 Mon Sep 17 00:00:00 2001
From: crast <contact@jamescrasta.com>
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

119
Bukkit-Patches/0008-Substantially-more-comprehensive-unit-tests.patch Normal file
View file

@ -0,0 +1,119 @@
From 634c75e521140bc33af968f8b5249123eee5aebc Mon Sep 17 00:00:00 2001
From: crast <contact@jamescrasta.com>
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

139
Bukkit-Patches/0009-Prevent-classloader-leakage-in-metadata-system.-Fixe.patch Normal file
View file

@ -0,0 +1,139 @@
From 7080e3e4d864249328686c7d04ffba43881a36ed Mon Sep 17 00:00:00 2001
From: crast <contact@jamescrasta.com>
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<T> {
- private Map<String, List<MetadataValue>> metadataMap = new HashMap<String, List<MetadataValue>>();
+ private Map<String, Map<Plugin, MetadataValue>> metadataMap = new HashMap<String, Map<Plugin, MetadataValue>>();
private WeakHashMap<T, Map<String, String>> disambiguationCache = new WeakHashMap<T, Map<String, String>>();
/**
@@ -26,23 +26,16 @@ public abstract class MetadataStoreBase<T> {
* @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<MetadataValue>());
- }
- // 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<MetadataValue> 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<Plugin, MetadataValue> entry = metadataMap.get(key);
+ if (entry == null) {
+ entry = new WeakHashMap<Plugin, MetadataValue>(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<T> {
public synchronized List<MetadataValue> getMetadata(T subject, String metadataKey) {
String key = cachedDisambiguate(subject, metadataKey);
if (metadataMap.containsKey(key)) {
- return Collections.unmodifiableList(metadataMap.get(key));
+ Collection<MetadataValue> values = metadataMap.get(key).values();
+ return Collections.unmodifiableList(new ArrayList<MetadataValue>(values));
} else {
return Collections.emptyList();
}
@@ -87,15 +81,11 @@ public abstract class MetadataStoreBase<T> {
public synchronized void removeMetadata(T subject, String metadataKey, Plugin owningPlugin) {
Validate.notNull(owningPlugin, "Plugin cannot be null");
String key = cachedDisambiguate(subject, metadataKey);
- List<MetadataValue> 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<Plugin, MetadataValue> 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<T> {
*/
public synchronized void invalidateAll(Plugin owningPlugin) {
Validate.notNull(owningPlugin, "Plugin cannot be null");
- for (List<MetadataValue> values : metadataMap.values()) {
- for (MetadataValue value : values) {
- if (value.getOwningPlugin().equals(owningPlugin)) {
- value.invalidate();
- }
+ for (Map<Plugin, MetadataValue> 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<Plugin> owningPlugin;
protected MetadataValueAdapter(Plugin owningPlugin) {
Validate.notNull(owningPlugin, "owningPlugin cannot be null");
- this.owningPlugin = owningPlugin;
+ this.owningPlugin = new WeakReference<Plugin>(owningPlugin);
}
public Plugin getOwningPlugin() {
- return owningPlugin;
+ return owningPlugin.get();
}
public int asInt() {
--
1.8.1-rc2