From 88228102298956f019ab4d2d6b3d98077317346b Mon Sep 17 00:00:00 2001 From: srkizer Date: Sun, 25 Aug 2024 22:06:21 +0900 Subject: [PATCH] DalamudAssetManager: avoid locks and lookups (#2015) * Made DalamudAsset-to-something tables into arrays from dictionaries. Number of items in the DalamudAsset enum aren't many, and the numbers are small enough that implementing lookup tables as arrays aren't wasting much memory space. * Removed locking from asset accessors, while still guaranteeing that the load operation happens only once per asset. * ISharedImmediateTexture: made it not even access assets if textures are available. --- Dalamud/DalamudAsset.cs | 1 + .../SeStringDrawResult.cs | 2 +- .../SharedImmediateTexture.cs | 11 +- .../TextureWraps/ForwardingTextureWrap.cs | 6 +- .../Internal/DisposeSuppressingTextureWrap.cs | 13 +- .../Storage/Assets/DalamudAssetExtensions.cs | 51 ++-- Dalamud/Storage/Assets/DalamudAssetManager.cs | 227 ++++++++++-------- Dalamud/Storage/Assets/DalamudAssetPurpose.cs | 2 +- 8 files changed, 168 insertions(+), 145 deletions(-) diff --git a/Dalamud/DalamudAsset.cs b/Dalamud/DalamudAsset.cs index 0d91a4b75..27771116e 100644 --- a/Dalamud/DalamudAsset.cs +++ b/Dalamud/DalamudAsset.cs @@ -9,6 +9,7 @@ namespace Dalamud; /// Any asset can cease to exist at any point, even if the enum value exists.
/// Either ship your own assets, or be prepared for errors. /// +// Implementation notes: avoid specifying numbers too high here. Lookup table is currently implemented as an array. public enum DalamudAsset { /// diff --git a/Dalamud/Interface/ImGuiSeStringRenderer/SeStringDrawResult.cs b/Dalamud/Interface/ImGuiSeStringRenderer/SeStringDrawResult.cs index 905e8ed23..f9dae288b 100644 --- a/Dalamud/Interface/ImGuiSeStringRenderer/SeStringDrawResult.cs +++ b/Dalamud/Interface/ImGuiSeStringRenderer/SeStringDrawResult.cs @@ -5,7 +5,7 @@ using Dalamud.Game.Text.SeStringHandling; namespace Dalamud.Interface.ImGuiSeStringRenderer; -/// Represents the result of n rendered interactable SeString. +/// Represents the result of a rendered interactable SeString. public ref struct SeStringDrawResult { private Payload? lazyPayload; diff --git a/Dalamud/Interface/Textures/Internal/SharedImmediateTextures/SharedImmediateTexture.cs b/Dalamud/Interface/Textures/Internal/SharedImmediateTextures/SharedImmediateTexture.cs index c71d83fe8..5f9925ed3 100644 --- a/Dalamud/Interface/Textures/Internal/SharedImmediateTextures/SharedImmediateTexture.cs +++ b/Dalamud/Interface/Textures/Internal/SharedImmediateTextures/SharedImmediateTexture.cs @@ -171,17 +171,14 @@ internal abstract class SharedImmediateTexture /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - public IDalamudTextureWrap GetWrapOrEmpty() => this.GetWrapOrDefault(Service.Get().Empty4X4); + public IDalamudTextureWrap GetWrapOrEmpty() => + this.TryGetWrap(out var texture, out _) ? texture : Service.Get().Empty4X4; /// [return: NotNullIfNotNull(nameof(defaultWrap))] [MethodImpl(MethodImplOptions.AggressiveInlining)] - public IDalamudTextureWrap? GetWrapOrDefault(IDalamudTextureWrap? defaultWrap) - { - if (!this.TryGetWrap(out var texture, out _)) - texture = null; - return texture ?? defaultWrap; - } + public IDalamudTextureWrap? GetWrapOrDefault(IDalamudTextureWrap? defaultWrap) => + this.TryGetWrap(out var texture, out _) ? texture : defaultWrap; /// [MethodImpl(MethodImplOptions.AggressiveInlining)] diff --git a/Dalamud/Interface/Textures/TextureWraps/ForwardingTextureWrap.cs b/Dalamud/Interface/Textures/TextureWraps/ForwardingTextureWrap.cs index 8b0516e03..7d6ff8580 100644 --- a/Dalamud/Interface/Textures/TextureWraps/ForwardingTextureWrap.cs +++ b/Dalamud/Interface/Textures/TextureWraps/ForwardingTextureWrap.cs @@ -37,7 +37,11 @@ public abstract class ForwardingTextureWrap : IDalamudTextureWrap public Vector2 Size { [MethodImpl(MethodImplOptions.AggressiveInlining)] - get => new(this.Width, this.Height); + get + { + var wrap = this.GetWrap(); + return new(wrap.Width, wrap.Height); + } } /// diff --git a/Dalamud/Interface/Textures/TextureWraps/Internal/DisposeSuppressingTextureWrap.cs b/Dalamud/Interface/Textures/TextureWraps/Internal/DisposeSuppressingTextureWrap.cs index 0dd5c9f25..3bb984be8 100644 --- a/Dalamud/Interface/Textures/TextureWraps/Internal/DisposeSuppressingTextureWrap.cs +++ b/Dalamud/Interface/Textures/TextureWraps/Internal/DisposeSuppressingTextureWrap.cs @@ -1,20 +1,13 @@ -using Dalamud.Interface.Internal; - namespace Dalamud.Interface.Textures.TextureWraps.Internal; /// A texture wrap that ignores calls. -internal class DisposeSuppressingTextureWrap : ForwardingTextureWrap +/// The inner wrap. +internal class DisposeSuppressingTextureWrap(IDalamudTextureWrap innerWrap) : ForwardingTextureWrap { - private readonly IDalamudTextureWrap innerWrap; - - /// Initializes a new instance of the class. - /// The inner wrap. - public DisposeSuppressingTextureWrap(IDalamudTextureWrap wrap) => this.innerWrap = wrap; - /// protected override bool TryGetWrap(out IDalamudTextureWrap? wrap) { - wrap = this.innerWrap; + wrap = innerWrap; return true; } } diff --git a/Dalamud/Storage/Assets/DalamudAssetExtensions.cs b/Dalamud/Storage/Assets/DalamudAssetExtensions.cs index 9052a1c6d..4fe72240b 100644 --- a/Dalamud/Storage/Assets/DalamudAssetExtensions.cs +++ b/Dalamud/Storage/Assets/DalamudAssetExtensions.cs @@ -1,46 +1,37 @@ -using System.Collections.Frozen; -using System.Collections.Generic; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using Dalamud.Utility; namespace Dalamud.Storage.Assets; -/// -/// Extension methods for . -/// +/// Extension methods for . public static class DalamudAssetExtensions { - private static readonly FrozenDictionary AttributeCache = CreateCache(); + private static readonly DalamudAssetAttribute EmptyAttribute = new(DalamudAssetPurpose.Empty, null, false); + private static readonly DalamudAssetAttribute[] AttributeCache = CreateCache(); - /// - /// Gets the purpose. - /// + /// Gets the purpose. /// The asset. /// The purpose. - public static DalamudAssetPurpose GetPurpose(this DalamudAsset asset) - { - return GetAssetAttribute(asset)?.Purpose ?? DalamudAssetPurpose.Empty; - } + public static DalamudAssetPurpose GetPurpose(this DalamudAsset asset) => asset.GetAssetAttribute().Purpose; - /// - /// Gets the attribute. - /// + /// Gets the attribute. /// The asset. /// The attribute. - internal static DalamudAssetAttribute? GetAssetAttribute(this DalamudAsset asset) + internal static DalamudAssetAttribute GetAssetAttribute(this DalamudAsset asset) => + (int)asset < 0 || (int)asset >= AttributeCache.Length + ? EmptyAttribute + : Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(AttributeCache), (int)asset); + + private static DalamudAssetAttribute[] CreateCache() { - return AttributeCache.GetValueOrDefault(asset); - } - - private static FrozenDictionary CreateCache() - { - var dict = new Dictionary(); - - foreach (var asset in Enum.GetValues()) - { - dict.Add(asset, asset.GetAttribute()); - } - - return dict.ToFrozenDictionary(); + var assets = Enum.GetValues(); + var table = new DalamudAssetAttribute[assets.Max(x => (int)x) + 1]; + table.AsSpan().Fill(EmptyAttribute); + foreach (var asset in assets) + table[(int)asset] = asset.GetAttribute() ?? EmptyAttribute; + return table; } } diff --git a/Dalamud/Storage/Assets/DalamudAssetManager.cs b/Dalamud/Storage/Assets/DalamudAssetManager.cs index f750de64a..6fe26b90b 100644 --- a/Dalamud/Storage/Assets/DalamudAssetManager.cs +++ b/Dalamud/Storage/Assets/DalamudAssetManager.cs @@ -3,10 +3,11 @@ using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.IO; using System.Linq; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; -using Dalamud.Interface.Internal; using Dalamud.Interface.Textures.Internal; using Dalamud.Interface.Textures.TextureWraps; using Dalamud.Interface.Textures.TextureWraps.Internal; @@ -36,10 +37,9 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud private const int DownloadAttemptCount = 10; private const int RenameAttemptCount = 10; - private readonly object syncRoot = new(); private readonly DisposeSafety.ScopedFinalizer scopedFinalizer = new(); - private readonly Dictionary?> fileStreams; - private readonly Dictionary?> textureWraps; + private readonly Task?[] fileStreams; + private readonly Task?[] textureWraps; private readonly Dalamud dalamud; private readonly HappyHttpClient httpClient; private readonly string localSourceDirectory; @@ -59,18 +59,18 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud Directory.CreateDirectory(this.localSourceDirectory); this.scopedFinalizer.Add(this.cancellationTokenSource = new()); - this.fileStreams = Enum.GetValues().ToDictionary(x => x, _ => (Task?)null); - this.textureWraps = Enum.GetValues().ToDictionary(x => x, _ => (Task?)null); + var numDalamudAssetSlots = Enum.GetValues().Max(x => (int)x) + 1; + this.fileStreams = new Task?[numDalamudAssetSlots]; + this.textureWraps = new Task?[numDalamudAssetSlots]; // Block until all the required assets to be ready. var loadTimings = Timings.Start("DAM LoadAll"); registerStartupBlocker( Task.WhenAll( Enum.GetValues() - .Where(x => x is not DalamudAsset.Empty4X4) - .Where(x => x.GetAssetAttribute()?.Required is true) + .Where(static x => x.GetAssetAttribute() is { Required: true, Data: null }) .Select(this.CreateStreamAsync) - .Select(x => x.ToContentDisposedTask())) + .Select(static x => x.ToContentDisposedTask())) .ContinueWith( r => { @@ -80,13 +80,13 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud .Unwrap(), "Prevent Dalamud from loading more stuff, until we've ensured that all required assets are available."); + // Begin preloading optional(non-required) assets. Task.WhenAll( Enum.GetValues() - .Where(x => x is not DalamudAsset.Empty4X4) - .Where(x => x.GetAssetAttribute()?.Required is false) + .Where(static x => x.GetAssetAttribute() is { Required: false, Data: null }) .Select(this.CreateStreamAsync) - .Select(x => x.ToContentDisposedTask(true))) - .ContinueWith(r => Log.Verbose($"Optional assets load state: {r}")); + .Select(static x => x.ToContentDisposedTask(true))) + .ContinueWith(static r => Log.Verbose($"Optional assets load state: {r}")); } /// @@ -98,77 +98,97 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud /// void IInternalDisposableService.DisposeService() { - lock (this.syncRoot) - { - if (this.isDisposed) - return; + if (this.isDisposed) + return; - this.isDisposed = true; - } + this.isDisposed = true; this.cancellationTokenSource.Cancel(); Task.WaitAll( Array.Empty() - .Concat(this.fileStreams.Values) - .Concat(this.textureWraps.Values) - .Where(x => x is not null) - .Select(x => x.ContinueWith(r => { _ = r.Exception; })) - .ToArray()); + .Concat(this.fileStreams) + .Concat(this.textureWraps) + .Where(static x => x is not null) + .Select(static x => x.ContinueWith(static r => _ = r.Exception)) + .ToArray()); this.scopedFinalizer.Dispose(); } /// [Pure] public bool IsStreamImmediatelyAvailable(DalamudAsset asset) => - asset.GetAssetAttribute()?.Data is not null - || this.fileStreams[asset]?.IsCompletedSuccessfully is true; + asset.GetAssetAttribute().Data is not null + || this.fileStreams[(int)asset]?.IsCompletedSuccessfully is true; /// [Pure] - public Stream CreateStream(DalamudAsset asset) - { - var s = this.CreateStreamAsync(asset); - s.Wait(); - if (s.IsCompletedSuccessfully) - return s.Result; - if (s.Exception is not null) - throw new AggregateException(s.Exception.InnerExceptions); - throw new OperationCanceledException(); - } + public Stream CreateStream(DalamudAsset asset) => this.CreateStreamAsync(asset).Result; /// [Pure] public Task CreateStreamAsync(DalamudAsset asset) { - if (asset.GetAssetAttribute() is { Data: { } rawData }) - return Task.FromResult(new MemoryStream(rawData, false)); + ObjectDisposedException.ThrowIf(this.isDisposed, this); - Task task; - lock (this.syncRoot) + var attribute = asset.GetAssetAttribute(); + + // The corresponding asset does not exist. + if (attribute.Purpose is DalamudAssetPurpose.Empty) + return Task.FromException(new ArgumentOutOfRangeException(nameof(asset), asset, null)); + + // Special case: raw data is specified from asset definition. + if (attribute.Data is not null) + return Task.FromResult(new MemoryStream(attribute.Data, false)); + + // Range is guaranteed to be satisfied if the asset has a purpose; get the slot for the stream task. + ref var streamTaskRef = ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(this.fileStreams), (int)asset); + + // The stream task is already set. + if (streamTaskRef is not null) + return CloneFileStreamAsync(streamTaskRef); + + var tcs = new TaskCompletionSource(); + if (Interlocked.CompareExchange(ref streamTaskRef, tcs.Task, null) is not { } streamTask) { - if (this.isDisposed) - throw new ObjectDisposedException(nameof(DalamudAssetManager)); - - task = this.fileStreams[asset] ??= CreateInnerAsync(); + // The stream task has just been set. Actually start the operation. + // In case it did not correctly finish the task in tcs, set the task to a failed state. + // Do not pass cancellation token here; we always want to touch tcs. + Task.Run( + async () => + { + try + { + tcs.SetResult(await CreateInnerAsync(this, asset)); + } + catch (Exception e) + { + tcs.SetException(e); + } + }, + default); + return CloneFileStreamAsync(tcs.Task); } - return this.TransformImmediate( - task, - x => (Stream)new FileStream( - x.Name, + // Discard the new task, and return the already created task. + tcs.SetCanceled(); + return CloneFileStreamAsync(streamTask); + + static async Task CloneFileStreamAsync(Task fileStreamTask) => + new FileStream( + (await fileStreamTask).Name, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, - FileOptions.Asynchronous | FileOptions.SequentialScan)); + FileOptions.Asynchronous | FileOptions.SequentialScan); - async Task CreateInnerAsync() + static async Task CreateInnerAsync(DalamudAssetManager dam, DalamudAsset asset) { string path; List exceptions = null; - foreach (var name in asset.GetAttributes().Select(x => x.FileName)) + foreach (var name in asset.GetAttributes().Select(static x => x.FileName)) { - if (!File.Exists(path = Path.Combine(this.dalamud.AssetDirectory.FullName, name))) + if (!File.Exists(path = Path.Combine(dam.dalamud.AssetDirectory.FullName, name))) continue; try @@ -177,12 +197,12 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud } catch (Exception e) when (e is not OperationCanceledException) { - exceptions ??= new(); + exceptions ??= []; exceptions.Add(e); } } - if (File.Exists(path = Path.Combine(this.localSourceDirectory, asset.ToString()))) + if (File.Exists(path = Path.Combine(dam.localSourceDirectory, asset.ToString()))) { try { @@ -190,7 +210,7 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud } catch (Exception e) when (e is not OperationCanceledException) { - exceptions ??= new(); + exceptions ??= []; exceptions.Add(e); } } @@ -211,9 +231,9 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud await using (var tempPathStream = File.Open(tempPath, FileMode.Create, FileAccess.Write)) { await url.DownloadAsync( - this.httpClient.SharedHttpClient, + dam.httpClient.SharedHttpClient, tempPathStream, - this.cancellationTokenSource.Token); + dam.cancellationTokenSource.Token); } for (var j = RenameAttemptCount; ; j--) @@ -232,7 +252,7 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud nameof(DalamudAssetManager), asset, j); - await Task.Delay(1000, this.cancellationTokenSource.Token); + await Task.Delay(1000, dam.cancellationTokenSource.Token); continue; } @@ -255,14 +275,18 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud nameof(DalamudAssetManager), asset, delay); - await Task.Delay(delay * 1000, this.cancellationTokenSource.Token); + await Task.Delay(delay * 1000, dam.cancellationTokenSource.Token); } throw new FileNotFoundException($"Failed to load the asset {asset}.", asset.ToString()); } - catch (Exception e) when (e is not OperationCanceledException) + catch (OperationCanceledException) { - exceptions ??= new(); + throw; + } + catch (Exception e) + { + exceptions ??= []; exceptions.Add(e); try { @@ -272,9 +296,9 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud { // don't care } - } - throw new AggregateException(exceptions); + throw new AggregateException(exceptions); + } } } @@ -296,33 +320,63 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud [Pure] public Task GetDalamudTextureWrapAsync(DalamudAsset asset) { - var purpose = asset.GetPurpose(); - if (purpose is not DalamudAssetPurpose.TextureFromPng and not DalamudAssetPurpose.TextureFromRaw) - throw new ArgumentOutOfRangeException(nameof(asset), asset, "The asset cannot be taken as a Texture2D."); + ObjectDisposedException.ThrowIf(this.isDisposed, this); - Task task; - lock (this.syncRoot) + // Check if asset is a texture asset. + if (asset.GetPurpose() is not DalamudAssetPurpose.TextureFromPng and not DalamudAssetPurpose.TextureFromRaw) { - if (this.isDisposed) - throw new ObjectDisposedException(nameof(DalamudAssetManager)); - - task = this.textureWraps[asset] ??= CreateInnerAsync(); + return Task.FromException( + new ArgumentOutOfRangeException( + nameof(asset), + asset, + "The asset does not exist or cannot be taken as a Texture2D.")); } - return task; + // Range is guaranteed to be satisfied if the asset has a purpose; get the slot for the wrap task. + ref var wrapTaskRef = ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(this.textureWraps), (int)asset); - async Task CreateInnerAsync() + // The wrap task is already set. + if (wrapTaskRef is not null) + return wrapTaskRef; + + var tcs = new TaskCompletionSource(); + if (Interlocked.CompareExchange(ref wrapTaskRef, tcs.Task, null) is not { } wrapTask) + { + // The stream task has just been set. Actually start the operation. + // In case it did not correctly finish the task in tcs, set the task to a failed state. + // Do not pass cancellation token here; we always want to touch tcs. + Task.Run( + async () => + { + try + { + tcs.SetResult(await CreateInnerAsync(this, asset)); + } + catch (Exception e) + { + tcs.SetException(e); + } + }, + default); + return tcs.Task; + } + + // Discard the new task, and return the already created task. + tcs.SetCanceled(); + return wrapTask; + + static async Task CreateInnerAsync(DalamudAssetManager dam, DalamudAsset asset) { var buf = Array.Empty(); try { var tm = await Service.GetAsync(); - await using var stream = await this.CreateStreamAsync(asset); + await using var stream = await dam.CreateStreamAsync(asset); var length = checked((int)stream.Length); buf = ArrayPool.Shared.Rent(length); stream.ReadExactly(buf, 0, length); var name = $"{nameof(DalamudAsset)}[{Enum.GetName(asset)}]"; - var image = purpose switch + var texture = asset.GetPurpose() switch { DalamudAssetPurpose.TextureFromPng => await tm.CreateFromImageAsync(buf, name), DalamudAssetPurpose.TextureFromRaw => @@ -330,17 +384,9 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud ? await tm.CreateFromRawAsync(raw.Specification, buf, name) : throw new InvalidOperationException( "TextureFromRaw must accompany a DalamudAssetRawTextureAttribute."), - _ => null, + _ => throw new InvalidOperationException(), // cannot happen }; - var disposeDeferred = - this.scopedFinalizer.Add(image) - ?? throw new InvalidOperationException("Something went wrong very badly"); - return new DisposeSuppressingTextureWrap(disposeDeferred); - } - catch (Exception e) - { - Log.Error(e, "[{name}] Failed to load {asset}.", nameof(DalamudAssetManager), asset); - throw; + return new DisposeSuppressingTextureWrap(dam.scopedFinalizer.Add(texture)); } finally { @@ -348,13 +394,4 @@ internal sealed class DalamudAssetManager : IInternalDisposableService, IDalamud } } } - - private Task TransformImmediate(Task task, Func transformer) - { - if (task.IsCompletedSuccessfully) - return Task.FromResult(transformer(task.Result)); - if (task.Exception is { } exc) - return Task.FromException(exc); - return task.ContinueWith(_ => this.TransformImmediate(task, transformer)).Unwrap(); - } } diff --git a/Dalamud/Storage/Assets/DalamudAssetPurpose.cs b/Dalamud/Storage/Assets/DalamudAssetPurpose.cs index b059cb3d6..e6c7bd920 100644 --- a/Dalamud/Storage/Assets/DalamudAssetPurpose.cs +++ b/Dalamud/Storage/Assets/DalamudAssetPurpose.cs @@ -6,7 +6,7 @@ namespace Dalamud.Storage.Assets; public enum DalamudAssetPurpose { /// - /// The asset has no purpose. + /// The asset has no purpose, and is not valid and/or not accessible. /// Empty = 0,