[GitHub] [maven-resolver] tawek commented on a change in pull request #22: Introduce caching for tracking files.

[GitBox <gi...@apache.org>]

tawek commented on a change in pull request #22: Introduce caching for tracking files.
URL: https://github.com/apache/maven-resolver/pull/22#discussion_r271065809
 
 

 ##########
 File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/TrackingFileManager.java
 ##########
 @@ -131,52 +207,145 @@ public Properties update( File file, Map<String, String> updates )
                 raf.seek( 0 );
                 raf.write( stream.toByteArray() );
                 raf.setLength( raf.getFilePointer() );
+
+                this.entryTs = System.currentTimeMillis();
+                this.lastModifiedTs = file.lastModified();
             }
             catch ( IOException e )
             {
                 LOGGER.warn( "Failed to write tracking file {}", file, e );
             }
             finally
             {
-                release( lock, file );
-                close( raf, file );
+                release( lock );
+                close( raf );
             }
-        }
 
-        return props;
-    }
+            return props;
+        }
 
-    private void release( FileLock lock, File file )
-    {
-        if ( lock != null )
+        private void release( FileLock lock )
         {
-            try
+            if ( lock != null )
             {
-                lock.release();
+                try
+                {
+                    lock.release();
+                }
+                catch ( IOException e )
+                {
+                    LOGGER.warn( "Error releasing lock for tracking file {}", file, e );
+                }
             }
-            catch ( IOException e )
+        }
+
+        private void close( Closeable closeable )
+        {
+            if ( closeable != null )
             {
-                LOGGER.warn( "Error releasing lock for tracking file {}", file, e );
+                try
+                {
+                    closeable.close();
+                }
+                catch ( IOException e )
+                {
+                    LOGGER.warn( "Error closing tracking file {}", file, e );
+                }
             }
         }
-    }
 
-    private void close( Closeable closeable, File file )
-    {
-        if ( closeable != null )
+        private FileLock lock( FileChannel channel, long size, boolean shared ) throws IOException
         {
-            try
+            FileLock lock = null;
+
+            for ( int attempts = 8; attempts >= 0; attempts-- )
             {
-                closeable.close();
+                try
+                {
+                    lock = channel.lock( 0, size, shared );
+                    break;
+                }
+                catch ( OverlappingFileLockException e )
+                {
+                    if ( attempts <= 0 )
+                    {
+                        throw (IOException) new IOException().initCause( e );
+                    }
+                    try
+                    {
+                        Thread.sleep( 50L );
+                    }
+                    catch ( InterruptedException e1 )
+                    {
+                        Thread.currentThread().interrupt();
+                    }
+                }
             }
-            catch ( IOException e )
+
+            if ( lock == null )
             {
-                LOGGER.warn( "Error closing tracking file {}", file, e );
+                throw new IOException( "Could not lock file" );
             }
+
+            return lock;
+        }
+
+    }
+
+    /**
+     * Canonicalized files by their path (the same canonicalized file may be registered under different paths). This
+     * cache is especially useful on Windows platform where canonicalization is relatively expensive.
+     */
+    private static ConcurrentHashMap<String, File> canonicalizedCache = new ConcurrentHashMap<>();
 
 Review comment:
   Glad to here there is some interest in merging either this or #29. 
   
   As to split "this and canonicalization" - I understand that we need to have a different PR on canonicalization cache (not sure why this would bring any benefit...).
   
   As to "I need JIRA" - as there is already MRESOLVER-68 so I may hijack it or create two new JIRAs tickets (assuming there is a real need to split). Please advise.
   
   For the weakly reachable keys in the canonicalization cache hash map I understand the concern for memory. But since WeakHashMap expiry is based on key object identity it effectively means that caching for canonicalization may not work unless there is String deduplication enabled on JVM. Please see the javadoc for WeakHashMap. For File its even worse since those handles are not deduplicated in any way as far as I know and the caches would be flushed clean on gc (probably depends on collector impl and finalizer threads/reference queues).
   
   There is already a canonicalization cache in JVM for Windows (!) - see 'ExpiringCache.java' it is just limited to 200 entries FIFO and 30s. I've found it to be ineffective - too small and too time-limited. Therefore I haven't put a time nor size constraint as indicated in TODO but I agree there should be some size bound just in case of some runaway build ...
   
   My choice therefore would be to make LRU cache with some 10000 entries bound (not to be reached ever - just in case of runaway programs) with TTL of couple of minutes (threshold at which a human will no longer be willing to wait for m2e and will just make the projects smaller :) )

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services