From 87ac3f04f9adfc5780a1d271d7bed896a99f1ef7 Mon Sep 17 00:00:00 2001 From: Eran Landau Date: Fri, 30 Sep 2016 11:04:27 -0700 Subject: [PATCH] Fix performance problem with DefaultPropertyContainer where not caching the Property in user code would result in excessive operations on the CopyOnWriteArrayList backing the property cache. The fix also address the problem of having multiple Property objects for the same types but each with a different default value. Each Property can now have a different default value. --- .../netflix/archaius/api/PropertyFactory.java | 1 - .../property/DefaultPropertyContainer.java | 326 ++++++++++-------- .../archaius/property/ListenerManager.java | 2 - .../archaius/property/PropertyTest.java | 23 +- 4 files changed, 197 insertions(+), 155 deletions(-) diff --git a/archaius2-api/src/main/java/com/netflix/archaius/api/PropertyFactory.java b/archaius2-api/src/main/java/com/netflix/archaius/api/PropertyFactory.java index 4fc632abc..b85970ea7 100644 --- a/archaius2-api/src/main/java/com/netflix/archaius/api/PropertyFactory.java +++ b/archaius2-api/src/main/java/com/netflix/archaius/api/PropertyFactory.java @@ -20,7 +20,6 @@ * Factory of PropertyContainer objects. * * @see PropertyContainer - * @author elandau */ public interface PropertyFactory { /** diff --git a/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java b/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java index 73d3146ea..a36c3cd4d 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/property/DefaultPropertyContainer.java @@ -26,6 +26,7 @@ import java.math.BigDecimal; import java.math.BigInteger; +import java.util.Iterator; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -42,14 +43,11 @@ * * Once created a PropertyContainer property cannot be removed. However, listeners may be * added and removed. - * - * @author elandau - * */ public class DefaultPropertyContainer implements PropertyContainer { private final Logger LOG = LoggerFactory.getLogger(DefaultPropertyContainer.class); - enum Type { + enum CacheType { INTEGER (int.class, Integer.class), BYTE (byte.class, Byte.class), SHORT (short.class, Short.class), @@ -64,12 +62,12 @@ enum Type { private final Class[] types; - Type(Class ... type) { + CacheType(Class ... type) { types = type; } - static Type fromClass(Class clazz) { - for (Type type : values()) { + static CacheType fromClass(Class clazz) { + for (CacheType type : values()) { for (Class cls : type.types) { if (cls.equals(clazz)) { return type; @@ -116,45 +114,23 @@ public DefaultPropertyContainer(String key, Config config, AtomicInteger version this.masterVersion = version; } - abstract class CachedProperty implements Property { + class PropertyWithDefault implements Property { + private CachedProperty delegate; + private T defaultValue; + public PropertyWithDefault(CachedProperty delegate, T defaultValue) { + this.defaultValue = defaultValue; + this.delegate = delegate; + } + @Override - public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + ((defaultValue == null) ? 0 : defaultValue.hashCode()); - result = prime * result + type; - return result; + public T get() { + T value = delegate.get(); + return value != null ? value : defaultValue; } @Override - public boolean equals(Object obj) { - if (this == obj) - return true; - if (obj == null) - return false; - if (getClass() != obj.getClass()) - return false; - CachedProperty other = (CachedProperty) obj; - if (defaultValue == null) { - if (other.defaultValue != null) - return false; - } else if (!defaultValue.equals(other.defaultValue)) - return false; - if (type != other.type) - return false; - return true; - } - private final AtomicStampedReference cache = new AtomicStampedReference<>(null, -1); - private final int type; - private final T defaultValue; - - CachedProperty(Type type, T defaultValue) { - this.type = type.ordinal(); - this.defaultValue = defaultValue; - } - - public void addListener(final PropertyListener listener) { + public void addListener(PropertyListener listener) { listeners.add(listener, new ListenerUpdater() { private AtomicReference last = new AtomicReference(null); @@ -165,35 +141,63 @@ public void update() { try { value = get(); - } - catch (Exception e) { + } catch (Exception e) { listener.onParseError(e); return; } - if (prev != value) { - if (last.compareAndSet(prev, value)) { - listener.onChange(value); - } + if (prev != value && last.compareAndSet(prev, value)) { + listener.onChange(value != null ? value : defaultValue); } } }); } - + + @Override + public void removeListener(PropertyListener listener) { + listeners.remove(listener); + } + @Override public String getKey() { - return DefaultPropertyContainer.this.key; + return delegate.getKey(); } + } + + abstract class CachedProperty { - public void removeListener(PropertyListener listener) { - listeners.remove(listener); + @Override + public int hashCode() { + return type; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) + return true; + if (obj == null) + return false; + if (getClass() != obj.getClass()) + return false; + CachedProperty other = (CachedProperty) obj; + return type != other.type; + } + private final AtomicStampedReference cache = new AtomicStampedReference<>(null, -1); + private final int type; + + CachedProperty(int type) { + this.type = type; } + public String getKey() { + return DefaultPropertyContainer.this.key; + } + /** * Fetch the latest version of the property. If not up to date then resolve to the latest * value, inline. * - * TODO: Make resolving property value an offline task + * TODO: Make resolving property value an background task * * @return */ @@ -213,29 +217,22 @@ public T get() { if (cache.compareAndSet(currentValue, newValue, cacheVersion, latestVersion)) { // Slight race condition here but not important enough to warrent locking lastUpdateTimeInMillis = System.currentTimeMillis(); - return firstNonNull(newValue, defaultValue); + return newValue; } } - return firstNonNull(cache.getReference(), defaultValue); + return cache.getReference(); } public long getLastUpdateTime(TimeUnit units) { return units.convert(lastUpdateTimeInMillis, TimeUnit.MILLISECONDS); } - private T firstNonNull(T first, T second) { - return first == null ? second : first; - } /** * Resolve to the most recent value * @return * @throws Exception */ protected abstract T resolveCurrent() throws Exception; - - private DefaultPropertyContainer getOuterType() { - return DefaultPropertyContainer.this; - } } /** @@ -245,118 +242,153 @@ private DefaultPropertyContainer getOuterType() { * @return */ @SuppressWarnings("unchecked") - private CachedProperty add(final CachedProperty newProperty) { - // TODO(nikos): This while() looks like it's redundant - // since we are only calling add() after a get(). - while (!cache.add(newProperty)) { - for (CachedProperty property : cache) { - if (property.equals(newProperty)) { - return (CachedProperty) property; + private Property add(final int type, final Function> creator, final T defaultValue) { + do { + Iterator> iter = cache.iterator(); + while (iter.hasNext()) { + CachedProperty element = iter.next(); + if (element.type == type) { + return new PropertyWithDefault((CachedProperty)element, defaultValue); } } - } - - return newProperty; + + // TODO(nikos): This while() looks like it's redundant + // since we are only calling add() after a get(). + CachedProperty cachedProperty = creator.apply(type); + if (cache.addIfAbsent(cachedProperty)) { + return new PropertyWithDefault(cachedProperty, defaultValue); + } + } while (true); } @Override public Property asString(final String defaultValue) { - return add(new CachedProperty(Type.STRING, defaultValue) { - @Override - protected String resolveCurrent() throws Exception { - return config.getString(key, null); - } - }); + return add( + CacheType.STRING.ordinal(), + type -> new CachedProperty(type) { + @Override + protected String resolveCurrent() throws Exception { + return config.getString(key, null); + } + }, + defaultValue); } @Override public Property asInteger(final Integer defaultValue) { - return add(new CachedProperty(Type.INTEGER, defaultValue) { - @Override - protected Integer resolveCurrent() throws Exception { - return config.getInteger(key, null); - } - }); - } + return add( + CacheType.INTEGER.ordinal(), + type -> new CachedProperty(type) { + @Override + protected Integer resolveCurrent() throws Exception { + return config.getInteger(key, null); + } + }, + defaultValue); + } @Override public Property asLong(final Long defaultValue) { - return add(new CachedProperty(Type.LONG, defaultValue) { - @Override - protected Long resolveCurrent() throws Exception { - return config.getLong(key, null); - } - }); + return add( + CacheType.LONG.ordinal(), + type -> new CachedProperty(type) { + @Override + protected Long resolveCurrent() throws Exception { + return config.getLong(key, null); + } + }, + defaultValue); } @Override public Property asDouble(final Double defaultValue) { - return add(new CachedProperty(Type.DOUBLE, defaultValue) { - @Override - protected Double resolveCurrent() throws Exception { - return config.getDouble(key, null); - } - }); + return add( + CacheType.DOUBLE.ordinal(), + type -> new CachedProperty(type) { + @Override + protected Double resolveCurrent() throws Exception { + return config.getDouble(key, null); + } + }, + defaultValue); } @Override public Property asFloat(final Float defaultValue) { - return add(new CachedProperty(Type.FLOAT, defaultValue) { - @Override - protected Float resolveCurrent() throws Exception { - return config.getFloat(key, null); - } - }); + return add( + CacheType.FLOAT.ordinal(), + type -> new CachedProperty(type) { + @Override + protected Float resolveCurrent() throws Exception { + return config.getFloat(key, null); + } + }, + defaultValue); } @Override public Property asShort(final Short defaultValue) { - return add(new CachedProperty(Type.SHORT, defaultValue) { - @Override - protected Short resolveCurrent() throws Exception { - return config.getShort(key, null); - } - }); + return add( + CacheType.SHORT.ordinal(), + type -> new CachedProperty(type) { + @Override + protected Short resolveCurrent() throws Exception { + return config.getShort(key, null); + } + }, + defaultValue); } @Override public Property asByte(final Byte defaultValue) { - return add(new CachedProperty(Type.BYTE, defaultValue) { - @Override - protected Byte resolveCurrent() throws Exception { - return config.getByte(key, defaultValue); - } - }); + return add( + CacheType.BYTE.ordinal(), + type -> new CachedProperty(type) { + @Override + protected Byte resolveCurrent() throws Exception { + return config.getByte(key, null); + } + }, + defaultValue); } @Override public Property asBigDecimal(final BigDecimal defaultValue) { - return add(new CachedProperty(Type.BIG_DECIMAL, defaultValue) { - @Override - protected BigDecimal resolveCurrent() throws Exception { - return config.getBigDecimal(key, defaultValue); - } - }); + return add( + CacheType.BIG_DECIMAL.ordinal(), + type -> new CachedProperty(type) { + @Override + protected BigDecimal resolveCurrent() throws Exception { + return config.getBigDecimal(key, null); + } + }, + defaultValue); } @Override public Property asBoolean(final Boolean defaultValue) { - return add(new CachedProperty(Type.BOOLEAN, defaultValue) { - @Override - protected Boolean resolveCurrent() throws Exception { - return config.getBoolean(key, defaultValue); - } - }); + return add( + CacheType.BOOLEAN.ordinal(), + type -> new CachedProperty(type) { + @Override + protected Boolean resolveCurrent() throws Exception { + return config.getBoolean(key, null); + } + }, + defaultValue); } @Override public Property asBigInteger(final BigInteger defaultValue) { - return add(new CachedProperty(Type.BIG_INTEGER, defaultValue) { - @Override - protected BigInteger resolveCurrent() throws Exception { - return config.getBigInteger(key, defaultValue); - } - }); + return add( + CacheType.BIG_INTEGER.ordinal(), + type -> new CachedProperty(type) { + @Override + protected BigInteger resolveCurrent() throws Exception { + return config.getBigInteger(key, null); + } + }, + defaultValue); } /** @@ -365,7 +397,7 @@ protected BigInteger resolveCurrent() throws Exception { @SuppressWarnings("unchecked") @Override public Property asType(final Class type, final T defaultValue) { - switch (Type.fromClass(type)) { + switch (CacheType.fromClass(type)) { case INTEGER: return (Property) asInteger((Integer)defaultValue); case BYTE: @@ -386,25 +418,29 @@ public Property asType(final Class type, final T defaultValue) { return (Property) asBigDecimal((BigDecimal)defaultValue); case BIG_INTEGER: return (Property) asBigInteger((BigInteger)defaultValue); - default: { - CachedProperty prop = add(new CachedProperty(Type.CUSTOM, defaultValue) { + default: + return add( + CacheType.CUSTOM.ordinal(), + t -> new CachedProperty(t) { @Override protected T resolveCurrent() throws Exception { - return config.get(type, key, defaultValue); + return config.get(type, key, null); } - }); - return prop; - } + }, + defaultValue); } } @Override public Property asType(Function type, String defaultValue) { - return add(new CachedProperty(Type.CUSTOM, null) { - @Override - protected T resolveCurrent() throws Exception { - return type.apply(config.getString(key, defaultValue)); - } - }); + return add( + CacheType.CUSTOM.ordinal(), + t -> new CachedProperty(t) { + @Override + protected T resolveCurrent() throws Exception { + return type.apply(config.getString(key, defaultValue)); + } + }, + type.apply(defaultValue)); } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/property/ListenerManager.java b/archaius2-core/src/main/java/com/netflix/archaius/property/ListenerManager.java index 4ba095364..da5c006ec 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/property/ListenerManager.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/property/ListenerManager.java @@ -11,8 +11,6 @@ * an optimization so it is not necessary to iterate through all property * containers when the listeners need to be invoked since the expectation * is to have far less listeners than property containers. - * - * @author elandau */ public class ListenerManager { public static interface ListenerUpdater { diff --git a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java index 2ef9a1be7..b3a6dd1a6 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java @@ -15,13 +15,6 @@ */ package com.netflix.archaius.property; -import java.math.BigDecimal; -import java.math.BigInteger; -import java.util.concurrent.atomic.AtomicInteger; - -import org.junit.Assert; -import org.junit.Test; - import com.netflix.archaius.DefaultPropertyFactory; import com.netflix.archaius.api.Property; import com.netflix.archaius.api.PropertyFactory; @@ -30,6 +23,13 @@ import com.netflix.archaius.api.exceptions.ConfigException; import com.netflix.archaius.config.DefaultSettableConfig; +import org.junit.Assert; +import org.junit.Test; + +import java.math.BigDecimal; +import java.math.BigInteger; +import java.util.concurrent.atomic.AtomicInteger; + public class PropertyTest { static class MyService { private Property value; @@ -209,4 +209,13 @@ public void onParseError(Throwable error) { config.setProperty("foo", "${goo}"); Assert.assertEquals(456, current.intValue()); } + + @Test + public void testDifferentDefaults() { + SettableConfig config = new DefaultSettableConfig(); + + DefaultPropertyFactory factory = DefaultPropertyFactory.from(config); + Assert.assertFalse(factory.getProperty("foo").asBoolean(false).get()); + Assert.assertTrue(factory.getProperty("foo").asBoolean(true).get()); + } }