diff options
| -rw-r--r-- | MediaBrowser.Controller/Providers/DirectoryService.cs | 33 | ||||
| -rw-r--r-- | tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs | 61 |
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)); + } } } |