[MRESOLVER-68] Add cache around TrackingFileManager.getLock(File)

maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java

@@ -144,6 +144,48 @@ public final class ConfigurationProperties
      */
     public static final boolean DEFAULT_PERSISTED_CHECKSUMS = true;
 
+    /**
+     * The prefix for configuration for internal components.
+     */
+    private static final String PREFIX_INTERNAL = PREFIX_AETHER + "internal.";
+
+    /**
+     * The size of the file lock name cache, used by the TrackingFileManager to map Files to their interned, canonical
+     * names.
+     *
+     * @see #DEFAULT_TRACKING_FILE_MANAGER_FILE_LOCK_CACHE_SIZE
+     */
+    public static final String TRACKING_FILE_MANAGER_FILE_LOCK_CACHE_SIZE = PREFIX_INTERNAL + "trackingFileManager.fileLockCacheSize";
+
+    /**
+     * The default size if {@link #TRACKING_FILE_MANAGER_FILE_LOCK_CACHE_SIZE} isn't set.
+     */
+    public static final long DEFAULT_TRACKING_FILE_MANAGER_FILE_LOCK_CACHE_SIZE = 1024L;
+
+    /**
+     * The size of the properties cache, used by the TrackingFileManager to map Files to their parsed properties.
+     *
+     * @see #DEFAULT_TRACKING_FILE_MANAGER_PROPERTIES_CACHE_SIZE
+     */
+    public static final String TRACKING_FILE_MANAGER_FILE_PROPERTIES_CACHE_SIZE = PREFIX_INTERNAL + "trackingFileManager.propertiesCacheSize";
+
+    /**
+     * The default size if {@link #TRACKING_FILE_MANAGER_FILE_PROPERTIES_CACHE_SIZE} isn't set.
+     */
+    public static final long DEFAULT_TRACKING_FILE_MANAGER_PROPERTIES_CACHE_SIZE = 1024L;
+
+    /**
+     * The number of seconds to keep entries in the properties cache, used by the TrackingFileManager to map Files to their parsed properties.
+     *
+     * @see #DEFAULT_TRACKING_FILE_MANAGER_PROPERTIES_CACHE_SIZE
+     */
+    public static final String TRACKING_FILE_MANAGER_FILE_PROPERTIES_EXPIRE_AFTER_ACCESS = PREFIX_INTERNAL + "trackingFileManager.propertiesExpireAfterWrite";
+
+    /**
+     * The default number of seconds if {@link #TRACKING_FILE_MANAGER_FILE_PROPERTIES_EXPIRE_AFTER_ACCESS} isn't set.
+     */
+    public static final long DEFAULT_TRACKING_FILE_MANAGER_PROPERTIES_EXPIRE_AFTER_ACCESS = 60L;
+
     private ConfigurationProperties()
     {
         // hide constructor

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/EnhancedLocalRepositoryManager.java

@@ -75,7 +75,7 @@ class EnhancedLocalRepositoryManager
             filename = "_remote.repositories";
         }
         trackingFilename = filename;
-        trackingFileManager = new TrackingFileManager();
+        trackingFileManager = new TrackingFileManager( session );
     }
 
     @Override

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/TrackingFileManager.java

@@ -19,6 +19,13 @@
  * under the License.
  */
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import org.eclipse.aether.ConfigurationProperties;
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.util.ConfigUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -32,8 +39,10 @@
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
 import java.nio.channels.OverlappingFileLockException;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Properties;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Manages potentially concurrent accesses to a properties file.
@@ -42,41 +51,110 @@ class TrackingFileManager
 {
 
     private static final Logger LOGGER = LoggerFactory.getLogger( TrackingFileManager.class );
+    /** Cache mapping Files to their canonical names, for use as a lock. */
+    private final LoadingCache<File, Object> fileLockCache;
+    /** Cache mapping Files to their parsed properties. */
+    private final LoadingCache<File, Properties> propertiesCache;
+    /** Marker for nulls in the cache. Guava doesn't allow null return values from CacheLoaders. */
+    private final Properties NULL_PROPERTIES = new Properties();
 
-    public Properties read( File file )
+    TrackingFileManager()
     {
-        synchronized ( getLock( file ) )
-        {
-            FileLock lock = null;
-            FileInputStream stream = null;
-            try
-            {
-                if ( !file.exists() )
-                {
-                    return null;
-                }
+        this( Collections.emptyMap() );
+    }
 
-                stream = new FileInputStream( file );
+    TrackingFileManager( RepositorySystemSession session )
+    {
+        this( session.getConfigProperties() );
+    }
 
-                lock = lock( stream.getChannel(), Math.max( 1, file.length() ), true );
+    TrackingFileManager( Map<?, ?> configurationProperties )
+    {
+        long lockCacheMaxSize = ConfigUtils.getLong( configurationProperties,
+                ConfigurationProperties.DEFAULT_TRACKING_FILE_MANAGER_FILE_LOCK_CACHE_SIZE,
+                ConfigurationProperties.TRACKING_FILE_MANAGER_FILE_LOCK_CACHE_SIZE );
+
+        fileLockCache = CacheBuilder.newBuilder()
+                .maximumSize( lockCacheMaxSize )
+                .build( new CacheLoader<File, Object>()
+        {
+            @Override
+            public Object load( File file )
+            {
+                return getLockInternal( file );
+            }
+        } );
 
-                Properties props = new Properties();
-                props.load( stream );
+        long propertiesCacheMaxSize = ConfigUtils.getLong( configurationProperties,
+                ConfigurationProperties.DEFAULT_TRACKING_FILE_MANAGER_PROPERTIES_CACHE_SIZE,
+                ConfigurationProperties.TRACKING_FILE_MANAGER_FILE_PROPERTIES_CACHE_SIZE );
+        long propertiesCacheExpireAfterAccess = ConfigUtils.getLong( configurationProperties,
+                ConfigurationProperties.DEFAULT_TRACKING_FILE_MANAGER_PROPERTIES_EXPIRE_AFTER_ACCESS,
+                ConfigurationProperties.TRACKING_FILE_MANAGER_FILE_PROPERTIES_EXPIRE_AFTER_ACCESS );
 
-                return props;
+        propertiesCache = CacheBuilder.newBuilder()
+                .maximumSize( propertiesCacheMaxSize )
+                .expireAfterAccess( propertiesCacheExpireAfterAccess, TimeUnit.SECONDS )
+                .build( new CacheLoader<File, Properties>()
+        {
+            @Override
+            public Properties load( File file )
+            {
+                return readInternal( file );
             }
-            catch ( IOException e )
+        } );
+    }
+
+    public Properties read( File file )
+    {
+        synchronized ( getLock( file ) )
+        {
+            Properties props = propertiesCache.getUnchecked( file );
+            if ( props != NULL_PROPERTIES )
             {
-                LOGGER.warn( "Failed to read tracking file {}", file, e );
+                return ( Properties ) props.clone();
             }
-            finally
+        }
+        return null;
+    }
+
+    @VisibleForTesting
+    LoadingCache<File, Properties> getPropertiesCache()
+    {
+        return propertiesCache;
+    }
+
+    private Properties readInternal( File file )
+    {
+        FileLock lock = null;
+        FileInputStream stream = null;
+        try
+        {
+            if ( !file.exists() )
             {
-                release( lock, file );
-                close( stream, file );
+                return NULL_PROPERTIES;
             }
+
+            stream = new FileInputStream( file );
+
+            lock = lock( stream.getChannel(), Math.max( 1, file.length() ), true );
+
+            Properties props = new Properties();
+            props.load( stream );
+
+            return props;
+        }
+        catch ( IOException e )
+        {
+            LOGGER.warn( "Failed to read tracking file {}", file, e );
+        }
+        finally
+        {
+            release( lock, file );
+            close( stream, file );
         }
 
-        return null;
+        return NULL_PROPERTIES;
     }
 
     public Properties update( File file, Map<String, String> updates )
@@ -140,6 +218,7 @@ public Properties update( File file, Map<String, String> updates )
             {
                 release( lock, file );
                 close( raf, file );
+                propertiesCache.invalidate( file );
             }
         }
 
@@ -177,6 +256,17 @@ private void close( Closeable closeable, File file )
     }
 
     private Object getLock( File file )
+    {
+        return fileLockCache.getUnchecked( file );
+    }
+
+    @VisibleForTesting
+    LoadingCache<File, Object> getFileLockCache()
+    {
+        return fileLockCache;
+    }
+
+    private Object getLockInternal( File file )
     {
         /*
          * NOTE: Locks held by one JVM must not overlap and using the canonical path is our best bet, still another

maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/TrackingFileManagerTest.java

@@ -30,7 +30,7 @@
 import java.util.Map;
 import java.util.Properties;
 
-import org.eclipse.aether.internal.impl.TrackingFileManager;
+import org.eclipse.aether.ConfigurationProperties;
 import org.eclipse.aether.internal.test.util.TestFileUtils;
 import org.junit.Test;
 
@@ -54,9 +54,11 @@ public void testRead()
         assertEquals( "value2", props.get( "key2" ) );
 
         assertTrue( "Leaked file: " + propFile, propFile.delete() );
+        tfm.getPropertiesCache().invalidate( propFile );
 
         props = tfm.read( propFile );
         assertNull( String.valueOf( props ), props );
+        assertEquals( 1, tfm.getFileLockCache().asMap().size() );
     }
 
     @Test
@@ -70,7 +72,12 @@ public void testReadNoFileLeak()
             File propFile = TestFileUtils.createTempFile( "#COMMENT\nkey1=value1\nkey2 : value2" );
             assertNotNull( tfm.read( propFile ) );
             assertTrue( "Leaked file: " + propFile, propFile.delete() );
+            tfm.getPropertiesCache().invalidate( propFile );
         }
+
+        int size = tfm.getFileLockCache().asMap().size();
+        assertTrue(size > 900);
+        assertTrue(size <= 1000);
     }
 
     @Test
@@ -87,13 +94,15 @@ public void testUpdate()
         updates.put( "key2", null );
 
         tfm.update( propFile, updates );
+        assertEquals( 1, tfm.getFileLockCache().asMap().size() );
 
         Properties props = tfm.read( propFile );
 
         assertNotNull( props );
         assertEquals( String.valueOf( props ), 1, props.size() );
         assertEquals( "v", props.get( "key1" ) );
         assertNull( String.valueOf( props.get( "key2" ) ), props.get( "key2" ) );
+        assertEquals( 1, tfm.getFileLockCache().asMap().size() );
     }
 
     @Test
@@ -111,6 +120,9 @@ public void testUpdateNoFileLeak()
             assertNotNull( tfm.update( propFile, updates ) );
             assertTrue( "Leaked file: " + propFile, propFile.delete() );
         }
+        int size = tfm.getFileLockCache().asMap().size();
+        assertTrue(size > 900);
+        assertTrue(size <= 1000);
     }
 
     @Test
@@ -164,6 +176,23 @@ public void run()
         }
 
         assertEquals( Collections.emptyList(), errors );
+        assertEquals( 4, tfm.getFileLockCache().asMap().size() );
     }
 
+    @Test
+    public void testCacheSizeConfig()
+            throws Exception
+    {
+        TrackingFileManager tfm = new TrackingFileManager(
+                Collections.singletonMap( ConfigurationProperties.TRACKING_FILE_MANAGER_FILE_LOCK_CACHE_SIZE, 5L )
+        );
+
+        for ( int i = 0; i < 1000; i++ )
+        {
+            File propFile = TestFileUtils.createTempFile( "#COMMENT\nkey1=value1\nkey2 : value2" );
+            assertNotNull( tfm.read( propFile ) );
+        }
+
+        assertEquals( 5, tfm.getFileLockCache().asMap().size() );
+    }
 }