Fix thread contention in GenericVersionScheme and WeakInternPool

[gnodet]

Problem

PR #1902 introduced synchronized(versionCache) and synchronized(this.map) blocks around compound cache operations to fix a thread-safety issue with computeIfAbsent() on synchronizedMap(WeakHashMap). While correct, this serialized all threads on hot paths, causing a 2x build time regression for Quarkus (~6min → ~12min).

Root cause analysis

Collections.synchronizedMap(new WeakHashMap<>()) wraps each individual method (get, put) in synchronized(mutex), but does NOT make compound operations like computeIfAbsent() atomic. PR #1902 fixed this by adding outer synchronized blocks — correct but serializing.

The original pre-#1902 code used separate get() + put() calls where each individually acquires/releases the lock for microseconds. The compound race is benign for a cache — at worst a duplicate value is created and one wins the put.

Solution

1. ConcurrentWeakCache — new lightweight concurrent cache

Introduces ConcurrentWeakCache<K,V> — a stripped-down version of maven-impl's Cache, optimized for hot-path performance:

• Lock-free reads — ConcurrentHashMap.get() is a volatile read, no lock acquisition
• Zero allocation on get() — uses a ThreadLocal reusable lookup key instead of allocating a new wrapper per read (the main perf issue with the full Cache class)
• O(1) stale entry cleanup — identity-based removal from ReferenceQueue instead of entrySet().removeIf() full-map scan
• Weak keys — entries GC'd when key is no longer strongly referenced (same as WeakHashMap)
• Weak values — values also held via WeakReference

2. PathConflictResolver memory explosion fix

The PathConflictResolver had an O(n²) memory issue from copying HashSet<Object> conflictIdsSinceRoot at every graph node. Replaced with:

• Parent-chain walk (hasConflictIdOnPathToRoot()) — O(depth) per check, no copying
• LinkedHashSet partitions — O(1) removal instead of O(n) ArrayList.remove()

3. Tracking file read cache

EnhancedLocalRepositoryManager.readRepos() re-reads _remote.repositories tracking files from disk (with file locking) on every artifact resolution, even for artifacts in the same directory. In a primed build, this causes thousands of redundant file reads with synchronized blocks and FileLock acquisition.

Added a ConcurrentHashMap<Path, Properties> cache keyed by tracking file path. Multiple artifacts in the same directory (e.g. jar + pom) share the same tracking file, so the cache hit rate is high. The cache is invalidated (not updated) on writes via addRepo() to avoid a race where two concurrent writes could reorder their cache puts.

4. Lock-free fast path for NamedLockFactorySupport

getLockAndRefTrack() used ConcurrentHashMap.compute() on every call, which takes a per-bucket exclusive lock even when the holder already exists. In the common case (lock exists, just increment refcount), this serialized all threads hashing to the same bucket.

Added a lock-free fast path: ConcurrentHashMap.get() (volatile read) + tryIncRef() (CAS loop on AtomicInteger). Only falls back to compute() when the holder is absent or being closed.

The close/acquire race is handled by a CAS sentinel: closeLock() marks the holder as closed (CAS refcount 0 → MIN_VALUE) before destroying it. tryIncRef() rejects refcount ≤ 0, preventing revival of a destroyed lock.

5. InhibitingNameMapper stream→loop conversion

Replaced Stream.filter().collect() with imperative loops and added empty-list short-circuit to skip the filtering entirely when no LockingInhibitors are registered (the common case).

6. FileLockNamedLockFactory FileChannel reuse

destroyLock() was closing the FileChannel on every lock release, forcing a new open() syscall on every lock acquisition. Since the lock factory is session-scoped, the channels can be kept open for reuse across lock acquire/release cycles. Channels are now closed only during factory shutdown().

Profiling results (async-profiler wall-clock, 5ms interval)

End-to-end build time (Quarkus primed build, 8-thread)

Configuration	Build time	vs Baseline
Baseline (2.0.19-SNAPSHOT)	169.7s	—
+ tracking file cache	~160s	−6%
+ lock-free getLockAndRefTrack	149.1s	−12%
+ FileChannel reuse + InhibitingNameMapper	93.5s	−45%

Method-level sample comparison (baseline → all patches)

Method	Baseline	Patched	Change
EnhancedLocalRepositoryManager.find()	275	109	−60%
FileLockNamedLockFactory.createLock()	246	107	−56%
readRepos()	208	71	−66%
LegacyTrackingFileManager.read()	201	54	−73%
Retry.retry()	167	86	−49%
InhibitingNameMapper.nameLocks()	141	85	−40%
getLockAndRefTrack()	128	67	−48%
destroyLock()	46	0	−100%
mutex() (String.intern lock)	46	0	eliminated

ConcurrentWeakCache microbenchmark (4 threads, 8M ops/iteration)

Approach	Version Cache	Intern Pool
Master (synchronized blocks from #1902)	1668ms (4.8M ops/sec)	916ms (8.7M ops/sec)
Original synchronizedMap (pre-#1902)	360ms (22.2M ops/sec)	457ms (17.5M ops/sec)
ConcurrentWeakCache (this PR)	155ms (51.6M ops/sec)	176ms (45.5M ops/sec)

10.8x faster than master for version parsing, 5.2x faster for artifact interning, while preserving weak reference semantics for GC-friendly memory behavior.

Files changed

• ConcurrentWeakCache.java — new lightweight concurrent cache with weak keys/values
• GenericVersionScheme.java — use ConcurrentWeakCache for version parsing cache
• DataPool.java — use ConcurrentWeakCache for WeakInternPool
• PathConflictResolver.java — fix O(n²) memory from HashSet copying
• EnhancedLocalRepositoryManager.java — cache tracking file reads to eliminate redundant disk I/O
• NamedLockFactorySupport.java — lock-free fast path for ref-counted lock acquisition
• InhibitingNameMapper.java — stream→loop conversion with empty-list short-circuit
• FileLockNamedLockFactory.java — FileChannel reuse across lock lifecycle

[Copilot]

Pull request overview

This PR addresses performance regressions introduced by coarse-grained synchronization on hot-path caches by introducing a lightweight concurrent weak cache and switching existing caches to use it, while also reducing memory overhead in dependency graph conflict resolution.

Changes:

• Add ConcurrentWeakCache<K,V> to provide concurrent weak-key/weak-value caching with low read overhead.
• Switch GenericVersionScheme and DataPool.WeakInternPool from synchronized weak maps to ConcurrentWeakCache.
• Refactor PathConflictResolver to avoid per-node HashSet copying and to support O(1) removals via LinkedHashSet.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/ConcurrentWeakCache.java	Introduces a concurrent weak cache used to remove contention from cache hot paths.
maven-resolver-util/src/main/java/org/eclipse/aether/util/version/GenericVersionScheme.java	Replaces the synchronized version parsing cache with ConcurrentWeakCache.
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java	Reworks WeakInternPool to use ConcurrentWeakCache for lower contention and faster reads.
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java	Reduces memory usage by replacing conflict-id set copying with parent-chain cycle checks and using LinkedHashSet partitions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gnodet]

Concurrency & Data Lifecycle Review

I reviewed every changed file with a focus on (1) correctness when multiple builds run in parallel (separate JVMs sharing the same local repository), and (2) data lifecycle consistency within a single build. Here are the findings.

Scoping context

Component	Scope	Shared across sessions?
GenericVersionScheme	One per RepositorySystem	Yes — all sessions share the same versionCache
EnhancedLocalRepositoryManager	One per session (factory creates fresh)	No — each session gets its own trackingFileCache
DataPool.WeakInternPool	Stored in RepositoryCache	Yes if sessions share a RepositoryCache
FileLockNamedLockFactory	@Singleton per DI container	Yes — fileChannels map shared across sessions
NamedLockFactorySupport	@Singleton (inherited)	Yes — locks map shared across sessions
PathConflictResolver.State	Per transformGraph() call, single-threaded	No

---

Finding 1 — FileLockNamedLockFactory: DELETE_ON_CLOSE + channel reuse

Severity: Medium (not a regression, but changes observable behavior)

On non-Windows (the default), DELETE_LOCK_FILES=true and channels are opened with DELETE_ON_CLOSE. On Unix, the JDK's UnixChannelFactory.open() calls unlink(path) immediately after opening the file — the fd keeps the inode alive but the path disappears from the filesystem.

Old behavior (per-cycle): createLock → open(+unlink) → use fd → destroyLock → close fd (inode freed) → next createLock → open creates new file(+unlink) → new inode.

New behavior (reuse): createLock → open(+unlink) → use fd → destroyLock (no-op) → next createLock → computeIfAbsent returns existing channel → same fd, same inode. File never re-appears on disk.

For cross-process locking, both behaviors are equally broken with DELETE_ON_CLOSE on Unix — the lock file is unlinked immediately, so another process opening the same path creates a different inode and they lock on different files. This is pre-existing.

Practical concern: with the old code, a closed channel was removed from fileChannels, so the next createLock would open() with retry. With the new code, if the cached channel ever becomes invalid (e.g., NFS hiccup, disk error), all subsequent lock operations fail with no recovery path — computeIfAbsent keeps returning the stale channel.

Recommendation: Consider either (a) handling ClosedChannelException in FileLockNamedLock.obtainFileLock() by evicting the stale channel and retrying, or (b) documenting that the reuse assumes the underlying fd remains valid for the factory's lifetime.

---

Finding 2 — EnhancedLocalRepositoryManager: Stale cache entry after addRepo race

Severity: Low (documented, benign consequence)

Thread A: addRepo()    →  remove(trackingPath)  →  [gap]  →  update(trackingPath)
Thread B: readRepos()  →                   computeIfAbsent reads OLD data → caches stale entry

Between A's remove() and A's update(), thread B can read old data from disk and cache it. The stale entry persists until the next addRepo() invalidation or session end. Consequence: a redundant (but harmless) download.

Already correctly documented in the Javadoc. The invalidate-before-write ordering is correct (invalidate-after-write would be worse).

---

Finding 3 — EMPTY_PROPERTIES is a shared mutable singleton

Severity: Low (currently safe, fragile)

private static final Properties EMPTY_PROPERTIES = new Properties();

Properties extends Hashtable — fully mutable. All current callers (checkFind, isTracked) only call .get() and .keySet() (read-only). But nothing enforces this at the type level. If future code calls .put() on the cached Properties returned by readRepos(), it corrupts the shared singleton AND every trackingFileCache entry referencing it.

Recommendation: Consider adding a prominent @implNote warning, or wrapping readRepos() returns in an unmodifiable view for defense-in-depth.

---

Finding 4 — ConcurrentWeakCache.putIfAbsent: GC race on return path

Severity: Low (inherent to weak references, not a regression)

After merge() returns the winning existingRef, GC can collect the existing value before the subsequent ref.get():

WeakReference<V> ref = map.merge(...);
if (ref != newRef) {
    V existingValue = ref.get();  // ← GC window here
    if (existingValue != null) return existingValue;
}
return value;  // 'value' is NOT in the map

Two concurrent callers for the same key could briefly hold different instances, weakening the interning guarantee. The old synchronized(map) { map.compute() } had a tighter window because the synchronized block held a strong reference preventing GC during the return.

For GenericVersionScheme, this is harmless (value-equal GenericVersion instances). For DataPool intern pools, it's merely suboptimal (less deduplication). Inherent to weak references — not a bug.

---

Finding 5 — NamedLockFactorySupport: Lock-free fast path is correct ✅

Traced all race scenarios between getLockAndRefTrack() (fast path) and closeLock():

Scenario	Fast path	closeLock	Correct?
tryIncRef(1→2) before decRef(2→1)	Returns lock	Holder stays	✅
decRef(1→0) before tryIncRef	rejects 0 (≤0)	CAS(0→MIN_VALUE), destroys	✅
tryIncRef races with CAS sentinel	rejects MIN_VALUE (<0)	Lock destroyed	✅
tryIncRef(1→2) then decRef(2→1)	Lock acquired	refcount>0, holder kept	✅

Key correctness properties:

• tryIncRef() rejects both 0 and negative values (current <= 0)
• compute() serializes per-bucket — fast path and slow path can't create duplicate holders
• CAS sentinel (0→MIN_VALUE) is defense-in-depth, making "closed" permanent

---

Finding 6 — PathConflictResolver: Parent-chain walk is semantically equivalent ✅

The old HashSet<String> copy per child and the new hasConflictIdOnPathToRoot() walk check the same predicate. Objects.equals() correctly handles null conflictIds. LinkedHashSet for partitions gives O(1) removal vs O(N) with ArrayList, with no semantic change (identity-based Path elements are always unique). Purely single-threaded — no concurrency concerns.

---

Finding 7 — InhibitingNameMapper: Safe optimization ✅

lockingInhibitors is set once in the constructor and never modified. The isEmpty() short-circuit and stream→loop conversion are semantically identical. No shared mutable state.

---

Overall Assessment

The changes are concurrency-safe for the single-build case (one JVM, multiple threads). For parallel builds (separate JVMs sharing the same local repo):

• The tracking file cache is per-session-instance → separate JVMs get independent caches, no interaction
• The @Singleton lock factory is per-JVM → each JVM has its own locks map, correct
• Cross-process file locking with DELETE_ON_CLOSE on Unix was already unreliable pre-PR → no regression

The only actionable item is Finding 1 (stale FileChannel recovery). The rest are either informational, documented, or inherent to the weak-reference design.

[github-actions]

@cstamas Please assign appropriate label to PR according to the type of change.

[seregamorph]

nit: this seems to be not correct, shouldn't it be 2.0.20?