From 3cfa68100b263eccc8f7c111926ca7064b48d1c2 Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Mon, 30 Sep 2024 15:03:47 -0700 Subject: [PATCH] Small performance and memory usage tweaks Refactor PropertyImpl#get() to avoid an allocation on each call. The new implementation should be marginally faster. Removed a few unnecessary objects from each instance of Property objects returned from the DefaultPropertyFactory. This will remove a few dozen bytes from each Property instance. --- .../com/netflix/archaius/api/Property.java | 43 +-- .../netflix/archaius/AbstractProperty.java | 5 + .../archaius/DefaultPropertyFactory.java | 306 ++++++++++++------ .../netflix/archaius/DelegatingProperty.java | 6 + 4 files changed, 236 insertions(+), 124 deletions(-) diff --git a/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java b/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java index 037b94971..03e1a94cd 100644 --- a/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java +++ b/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java @@ -20,7 +20,7 @@ import java.util.function.Supplier; /** - * API for composeable property access with optional chaining with default value support + * API for composable property access with optional chaining with default value support * as well as change notification. * * A {@link PropertyRepository} implementation normally implements some level of caching @@ -44,15 +44,15 @@ */ public interface Property extends Supplier { /** - * Token returned when calling onChange through which change notification can be - * unsubscribed. + * Token returned when calling {@link #subscribe(Consumer)} or {@link #onChange(Consumer)} through which change + * notification can be unsubscribed. */ interface Subscription { void unsubscribe(); } /** - * Return the most recent value of the property. + * Return the most recent value of the property. * * @return Most recent value for the property */ @@ -60,32 +60,30 @@ interface Subscription { T get(); /** - * Add a listener that will be called whenever the property value changes - * @param listener + * Add a listener that will be called whenever the property value changes. + * @implNote Implementors of this interface MUST override this method or {@link #subscribe(Consumer)}. + * @deprecated Use {@link Property#subscribe(Consumer)} instead */ @Deprecated default void addListener(PropertyListener listener) { - onChange(new Consumer() { - @Override - public void accept(T t) { - listener.accept(t); - } - }); + // Call subscribe for backwards compatibility. + // TODO: This behavior should be removed soon, because it causes a loop that implementors must work around. + subscribe(listener); } /** * Remove a listener previously registered by calling addListener - * @param listener + * @deprecated Use the {@link Subscription} object returned by {@link Property#subscribe(Consumer)} instead. */ @Deprecated default void removeListener(PropertyListener listener) {} /** - * @deprecated Use {@link Property#subscribe(Consumer)} - * @param consumer + * @deprecated Use {@link Property#subscribe(Consumer)} instead. */ @Deprecated default Subscription onChange(Consumer consumer) { + // Call subscribe for backwards compatibility return subscribe(consumer); } @@ -95,17 +93,13 @@ default Subscription onChange(Consumer consumer) { * since the notification only applies to the state of the chained property * up until this point. Changes to subsequent Property objects returned from {@link Property#orElse} * or {@link Property#map(Function)} will not trigger calls to this consumer. - - * @param consumer + * + * @implNote Implementors of this interface MUST override this method or {@link #addListener(PropertyListener)} + * to break a circular loop between the default implementations. * @return Subscription that may be unsubscribed to no longer get change notifications */ default Subscription subscribe(Consumer consumer) { - PropertyListener listener = new PropertyListener() { - @Override - public void onChange(T value) { - consumer.accept(value); - } - }; + PropertyListener listener = (PropertyListener) consumer; addListener(listener); return () -> removeListener(listener); @@ -114,7 +108,6 @@ public void onChange(T value) { /** * Create a new Property object that will return the specified defaultValue if * this object's property is not found. - * @param defaultValue * @return Newly constructed Property object */ default Property orElse(T defaultValue) { @@ -125,7 +118,6 @@ default Property orElse(T defaultValue) { * Create a new Property object that will fetch the property backed by the provided * key. The return value of the supplier will be cached until the configuration has changed * - * @param key * @return Newly constructed Property object */ default Property orElseGet(String key) { @@ -139,7 +131,6 @@ default Property orElseGet(String key) { * * Note that no orElseGet() calls may be made on a mapped property * - * @param delegate * @return Newly constructed Property object */ default Property map(Function mapper) { diff --git a/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java b/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java index 09bd110d5..05655b0c7 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java @@ -3,6 +3,11 @@ import com.netflix.archaius.api.Property; import com.netflix.archaius.api.PropertyListener; +/** @deprecated This class has no known users and doesn't offer any actual advantage over implementing {@link Property} + * directly. Scheduled to be removed by the end of 2025. + **/ +@Deprecated +// TODO Remove by the end of 2025 public abstract class AbstractProperty implements Property { private final String key; diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java index 5cd3c7717..954cbaf13 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java @@ -13,13 +13,15 @@ import java.lang.reflect.Type; import java.math.BigDecimal; import java.math.BigInteger; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicStampedReference; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; @@ -68,83 +70,7 @@ public DefaultPropertyFactory(Config config) { @Deprecated @SuppressWarnings("deprecation") public PropertyContainer getProperty(String propName) { - return new PropertyContainer() { - @Override - public Property asString(String defaultValue) { - return get(propName, String.class).orElse(defaultValue); - } - - @Override - public Property asInteger(Integer defaultValue) { - return get(propName, Integer.class).orElse(defaultValue); - } - - @Override - public Property asLong(Long defaultValue) { - return get(propName, Long.class).orElse(defaultValue); - } - - @Override - public Property asDouble(Double defaultValue) { - return get(propName, Double.class).orElse(defaultValue); - } - - @Override - public Property asFloat(Float defaultValue) { - return get(propName, Float.class).orElse(defaultValue); - } - - @Override - public Property asShort(Short defaultValue) { - return get(propName, Short.class).orElse(defaultValue); - } - - @Override - public Property asByte(Byte defaultValue) { - return get(propName, Byte.class).orElse(defaultValue); - } - - @Override - public Property asBoolean(Boolean defaultValue) { - return get(propName, Boolean.class).orElse(defaultValue); - } - - @Override - public Property asBigDecimal(BigDecimal defaultValue) { - return get(propName, BigDecimal.class).orElse(defaultValue); - } - - @Override - public Property asBigInteger(BigInteger defaultValue) { - return get(propName, BigInteger.class).orElse(defaultValue); - } - - @Override - public Property asType(Class type, T defaultValue) { - return get(propName, type).orElse(defaultValue); - } - - @Override - public Property asType(Function mapper, String defaultValue) { - T typedDefaultValue = applyOrThrow(mapper, defaultValue); - return getFromSupplier(propName, null, () -> { - String value = config.getString(propName, null); - if (value != null) { - return applyOrThrow(mapper, value); - } - - return typedDefaultValue; - }); - } - - private T applyOrThrow(Function mapper, String value) { - try { - return mapper.apply(value); - } catch (RuntimeException e) { - throw new ParseException("Invalid value '" + value + "' for property '" + propName + "'.", e); - } - } - }; + return new PropertyContainerImpl(propName); } @Override @@ -201,42 +127,103 @@ private Property getFromSupplier(KeyAndType keyAndType, Supplier su return (Property) properties.computeIfAbsent(keyAndType, (ignore) -> new PropertyImpl<>(keyAndType, supplier)); } + /** + * Implementation of the Property interface. This class looks at the factory's masterVersion on each read to + * determine if the cached parsed values is stale. + */ private final class PropertyImpl implements Property { + private final KeyAndType keyAndType; private final Supplier supplier; - private final AtomicStampedReference cache = new AtomicStampedReference<>(null, -1); - private final ConcurrentMap, Subscription> oldSubscriptions = new ConcurrentHashMap<>(); + + // Caching machinery. Writes to cachedValue are guarded by updateLock. Reads can be unguarded because the field is volatile. + private final ReentrantLock updateLock = new ReentrantLock(); + private volatile CachedValue cachedValue; + + // Keep track of old-style listeners so we can unsubscribe them when they are removed + // Field is initialized on demand only if it's actually needed. + // Access is synchronized on _this_. + private Map, Subscription> oldSubscriptions; public PropertyImpl(KeyAndType keyAndType, Supplier supplier) { this.keyAndType = keyAndType; this.supplier = supplier; } - + + /** + * Get the current value of the property. If the value is not cached or the cache is stale, the value is + * updated from the supplier. If the supplier throws an exception, the exception is logged and rethrown. + *

+ * This method is intended to provide the following semantics: + *

    + *
  • Changes to a property are atomic.
    • + *
  • Updates from the backing Config are eventually consistent.
    • + *
  • When multiple updates happen then "last one wins", as ordered by calls to the PropertyFactory's invalidate() method.
    • + *
  • A property only changes value *after* a call to invalidate()
    • + *
  • Updates *across* different properties are not transactional. A thread may see (newA, oldB) while a different concurrent thread sees (oldA, newB)
    • + *
+ * @throws RuntimeException if the supplier throws an exception + */ @Override public T get() { - int[] cacheVersion = new int[1]; - T currentValue = cache.get(cacheVersion); - int latestVersion = masterVersion.get(); + int currentMasterVersion = masterVersion.get(); + + if (cachedValue != null) { + // Happy path. We have an up-to-date cached value, so just return that + if (cachedValue.version >= currentMasterVersion) { + return cachedValue.value; + } + + // The cached value is stale, someone needs to update it + if (!updateLock.tryLock()) { + // The lock is taken, so another thread is already working on the update. + // We can just return the stale value. Since cachedValue is a volatile, it may even have gotten updated + // by now. That's fine too. :-) + return cachedValue.value; + } + + // If we're here we have the lock and we skip past the else ... + + } else { + + // There is no cached value at all, not even stale, so we MUST wait until it's updated, either by us + // or by another thread. So no tryLock here, we HAVE to block until the lock is available. + updateLock.lock(); + + // Done waiting, got the lock. But maybe someone did the update while we were waiting for the lock? + if (cachedValue != null) { + // Yes! We can just return the value that was computed by that other thread (and release the lock before leaving!) + try { + return cachedValue.value; + } finally { + updateLock.unlock(); + } + } - if (cacheVersion[0] == latestVersion) { - return currentValue; + // Nope, still null. Let's proceed. } + // At this point, we have the lock AND there's no valid cache. The job of updating falls on us. try { + // Get the new value from the supplier. This call could fail. T newValue = supplier.get(); - if (cache.compareAndSet(currentValue, newValue, cacheVersion[0], latestVersion)) { - // newValue could be stale here already, if the cache was updated *again* between the CAS and this line - // We don't care enough about this edge case to fix it. - return newValue; - } + // We successfully got the new value! + // Atomically update both the value and the version + cachedValue = new CachedValue<>(newValue, currentMasterVersion); + // And return + return newValue; } catch (RuntimeException e) { + // Oh, no, something went wrong while trying to get the new value. Log the error and rethrow the exception + // so our caller knows there's a problem. We leave the cache unchanged. Next caller will try again. LOG.error("Unable to get current version of property '{}'", keyAndType.key, e); - throw e; // Rethrow the exception, our caller should know that the property is not available - } + throw e; + } finally { - return cache.getReference(); + // Success or not, release the lock before leaving. + updateLock.unlock(); + } } @Override @@ -273,8 +260,11 @@ public synchronized void run() { @Deprecated @Override @SuppressWarnings("deprecation") - public void addListener(PropertyListener listener) { - oldSubscriptions.put(listener, onChange(listener)); + public synchronized void addListener(PropertyListener listener) { + if (oldSubscriptions == null) { + oldSubscriptions = new HashMap<>(); + } + oldSubscriptions.put(listener, subscribe(listener)); } /** @@ -284,7 +274,11 @@ public void addListener(PropertyListener listener) { @Deprecated @Override @SuppressWarnings("deprecation") - public void removeListener(PropertyListener listener) { + public synchronized void removeListener(PropertyListener listener) { + if (oldSubscriptions == null) { + return; + } + Subscription subscription = oldSubscriptions.remove(listener); if (subscription != null) { subscription.unsubscribe(); @@ -326,10 +320,14 @@ public Property map(Function mapper) { @Override public String toString() { - return "Property [Key=" + getKey() + "; value="+get() + "]"; + return "Property [Key=" + keyAndType + "; cachedValue="+ cachedValue + "]"; } } + /** + * Holder for a pair of property name and type. Used as a key in the properties map. + * @param + */ private static final class KeyAndType { private final String key; private final Type type; @@ -384,4 +382,116 @@ public String toString() { '}'; } } + + /** A holder for a cached value and the version of the master config at which it was updated. */ + private static final class CachedValue { + final T value; + final int version; + + CachedValue(T value, int version) { + this.value = value; + this.version = version; + } + + @Override + public String toString() { + return "CachedValue{" + + "value=" + value + + ", version=" + version + + '}'; + } + } + + /** + * Implements the deprecated PropertyContainer interface, for backwards compatibility. + */ + @SuppressWarnings("deprecation") + private final class PropertyContainerImpl implements PropertyContainer { + private final String propName; + + public PropertyContainerImpl(String propName) { + this.propName = propName; + } + + @Override + public Property asString(String defaultValue) { + return get(propName, String.class).orElse(defaultValue); + } + + @Override + public Property asInteger(Integer defaultValue) { + return get(propName, Integer.class).orElse(defaultValue); + } + + @Override + public Property asLong(Long defaultValue) { + return get(propName, Long.class).orElse(defaultValue); + } + + @Override + public Property asDouble(Double defaultValue) { + return get(propName, Double.class).orElse(defaultValue); + } + + @Override + public Property asFloat(Float defaultValue) { + return get(propName, Float.class).orElse(defaultValue); + } + + @Override + public Property asShort(Short defaultValue) { + return get(propName, Short.class).orElse(defaultValue); + } + + @Override + public Property asByte(Byte defaultValue) { + return get(propName, Byte.class).orElse(defaultValue); + } + + @Override + public Property asBoolean(Boolean defaultValue) { + return get(propName, Boolean.class).orElse(defaultValue); + } + + @Override + public Property asBigDecimal(BigDecimal defaultValue) { + return get(propName, BigDecimal.class).orElse(defaultValue); + } + + @Override + public Property asBigInteger(BigInteger defaultValue) { + return get(propName, BigInteger.class).orElse(defaultValue); + } + + @Override + public Property asType(Class type, T defaultValue) { + return get(propName, type).orElse(defaultValue); + } + + @Override + public Property asType(Function mapper, String defaultValue) { + T typedDefaultValue = applyOrThrow(mapper, defaultValue); + return getFromSupplier(propName, null, () -> { + String value = config.getString(propName, null); + if (value != null) { + return applyOrThrow(mapper, value); + } + + return typedDefaultValue; + }); + } + + private T applyOrThrow(Function mapper, String value) { + try { + return mapper.apply(value); + } catch (RuntimeException e) { + throw new ParseException("Invalid value '" + value + "' for property '" + propName + "'.", e); + } + } + + @Override + public String toString() { + return "PropertyContainer [name=" + propName + "]"; + } + } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java b/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java index 5c18d4078..9be1c8d90 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java @@ -3,6 +3,12 @@ import com.netflix.archaius.api.Property; import com.netflix.archaius.api.PropertyListener; +/** + * Base class for Property implementations that delegate to another Property. + * @deprecated There are no known implementations of this class. To be removed by the end of 2025 + */ +@Deprecated +// TODO Remove by the end of 2025 public abstract class DelegatingProperty implements Property { protected Property delegate;