Bound the directory caches a singleton would otherwise hold for the process lifetime

2 files changed, 84 insertions, 10 deletions

diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs
index 6060d051a5..43f0f11dfe 100644
--- a/MediaBrowser.Controller/Providers/DirectoryService.cs
+++ b/MediaBrowser.Controller/Providers/DirectoryService.cs
@@ -1,22 +1,32 @@
 #pragma warning disable CS1591
 
 using System;
-using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using BitFaster.Caching.Lru;
 using MediaBrowser.Model.IO;
 
 namespace MediaBrowser.Controller.Providers
 {
     public class DirectoryService : IDirectoryService
     {
-        // TODO make static and switch to FastConcurrentLru.
-        private readonly ConcurrentDictionary<string, FileSystemMetadata[]> _cache = new(StringComparer.Ordinal);
+        private const int DirectoryCacheSize = 2048;
+        private const int FileCacheSize = 4096;
 
-        private readonly ConcurrentDictionary<string, FileSystemMetadata> _fileCache = new(StringComparer.Ordinal);
+        // A bounded LRU sizes its table up front, so it costs several kilobytes while still empty.
+        // One instance is a DI singleton and lives for the process, but the library code also news
+        // one up per item in several loops and hands it to QueueRefresh, which holds on to it until
+        // the refresh runs. Those instances usually ask about a single path, so each cache waits
+        // until something actually looks in it.
+        private readonly Lazy<FastConcurrentLru<string, FileSystemMetadata[]>> _cache
+            = new(static () => new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal));
 
-        private readonly ConcurrentDictionary<string, List<string>> _filePathCache = new(StringComparer.Ordinal);
+        private readonly Lazy<FastConcurrentLru<string, FileSystemMetadata>> _fileCache
+            = new(static () => new(Environment.ProcessorCount, FileCacheSize, StringComparer.Ordinal));
+
+        private readonly Lazy<FastConcurrentLru<string, List<string>>> _filePathCache
+            = new(static () => new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal));
 
         private readonly IFileSystem _fileSystem;
 
@@ -27,7 +37,7 @@ namespace MediaBrowser.Controller.Providers
 
         public FileSystemMetadata[] GetFileSystemEntries(string path)
         {
-            return _cache.GetOrAdd(
+            return _cache.Value.GetOrAdd(
                 path,
                 static (p, fileSystem) =>
                 {
@@ -89,13 +99,16 @@ namespace MediaBrowser.Controller.Providers
 
         public FileSystemMetadata? GetFileSystemEntry(string path)
         {
-            if (!_fileCache.TryGetValue(path, out var result))
+            if (!_fileCache.Value.TryGet(path, out var result))
             {
                 var file = _fileSystem.GetFileSystemInfo(path);
+
+                // Only a hit is remembered. A miss is the one answer that changes on its own, when
+                // the file the path names turns up.
                 if (file?.Exists ?? false)
                 {
                     result = file;
-                    _fileCache.TryAdd(path, result);
+                    _fileCache.Value.AddOrUpdate(path, result);
                 }
             }
 
@@ -109,10 +122,10 @@ namespace MediaBrowser.Controller.Providers
         {
             if (clearCache)
             {
-                _filePathCache.TryRemove(path, out _);
+                _filePathCache.Value.TryRemove(path, out _);
             }
 
-            var filePaths = _filePathCache.GetOrAdd(
+            var filePaths = _filePathCache.Value.GetOrAdd(
                 path,
                 static (p, fileSystem) =>
                 {
diff --git a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
index 1f59908a86..bc03bfc33b 100644
--- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
+++ b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
@@ -1,3 +1,4 @@
+using System.Globalization;
 using System.Linq;
 using MediaBrowser.Controller.Providers;
 using MediaBrowser.Model.IO;
@@ -248,5 +249,65 @@ namespace Jellyfin.Controller.Tests
             Assert.Equal(cachedPaths, result);
             Assert.Equal(newPaths, secondResult);
         }
+
+        [Fact]
+        public void GetFileSystemEntries_RepeatedPath_ReadsTheFileSystemOnce()
+        {
+            var fileSystemMock = new Mock<IFileSystem>(MockBehavior.Strict);
+            fileSystemMock.Setup(f => f.GetFileSystemEntries(LowerCasePath))
+                .Returns(_lowerCaseFileSystemMetadata);
+
+            var directoryService = new DirectoryService(fileSystemMock.Object);
+
+            directoryService.GetFileSystemEntries(LowerCasePath);
+            directoryService.GetFileSystemEntries(LowerCasePath);
+
+            fileSystemMock.Verify(f => f.GetFileSystemEntries(LowerCasePath), Times.Once);
+        }
+
+        [Fact]
+        public void GetFileSystemEntries_FarMorePathsThanTheCacheHolds_EvictsInsteadOfGrowing()
+        {
+            // The service is a singleton, so the cache has to give entries back rather than hold every
+            // path the server ever saw. Asking for far more paths than it can hold must push the first
+            // one out, which shows up as the file system being read for it a second time.
+            const int PathCount = 40000;
+
+            var fileSystemMock = new Mock<IFileSystem>();
+            fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny<string>()))
+                .Returns(_lowerCaseFileSystemMetadata);
+
+            var directoryService = new DirectoryService(fileSystemMock.Object);
+
+            var firstPath = "/music/artist0";
+            directoryService.GetFileSystemEntries(firstPath);
+
+            for (var i = 1; i < PathCount; i++)
+            {
+                directoryService.GetFileSystemEntries("/music/artist" + i.ToString(CultureInfo.InvariantCulture));
+            }
+
+            directoryService.GetFileSystemEntries(firstPath);
+
+            fileSystemMock.Verify(f => f.GetFileSystemEntries(firstPath), Times.Exactly(2));
+        }
+
+        [Fact]
+        public void GetFileSystemEntry_MissingPath_IsNotRemembered()
+        {
+            const string MissingPath = "/music/not-here";
+
+            var fileSystemMock = new Mock<IFileSystem>();
+            fileSystemMock.SetupSequence(f => f.GetFileSystemInfo(MissingPath))
+                .Returns(new FileSystemMetadata { FullName = MissingPath, Exists = false })
+                .Returns(new FileSystemMetadata { FullName = MissingPath, Exists = true });
+
+            var directoryService = new DirectoryService(fileSystemMock.Object);
+
+            Assert.Null(directoryService.GetFileSystemEntry(MissingPath));
+
+            // The one answer that changes on its own: the file turning up has to be visible.
+            Assert.NotNull(directoryService.GetFileSystemEntry(MissingPath));
+        }
     }
 }