From 331e402b1a7f6e158444984d1272aeff18f54549 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Fri, 27 Nov 2020 10:36:42 +0100 Subject: [PATCH] Do not use a "persistent" cache which is the root cause MNG-6530 The root of the problem is MNG-5312 which is a performance issue because a ModelCache was not always involved during a model build. This lead to MNG-6030 (memory consumption issue because of the usage of multiple caches). The fix for MNG-6030 lead to MNG-6311 (problem with changes not being picked up in M2E). The root problem is that the fix for MNG-5312 was wrong. The caches and its data should be managed at the RepositorySystemSession level, especially as the data is related to a given session parameters (maven global settings). This commit brings back the cache at the session level, and a default cache is now set on the session by the MavenCli. The cache is used later by DefaultArtifactDescriptorReader and DefaultProjectBuilder to build a DefaultModelCache. * use the cache on the MavenExecutionRequest * get rid of the hack for MNG-6530. * MavenCli configures a cache for the whole execution request * merge ReactorModelCache into DefaultModelCache --- .../maven/project/DefaultProjectBuilder.java | 36 ++-- .../maven/project/ReactorModelCache.java | 167 ------------------ .../maven/project/ProjectBuilderTest.java | 9 - .../java/org/apache/maven/cli/MavenCli.java | 6 + .../internal/DefaultModelCache.java | 158 +++++++++++++---- 5 files changed, 141 insertions(+), 235 deletions(-) delete mode 100644 maven-core/src/main/java/org/apache/maven/project/ReactorModelCache.java diff --git a/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java b/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java index 73f3ed5b545c..86b9535d8172 100644 --- a/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java +++ b/maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuilder.java @@ -63,6 +63,7 @@ import org.apache.maven.model.building.ModelBuildingException; import org.apache.maven.model.building.ModelBuildingRequest; import org.apache.maven.model.building.ModelBuildingResult; +import org.apache.maven.model.building.ModelCache; import org.apache.maven.model.building.ModelProblem; import org.apache.maven.model.building.ModelProcessor; import org.apache.maven.model.building.ModelSource; @@ -70,6 +71,7 @@ import org.apache.maven.model.building.TransformerContext; import org.apache.maven.model.resolution.ModelResolver; import org.apache.maven.repository.internal.ArtifactDescriptorUtils; +import org.apache.maven.repository.internal.DefaultModelCache; import org.codehaus.plexus.logging.Logger; import org.codehaus.plexus.util.Os; import org.codehaus.plexus.util.StringUtils; @@ -91,9 +93,6 @@ public class DefaultProjectBuilder implements ProjectBuilder { - public static final String DISABLE_GLOBAL_MODEL_CACHE_SYSTEM_PROPERTY = - "maven.defaultProjectBuilder.disableGlobalModelCache"; - private final Logger logger; private final ModelBuilder modelBuilder; @@ -110,8 +109,6 @@ public class DefaultProjectBuilder private final ProjectDependenciesResolver dependencyResolver; - private final ReactorModelCache modelCache; - @Inject @SuppressWarnings( "checkstyle:parameternumber" ) public DefaultProjectBuilder( @@ -133,7 +130,6 @@ public DefaultProjectBuilder( this.repoSystem = repoSystem; this.repositoryManager = repositoryManager; this.dependencyResolver = dependencyResolver; - this.modelCache = new ReactorModelCache(); } // ---------------------------------------------------------------------- @@ -145,12 +141,7 @@ public ProjectBuildingResult build( File pomFile, ProjectBuildingRequest request throws ProjectBuildingException { return build( pomFile, new FileModelSource( pomFile ), - new InternalConfig( request, null, useGlobalModelCache() ? getModelCache() : null ) ); - } - - private boolean useGlobalModelCache() - { - return !Boolean.getBoolean( DISABLE_GLOBAL_MODEL_CACHE_SYSTEM_PROPERTY ); + new InternalConfig( request, null ) ); } @Override @@ -158,7 +149,7 @@ public ProjectBuildingResult build( ModelSource modelSource, ProjectBuildingRequ throws ProjectBuildingException { return build( null, modelSource, - new InternalConfig( request, null, useGlobalModelCache() ? getModelCache() : null ) ); + new InternalConfig( request, null ) ); } private ProjectBuildingResult build( File pomFile, ModelSource modelSource, InternalConfig config ) @@ -301,6 +292,8 @@ private ModelBuildingRequest getModelBuildingRequest( InternalConfig config ) new ProjectModelResolver( config.session, trace, repoSystem, repositoryManager, config.repositories, configuration.getRepositoryMerging(), config.modelPool ); + ModelCache modelCache = DefaultModelCache.newInstance( config.session ); + request.setValidationLevel( configuration.getValidationLevel() ); request.setProcessPlugins( configuration.isProcessPlugins() ); request.setProfiles( configuration.getProfiles() ); @@ -310,7 +303,7 @@ private ModelBuildingRequest getModelBuildingRequest( InternalConfig config ) request.setUserProperties( configuration.getUserProperties() ); request.setBuildStartTime( configuration.getBuildStartTime() ); request.setModelResolver( resolver ); - request.setModelCache( config.modelCache ); + request.setModelCache( modelCache ); request.setTransformerContext( (TransformerContext) config.session.getData().get( TransformerContext.KEY ) ); return request; @@ -330,7 +323,7 @@ public ProjectBuildingResult build( Artifact artifact, boolean allowStubModel, P org.eclipse.aether.artifact.Artifact pomArtifact = RepositoryUtils.toArtifact( artifact ); pomArtifact = ArtifactDescriptorUtils.toPomArtifact( pomArtifact ); - InternalConfig config = new InternalConfig( request, null, useGlobalModelCache() ? getModelCache() : null ); + InternalConfig config = new InternalConfig( request, null ); boolean localProject; @@ -427,8 +420,7 @@ public Model getRawModel( String groupId, String artifactId ) request.getRepositorySession().getData().set( TransformerContext.KEY, context ); } - InternalConfig config = new InternalConfig( request, modelPool, - useGlobalModelCache() ? getModelCache() : new ReactorModelCache() ); + InternalConfig config = new InternalConfig( request, modelPool ); Map projectIndex = new HashMap<>( 256 ); @@ -1110,13 +1102,10 @@ class InternalConfig private final ReactorModelPool modelPool; - private final ReactorModelCache modelCache; - - InternalConfig( ProjectBuildingRequest request, ReactorModelPool modelPool, ReactorModelCache modelCache ) + InternalConfig( ProjectBuildingRequest request, ReactorModelPool modelPool ) { this.request = request; this.modelPool = modelPool; - this.modelCache = modelCache; session = LegacyLocalRepositoryManager.overlay( request.getLocalRepository(), request.getRepositorySession(), repoSystem ); @@ -1125,9 +1114,4 @@ class InternalConfig } - private ReactorModelCache getModelCache() - { - return this.modelCache; - } - } diff --git a/maven-core/src/main/java/org/apache/maven/project/ReactorModelCache.java b/maven-core/src/main/java/org/apache/maven/project/ReactorModelCache.java deleted file mode 100644 index 72c25c79f888..000000000000 --- a/maven-core/src/main/java/org/apache/maven/project/ReactorModelCache.java +++ /dev/null @@ -1,167 +0,0 @@ -package org.apache.maven.project; - -/* - * 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. - */ - -import org.apache.maven.building.Source; -import org.apache.maven.model.building.ModelCache; - -import java.util.Map; -import java.util.Objects; -import java.util.concurrent.ConcurrentHashMap; - -/** - * A simple model cache used to accelerate model building during a reactor build. - * - * @author Benjamin Bentmann - */ -class ReactorModelCache - implements ModelCache -{ - - private final Map models = new ConcurrentHashMap<>( 256 ); - - @Override - public Object get( String groupId, String artifactId, String version, String tag ) - { - return models.get( new GavCacheKey( groupId, artifactId, version, tag ) ); - } - - @Override - public void put( String groupId, String artifactId, String version, String tag, Object data ) - { - models.put( new GavCacheKey( groupId, artifactId, version, tag ), data ); - } - - @Override - public Object get( Source source, String tag ) - { - return models.get( new SourceCacheKey( source, tag ) ); - } - - @Override - public void put( Source source, String tag, Object data ) - { - models.put( new SourceCacheKey( source, tag ), data ); - } - - private static final class GavCacheKey - { - - private final String groupId; - - private final String artifactId; - - private final String version; - - private final String tag; - - private final int hashCode; - - GavCacheKey( String groupId, String artifactId, String version, String tag ) - { - this.groupId = ( groupId != null ) ? groupId : ""; - this.artifactId = ( artifactId != null ) ? artifactId : ""; - this.version = ( version != null ) ? version : ""; - this.tag = ( tag != null ) ? tag : ""; - - int hash = 17; - hash = hash * 31 + this.groupId.hashCode(); - hash = hash * 31 + this.artifactId.hashCode(); - hash = hash * 31 + this.version.hashCode(); - hash = hash * 31 + this.tag.hashCode(); - hashCode = hash; - } - - @Override - public boolean equals( Object obj ) - { - if ( this == obj ) - { - return true; - } - - if ( !( obj instanceof GavCacheKey ) ) - { - return false; - } - - GavCacheKey that = (GavCacheKey) obj; - - return artifactId.equals( that.artifactId ) && groupId.equals( that.groupId ) - && version.equals( that.version ) && tag.equals( that.tag ); - } - - @Override - public int hashCode() - { - return hashCode; - } - - } - - private static final class SourceCacheKey - { - private final Source source; - - private final String tag; - - private final int hashCode; - - SourceCacheKey( Source source, String tag ) - { - this.source = source; - this.tag = tag; - this.hashCode = Objects.hash( source, tag ); - } - - @Override - public int hashCode() - { - return hashCode; - } - - @Override - public boolean equals( Object obj ) - { - if ( this == obj ) - { - return true; - } - if ( !( obj instanceof SourceCacheKey ) ) - { - return false; - } - - SourceCacheKey other = (SourceCacheKey) obj; - if ( !Objects.equals( this.source, other.source ) ) - { - return false; - } - - if ( !Objects.equals( this.tag, other.tag ) ) - { - return false; - } - - return true; - } - } - -} diff --git a/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java b/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java index 74c19a8edf3e..6c54c57fa442 100644 --- a/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java +++ b/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java @@ -140,7 +140,6 @@ public void testDontResolveDependencies() } public void testReadModifiedPoms() throws Exception { - String initialValue = System.setProperty( DefaultProjectBuilder.DISABLE_GLOBAL_MODEL_CACHE_SYSTEM_PROPERTY, Boolean.toString( true ) ); // TODO a similar test should be created to test the dependency management (basically all usages // of DefaultModelBuilder.getCache() are affected by MNG-6530 @@ -167,14 +166,6 @@ public void testReadModifiedPoms() throws Exception { } finally { - if ( initialValue == null ) - { - System.clearProperty( DefaultProjectBuilder.DISABLE_GLOBAL_MODEL_CACHE_SYSTEM_PROPERTY ); - } - else - { - System.setProperty( DefaultProjectBuilder.DISABLE_GLOBAL_MODEL_CACHE_SYSTEM_PROPERTY, initialValue ); - } FileUtils.deleteDirectory( tempDir.toFile() ); } } diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java b/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java index fd650f0d369c..35c5c52e57e2 100644 --- a/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java +++ b/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java @@ -82,6 +82,7 @@ import org.codehaus.plexus.logging.LoggerManager; import org.codehaus.plexus.util.StringUtils; import org.codehaus.plexus.util.xml.pull.XmlPullParserException; +import org.eclipse.aether.DefaultRepositoryCache; import org.eclipse.aether.transfer.TransferListener; import org.slf4j.ILoggerFactory; import org.slf4j.Logger; @@ -977,6 +978,11 @@ private int execute( CliRequest cliRequest ) { MavenExecutionRequest request = executionRequestPopulator.populateDefaults( cliRequest.request ); + if ( cliRequest.request.getRepositoryCache() == null ) + { + cliRequest.request.setRepositoryCache( new DefaultRepositoryCache() ); + } + eventSpyDispatcher.onEvent( request ); MavenExecutionResult result = maven.execute( request ); diff --git a/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelCache.java b/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelCache.java index 86edf16fffdb..b09971f65d71 100644 --- a/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelCache.java +++ b/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultModelCache.java @@ -19,8 +19,12 @@ * under the License. */ +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; + +import org.apache.maven.building.Source; import org.apache.maven.model.building.ModelCache; -import org.eclipse.aether.RepositoryCache; import org.eclipse.aether.RepositorySystemSession; /** @@ -28,68 +32,107 @@ * * @author Benjamin Bentmann */ -class DefaultModelCache - implements ModelCache +public class DefaultModelCache implements ModelCache { - private final RepositorySystemSession session; + private static final String KEY = DefaultModelCache.class.getName(); - private final RepositoryCache cache; + private final Map cache; public static ModelCache newInstance( RepositorySystemSession session ) { + Map cache; if ( session.getCache() == null ) { - return null; + cache = new ConcurrentHashMap<>(); } else { - return new DefaultModelCache( session ); + cache = ( Map ) session.getCache().get( session, KEY ); + if ( cache == null ) + { + cache = new ConcurrentHashMap<>(); + session.getCache().put( session, KEY, cache ); + } } + return new DefaultModelCache( cache ); + } + + private DefaultModelCache( Map cache ) + { + this.cache = cache; + } + + @Override + public Object get( Source path, String tag ) + { + return get( new SourceCacheKey( path, tag ) ); } - private DefaultModelCache( RepositorySystemSession session ) + @Override + public void put( Source path, String tag, Object data ) { - this.session = session; - this.cache = session.getCache(); + put( new SourceCacheKey( path, tag ), data ); } public Object get( String groupId, String artifactId, String version, String tag ) { - return cache.get( session, new Key( groupId, artifactId, version, tag ) ); + return get( new GavCacheKey( groupId, artifactId, version, tag ) ); } public void put( String groupId, String artifactId, String version, String tag, Object data ) { - cache.put( session, new Key( groupId, artifactId, version, tag ), data ); + put( new GavCacheKey( groupId, artifactId, version, tag ), data ); } - static class Key + protected Object get( Object key ) { + return cache.get( key ); + } - private final String groupId; + protected void put( Object key, Object data ) + { + cache.put( key, data ); + } - private final String artifactId; + private static final class GavCacheKey + { - private final String version; + private final String gav; private final String tag; - private final int hash; + private final int hashCode; - Key( String groupId, String artifactId, String version, String tag ) + GavCacheKey( String groupId, String artifactId, String version, String tag ) { - this.groupId = groupId; - this.artifactId = artifactId; - this.version = version; - this.tag = tag; + StringBuilder sb = new StringBuilder(); + if ( groupId != null ) + { + sb.append( groupId ); + } + sb.append( ":" ); + if ( artifactId != null ) + { + sb.append( artifactId ); + } + sb.append( ":" ); + if ( version != null ) + { + sb.append( version ); + } + this.gav = sb.toString(); + this.tag = ( tag != null ) ? tag : ""; + this.hashCode = Objects.hash( gav, tag ); + } - int h = 17; - h = h * 31 + this.groupId.hashCode(); - h = h * 31 + this.artifactId.hashCode(); - h = h * 31 + this.version.hashCode(); - h = h * 31 + this.tag.hashCode(); - hash = h; + @Override + public String toString() + { + return "GavCacheKey{" + + "gav='" + gav + '\'' + + ", tag='" + tag + '\'' + + '}'; } @Override @@ -99,21 +142,70 @@ public boolean equals( Object obj ) { return true; } - if ( null == obj || !getClass().equals( obj.getClass() ) ) + if ( !( obj instanceof GavCacheKey ) ) { return false; } - Key that = (Key) obj; - return artifactId.equals( that.artifactId ) && groupId.equals( that.groupId ) - && version.equals( that.version ) && tag.equals( that.tag ); + GavCacheKey that = (GavCacheKey) obj; + return Objects.equals( gav, that.gav ) + && Objects.equals( tag, that.tag ); } @Override public int hashCode() { - return hash; + return hashCode; + } + + } + + private static final class SourceCacheKey + { + private final Source source; + + private final String tag; + + private final int hashCode; + + SourceCacheKey( Source source, String tag ) + { + this.source = source; + this.tag = tag; + this.hashCode = Objects.hash( source, tag ); + } + + @Override + public String toString() + { + return "SourceCacheKey{" + + "source=" + source + + ", tag='" + tag + '\'' + + '}'; + } + + @Override + public boolean equals( Object obj ) + { + if ( this == obj ) + { + return true; + } + if ( !( obj instanceof SourceCacheKey ) ) + { + return false; + } + + SourceCacheKey that = (SourceCacheKey) obj; + return Objects.equals( this.source, that.source ) + && Objects.equals( this.tag, that.tag ); } + @Override + public int hashCode() + { + return hashCode; + } } + }