-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Improve Maven cache architecture for better memory efficiency and performance #2506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
48385ea
to
087a13a
Compare
60e48b1
to
888ab92
Compare
- Remove CacheMapAdapter.java as it's no longer needed - Update DefaultRequestCache to use Cache<K,V> directly instead of Map adapter - Update AbstractSession to use Cache for all internal caching: * services cache: Map -> Cache * allNodes cache: WeakHashMap -> Cache * allArtifacts cache: ConcurrentHashMap<Class, Map> -> Cache<Class, Cache> * allRepositories cache: WeakHashMap -> Cache * allDependencies cache: WeakHashMap -> Cache - This brings the implementation closer to the original PR apache#2506 - All 12 cache tests continue to pass - Maintains thread safety and memory efficiency with soft references
This change brings the implementation closer to the original PR apache#2506 by: 1. **Removed CacheMapAdapter**: Eliminated the adapter layer that was converting between Map and Cache interfaces, simplifying the architecture. 2. **Updated DefaultRequestCache**: Changed from using Map<Object, CachingSupplier<?, ?>> to Cache<Object, CachingSupplier<?, ?>> directly, removing the need for adapters. 3. **Updated AbstractSession**: Replaced all Map-based caches with Cache interface: - services: Map<Class<? extends Service>, Service> → Cache<Class<? extends Service>, Service> - allNodes: WeakHashMap<DependencyNode, Node> → Cache<DependencyNode, Node> - allArtifacts: ConcurrentHashMap<Class, Map<Artifact, Artifact>> → Cache<Class, Cache<Artifact, Artifact>> - allRepositories: WeakHashMap<RemoteRepository, RemoteRepository> → Cache<RemoteRepository, RemoteRepository> - allDependencies: WeakHashMap<Dependency, Dependency> → Cache<Dependency, Dependency> **Benefits:** - **Consistent caching**: All caches now use the same modern Cache implementation - **Better memory management**: Soft references throughout instead of mixed approaches - **Thread safety**: All caches benefit from the thread-safe DefaultCache implementation - **Simplified architecture**: Removed unnecessary adapter layer - **Closer to original vision**: Uses Cache<K,V> consistently as intended in PR apache#2506 **Verification:** - ✅ All 12 cache tests pass - ✅ Maven-impl module builds successfully - ✅ Maintains backward compatibility through the Cache interface This change focuses on the core cache infrastructure in maven-impl. Additional cache classes in maven-core can be updated in a separate effort.
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java
Outdated
Show resolved
Hide resolved
impl/maven-core/src/test/java/org/apache/maven/project/DefaultMavenProjectBuilderTest.java
Outdated
Show resolved
Hide resolved
impl/maven-core/src/test/java/org/apache/maven/project/DefaultMavenProjectBuilderTest.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java
Outdated
Show resolved
Hide resolved
...n-impl/src/test/java/org/apache/maven/impl/cache/ReferenceTypeStatisticsIntegrationTest.java
Outdated
Show resolved
Hide resolved
Analysis of the working PR apache#2506 revealed that the key difference is the use of WEAK references instead of SOFT references for cache entries. Root Cause: - SOFT references are only cleared when JVM is close to OutOfMemoryError - This causes excessive memory retention and can lead to OOM in large builds - WEAK references are cleared much more aggressively when objects are no longer strongly referenced, preventing memory buildup Changes: - Switch from SoftReference to WeakReference for both keys and values - Update documentation to reflect the change from soft to weak references - This aligns with the working PR apache#2506 which uses Cache.ReferenceType.WEAK Benefits: - Prevents OOM issues in integration tests and large builds - Maintains caching benefits while allowing more aggressive garbage collection - Follows the proven approach from the comprehensive PR apache#2506 All 12 cache tests continue to pass with this change.
This commit removes the following components from the comprehensive PR apache#2506: - CacheConfigurationResolver and related configuration classes - CacheStatistics and related statistics tracking - Cache configuration parsing and system property handling - Factory methods from InputSource and InputLocation (merge methods) - Complex object pooling configuration (keeping only Dependency pooling) Remaining work: - Fix compilation errors for missing interfaces - Adapt to current master branch API structure - Ensure all tests pass This trimmed version keeps the core cache improvements while removing the configuration complexity that's not needed for the 4.0.x branch.
This commit removes the following components from the comprehensive PR apache#2506: - CacheConfigurationResolver and related configuration classes - CacheStatistics and related statistics tracking - Cache configuration parsing and system property handling - Factory methods from InputSource and InputLocation (merge methods) - Complex object pooling configuration (keeping only Dependency pooling) Remaining work: - Fix compilation errors for missing interfaces - Adapt to current master branch API structure - Ensure all tests pass This trimmed version keeps the core cache improvements while removing the configuration complexity that's not needed for the 4.0.x branch.
This commit removes the following components from the comprehensive PR apache#2506: - CacheConfigurationResolver and related configuration classes - CacheStatistics and related statistics tracking - Cache configuration parsing and system property handling - Factory methods from InputSource and InputLocation (merge methods) - Complex object pooling configuration (keeping only Dependency pooling) Remaining work: - Fix compilation errors for missing interfaces - Adapt to current master branch API structure - Ensure all tests pass This trimmed version keeps the core cache improvements while removing the configuration complexity that's not needed for the 4.0.x branch.
a1ab28e
to
73b1fe8
Compare
This commit removes the following components from the comprehensive PR apache#2506: - CacheConfigurationResolver and related configuration classes - CacheStatistics and related statistics tracking - Cache configuration parsing and system property handling - Factory methods from InputSource and InputLocation (merge methods) - Complex object pooling configuration (keeping only Dependency pooling) Remaining work: - Fix compilation errors for missing interfaces - Adapt to current master branch API structure - Ensure all tests pass This trimmed version keeps the core cache improvements while removing the configuration complexity that's not needed for the 4.0.x branch. # Conflicts: # impl/maven-impl/src/main/java/org/apache/maven/impl/cache/DefaultRequestCache.java # impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelObjectPool.java # impl/maven-impl/src/test/java/org/apache/maven/impl/cache/ReferenceTypeStatisticsTest.java
src/mdo/java/InputLocation.java
Outdated
} | ||
|
||
public static InputLocation of(int lineNumber, int columnNumber) { | ||
return new InputLocation(lineNumber, columnNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of those have reasons for doing this that simply don't apply here. This does nothing the existing constructor doesn't do, and just adds code and API complexity. When the basics work, stick to the basics.
src/mdo/java/InputLocation.java
Outdated
} | ||
|
||
public static InputLocation of(int lineNumber, int columnNumber) { | ||
return new InputLocation(lineNumber, columnNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better yet, just don't deprecate the constructors and don't introduce factory methods. Effective Java lists 4 reasons to consider factory methods. Exactly zero of those apply here. In fact, rereading that chapter I notice that one of the reasons is so that constructors with different arguments have meaningful, dissimilar names. And yet here all four methods are named the undescriptive "of".
Plugging in ModelObjectPool could be a reason to use factory methods, but this PR isn't doing that.
d07426b
to
5bf21df
Compare
@elharo hopefully, laid out this way, you'll understand why the static factory method pattern makes more sense for the location tracking classes. |
5bf21df
to
dee7262
Compare
Is this to be revived? |
Could be. I need to rebase on latest master and run the tests at https://github.com/gnodet/maven-bench to get proper results. |
…formance This comprehensive enhancement to Maven's caching system addresses memory issues and significantly improves performance through several key improvements: - Enhanced DefaultRequestCache with configurable reference types and CSS-like selectors - Pluggable ModelObjectPool service architecture with configurable object types - Comprehensive cache statistics with eviction tracking - Improved InputLocation and InputSource with ImmutableCollections - Custom equality strategy for Dependency pooling - Enhanced parent request matching with interface checking - Configurable cache statistics display - Maven 3: Requires -Xmx1536m, runs in 45 seconds - Maven 4.0.0-rc-4: Runs with -Xmx1024m in 2'32" (cannot run with -Xmx512m) - Maven 4.0.0-rc-4 with -Xmx1536m: 2'5" - Maven 4.0.0-rc-4 + maven3Personality with -Xmx1536m: 1'14" - Maven 4 + this PR: Runs with -Xmx512m in 3'42" (more memory does not help) - Maven 4 + this PR + maven3Personality: Runs with -Xmx512m in 1'0" - Reduced minimum memory requirement from 1024m to 512m - Eliminated memory scaling issues - additional memory beyond 512m provides no benefit - Significant reduction in memory pressure through improved caching strategies This PR definitively solves memory problems while maintaining or improving performance.
dee7262
to
ad062e9
Compare
Remove unused imports: - WeakHashMap from AbstractSession.java - InputSource from DefaultModelBuilder.java These imports were not being used and were flagged by spotless during the rebase process.
The project has removed the Hamcrest dependency, so we need to replace: - assertThat(value, greaterThan(0)) with assertTrue(value > 0) - assertThat(value, containsString(text)) with assertTrue(value.contains(text)) - assertThat(message, value, matcher) with assertTrue(condition, message) This ensures compatibility with the current Maven codebase that no longer includes Hamcrest.
Improve test failure diagnostics by adding meaningful error messages to assertions: - Profile location null checks now include profile ID in error message - Line/column number assertions show actual values and profile context - Source location assertions display expected vs actual paths - External profile assertions clearly identify the profile being tested These improvements will make debugging test failures much easier by providing: - Clear context about which profile is failing - Actual values that caused the assertion to fail - Expected vs actual comparisons for path assertions - Distinction between POM-based and external profiles This enhances the developer experience when tests fail and makes it easier to identify the root cause of assertion failures.
SummaryMaven Performance AnalysisStandard Maven Versions Comparison
Key Findings (Standard Versions):
Maven3-Personality Versions Comparison
Key Findings (Maven3-Personality):
Performance Impact AnalysisMemory Efficiency:
Execution Speed:
Maven3-Personality Impact:The Maven3-personality configuration provides:
Current Maven 4 Advantages:Maven 4 Current demonstrates:
Results
|
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java
Outdated
Show resolved
Hide resolved
api/maven-api-model/src/main/java/org/apache/maven/api/model/ModelObjectProcessor.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CacheSelectorParser.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CacheSelectorParser.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/cache/CacheSelectorParser.java
Outdated
Show resolved
Hide resolved
softMap.size(); | ||
} | ||
|
||
// The map should eventually clean up the garbage collected entries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to a separate test suite or IT or something? I really don't want to introduce known flaky tests
impl/maven-impl/src/test/java/org/apache/maven/impl/cache/ReferenceTypeStatisticsTest.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/test/java/org/apache/maven/impl/cache/ReferenceTypeStatisticsTest.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/test/java/org/apache/maven/impl/cache/ReferenceTypeStatisticsTest.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelObjectPoolTest.java
Outdated
Show resolved
Hide resolved
This is awesome |
This comprehensive enhancement to Maven's caching system addresses memory issues and significantly improves performance through several key improvements.
Key Features:
Technical Highlights: