Copilot commented on code in PR #1937:
URL: https://github.com/apache/maven-resolver/pull/1937#discussion_r3484046525
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/version/GenericVersionScheme.java:
##########
@@ -54,102 +50,18 @@
*/
public class GenericVersionScheme extends VersionSchemeSupport {
- // Using WeakHashMap wrapped in synchronizedMap for thread safety and
memory-sensitive caching
- private final Map<String, GenericVersion> versionCache =
Collections.synchronizedMap(new WeakHashMap<>());
-
- // Cache statistics
- private final AtomicLong cacheHits = new AtomicLong(0);
- private final AtomicLong cacheMisses = new AtomicLong(0);
- private final AtomicLong totalRequests = new AtomicLong(0);
-
- // Static statistics across all instances
- private static final AtomicLong GLOBAL_CACHE_HITS = new AtomicLong(0);
- private static final AtomicLong GLOBAL_CACHE_MISSES = new AtomicLong(0);
- private static final AtomicLong GLOBAL_TOTAL_REQUESTS = new AtomicLong(0);
- private static final AtomicLong INSTANCE_COUNT = new AtomicLong(0);
-
- static {
- // Register shutdown hook to print statistics if enabled
- if (isStatisticsEnabled()) {
- Runtime.getRuntime().addShutdownHook(new
Thread(GenericVersionScheme::printGlobalStatistics));
- }
- }
-
- public GenericVersionScheme() {
- INSTANCE_COUNT.incrementAndGet();
- }
-
- /**
- * Checks if version scheme cache statistics should be printed.
- * This checks both the system property and the configuration property.
- */
- private static boolean isStatisticsEnabled() {
- // Check system property first (for backwards compatibility and ease
of use)
- String sysProp =
System.getProperty(ConfigurationProperties.VERSION_SCHEME_CACHE_DEBUG);
- if (sysProp != null) {
- return Boolean.parseBoolean(sysProp);
- }
-
- // Default to false if not configured
- return ConfigurationProperties.DEFAULT_VERSION_SCHEME_CACHE_DEBUG;
- }
+ // Concurrent cache with weak keys and weak values: lock-free reads
(volatile read, no lock
+ // acquisition via ConcurrentHashMap), lock-striped writes, zero
allocation on get() via
+ // ThreadLocal lookup key, weak references allow GC under memory pressure.
+ private final ConcurrentWeakCache<String, GenericVersion> versionCache =
new ConcurrentWeakCache<>();
Review Comment:
This change removes the previously added cache statistics/debug API in
GenericVersionScheme (including the public getCacheStatistics() method and
behavior controlled by ConfigurationProperties.VERSION_SCHEME_CACHE_DEBUG).
Removing a public method is a source/binary incompatible change, and it also
makes the documented VERSION_SCHEME_CACHE_DEBUG flag ineffective. Consider
keeping the API/feature (ideally with low-overhead counters gated behind the
flag) or deprecating and updating the documented configuration contract
accordingly.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/ConcurrentWeakCache.java:
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.eclipse.aether.util.concurrency;
+
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * A concurrent cache with weak keys and weak values, inspired by maven-impl's
Cache class
+ * but stripped down and optimized for hot-path performance.
+ * <p>
+ * Design compared to the full Cache:
+ * <ul>
+ * <li><b>Zero allocation on get()</b> — uses a ThreadLocal reusable lookup
key
+ * instead of allocating a new wrapper on every read</li>
+ * <li><b>O(1) cleanup per stale entry</b> — uses identity-based removal from
+ * the ReferenceQueue instead of a full {@code entrySet().removeIf()}
scan</li>
+ * <li><b>Lock-free reads</b> — {@link ConcurrentHashMap#get} is a volatile
read
+ * with no lock acquisition</li>
+ * <li><b>Lock-striped writes</b> — ConcurrentHashMap uses fine-grained
locking</li>
+ * </ul>
+ * <p>
+ * Keys are held via {@link WeakReference}: when a key is no longer strongly
referenced
+ * elsewhere, its entry becomes eligible for garbage collection. Values are
also held
+ * via WeakReference. Stale entries are cleaned up lazily on {@link #put}.
+ *
+ * @param <K> the type of keys
+ * @param <V> the type of values
+ * @since 2.0.19
+ */
+public class ConcurrentWeakCache<K, V> {
+
+ private final ConcurrentHashMap<Object, WeakReference<V>> map;
+ private final ReferenceQueue<Object> queue = new ReferenceQueue<>();
+
+ @SuppressWarnings("rawtypes")
+ private static final ThreadLocal<LookupKey> LOOKUP_KEY = new
ThreadLocal<LookupKey>() {
+ @Override
+ protected LookupKey initialValue() {
+ return new LookupKey();
+ }
+ };
+
+ /**
+ * Creates a new cache with default initial capacity.
+ */
+ public ConcurrentWeakCache() {
+ this.map = new ConcurrentHashMap<>();
+ }
+
+ /**
+ * Creates a new cache with the given initial capacity.
+ *
+ * @param initialCapacity the initial capacity
+ */
+ public ConcurrentWeakCache(int initialCapacity) {
+ this.map = new ConcurrentHashMap<>(initialCapacity);
+ }
+
+ /**
+ * Returns the value for the given key, or {@code null} if the key is not
present
+ * or the value has been garbage collected.
+ * <p>
+ * This method is lock-free and allocates no objects.
+ *
+ * @param key the key to look up
+ * @return the cached value, or {@code null}
+ */
+ @SuppressWarnings("unchecked")
+ public V get(K key) {
+ LookupKey<K> lookupKey = LOOKUP_KEY.get();
+ lookupKey.set(key);
+ try {
+ WeakReference<V> ref = map.get(lookupKey);
+ return ref != null ? ref.get() : null;
+ } finally {
+ lookupKey.clear();
+ }
+ }
+
+ /**
+ * Stores a key-value pair in the cache. Both key and value are held via
weak references.
+ * <p>
+ * Also performs lazy cleanup of entries whose keys have been garbage
collected.
+ *
+ * @param key the key
+ * @param value the value
+ */
+ public void put(K key, V value) {
+ expungeStaleEntries();
+ map.put(new WeakKey<>(key, queue), new WeakReference<>(value));
+ }
+
+ /**
+ * If the key is not already present (or its value has been GC'd), stores
the key-value pair
+ * and returns the given value. If the key is already present with a live
value, returns the
+ * existing value without storing. This ensures concurrent callers for the
same key always
+ * receive the same instance.
+ * <p>
+ * Also performs lazy cleanup of entries whose keys have been garbage
collected.
+ *
+ * @param key the key
+ * @param value the value to store if absent
+ * @return the existing value if present, or the given value if newly
stored
+ */
+ public V putIfAbsent(K key, V value) {
+ // Fast path: lock-free lookup, zero allocation (ThreadLocal LookupKey)
+ V existing = get(key);
+ if (existing != null) {
+ return existing;
+ }
+ // Slow path: allocate WeakKey + WeakReference and insert
+ expungeStaleEntries();
+ WeakReference<V> newRef = new WeakReference<>(value);
+ WeakReference<V> existingRef = map.putIfAbsent(new WeakKey<>(key,
queue), newRef);
+ if (existingRef != null) {
+ V existingValue = existingRef.get();
+ if (existingValue != null) {
+ return existingValue;
+ }
+ // Existing entry's value was GC'd — overwrite with new value
+ map.put(new WeakKey<>(key, queue), newRef);
+ }
+ return value;
+ }
Review Comment:
In ConcurrentWeakCache.putIfAbsent(), when an existing entry is present but
its WeakReference value has been GC’d, the current code unconditionally
overwrites via map.put(...). Under concurrent calls, two threads can both
observe a cleared existingRef and each overwrite/return their own value, so
concurrent callers for the same key may receive different instances (and
updates can clobber each other). This undermines the method’s documented
guarantee and can reduce effectiveness/correctness of interning.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java:
##########
@@ -635,9 +642,12 @@ private List<Path> addChildren(List<DependencyNode>
children) throws RepositoryE
ArrayList<Path> added = new ArrayList<>(children.size());
for (DependencyNode child : children) {
String childConflictId = this.state.conflictIds.get(child);
- Set<String> conflictIdsSinceRoot = new
HashSet<>(this.conflictIdsSinceRoot);
- boolean cycle = !conflictIdsSinceRoot.add(childConflictId);
- Path c = new Path(this.state, child, childConflictId,
conflictIdsSinceRoot, this);
+ // Detect cycles by walking up the parent chain instead of
copying a HashSet per child.
+ // This trades O(depth) per check for eliminating the dominant
memory consumer:
+ // previously, each Path copied its parent's entire
conflict-ID set, creating millions
+ // of HashMap.Node objects for large graphs.
+ boolean cycle = hasConflictIdOnPathToRoot(childConflictId);
Review Comment:
The addChildren() Javadoc (immediately above this loop) still refers to
maintaining "conflictIdsSinceRoot", but that field/approach was removed in this
PR. Please update the Javadoc to describe the new parent-chain walk used for
cycle detection, otherwise the comment is misleading.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
