chore: simplify refcounting logic, more concurrency fixes

This commit is contained in:
goat 2023-08-09 21:22:29 +02:00
parent 8a300cc98e
commit 24ad2d4c8b
No known key found for this signature in database
GPG key ID: 49E2AA8C6A76498B

View file

@ -233,13 +233,12 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP
/// Get texture info. /// Get texture info.
/// </summary> /// </summary>
/// <param name="path">Path to the texture.</param> /// <param name="path">Path to the texture.</param>
/// <param name="refresh">Whether or not the texture should be reloaded if it was unloaded.</param>
/// <param name="rethrow"> /// <param name="rethrow">
/// If true, exceptions caused by texture load will not be caught. /// If true, exceptions caused by texture load will not be caught.
/// If false, exceptions will be caught and a dummy texture will be returned to prevent plugins from using invalid texture handles. /// If false, exceptions will be caught and a dummy texture will be returned to prevent plugins from using invalid texture handles.
/// </param> /// </param>
/// <returns>Info object storing texture metadata.</returns> /// <returns>Info object storing texture metadata.</returns>
internal TextureInfo? GetInfo(string path, bool refresh = true, bool rethrow = false) internal TextureInfo GetInfo(string path, bool rethrow = false)
{ {
TextureInfo? info; TextureInfo? info;
lock (this.activeTextures) lock (this.activeTextures)
@ -248,30 +247,20 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP
{ {
Debug.Assert(rethrow, "This should never run when getting outside of creator"); Debug.Assert(rethrow, "This should never run when getting outside of creator");
if (!refresh)
return null;
info = new TextureInfo(); info = new TextureInfo();
this.activeTextures.Add(path, info); this.activeTextures.Add(path, info);
} }
if (info == null) if (info == null)
throw new Exception("null info in activeTextures"); throw new Exception("null info in activeTextures");
// NOTE: We need to increase the refcount here while locking the collection!
// Otherwise, if this is loaded from a task, cleanup might already try to delete it
// before it can be increased.
info.RefCount++;
} }
if (refresh && info.KeepAliveCount == 0) if (info.KeepAliveCount == 0)
info.LastAccess = DateTime.UtcNow; info.LastAccess = DateTime.UtcNow;
if (info is { Wrap: not null }) if (info is { Wrap: not null })
return info; return info;
if (refresh)
{
if (!this.im.IsReady) if (!this.im.IsReady)
throw new InvalidOperationException("Cannot create textures before scene is ready"); throw new InvalidOperationException("Cannot create textures before scene is ready");
@ -346,8 +335,6 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP
} }
info.Wrap = wrap; info.Wrap = wrap;
}
return info; return info;
} }
@ -359,12 +346,11 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP
/// <param name="keepAlive">Whether or not this handle was created in keep-alive mode.</param> /// <param name="keepAlive">Whether or not this handle was created in keep-alive mode.</param>
internal void NotifyTextureDisposed(string path, bool keepAlive) internal void NotifyTextureDisposed(string path, bool keepAlive)
{ {
var info = this.GetInfo(path, false); lock (this.activeTextures)
// This texture was already disposed
if (info == null)
{ {
Log.Warning("Disposing unknown texture {Path}", path); if (!this.activeTextures.TryGetValue(path, out var info))
{
Log.Warning("Disposing texture that didn't exist: {Path}", path);
return; return;
} }
@ -377,6 +363,7 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP
if (info.RefCount <= 0) if (info.RefCount <= 0)
info.LastAccess = default; info.LastAccess = default;
} }
}
private static string FormatIconPath(uint iconId, string? type, bool highResolution) private static string FormatIconPath(uint iconId, string? type, bool highResolution)
{ {
@ -390,16 +377,24 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP
} }
private TextureManagerTextureWrap? CreateWrap(string path, bool keepAlive) private TextureManagerTextureWrap? CreateWrap(string path, bool keepAlive)
{
lock (this.activeTextures)
{ {
// This will create the texture. // This will create the texture.
// That's fine, it's probably used immediately and this will let the plugin catch load errors. // That's fine, it's probably used immediately and this will let the plugin catch load errors.
var info = this.GetInfo(path, rethrow: true)!; var info = this.GetInfo(path, rethrow: true);
// We need to increase the refcounts here while locking the collection!
// Otherwise, if this is loaded from a task, cleanup might already try to delete it
// before it can be increased.
info.RefCount++;
if (keepAlive) if (keepAlive)
info.KeepAliveCount++; info.KeepAliveCount++;
return new TextureManagerTextureWrap(path, info.Extents, keepAlive, this); return new TextureManagerTextureWrap(path, info.Extents, keepAlive, this);
} }
}
private void FrameworkOnUpdate(Framework fw) private void FrameworkOnUpdate(Framework fw)
{ {
@ -589,7 +584,7 @@ internal class TextureManagerTextureWrap : IDalamudTextureWrap
/// <inheritdoc/> /// <inheritdoc/>
public IntPtr ImGuiHandle => !this.IsDisposed ? public IntPtr ImGuiHandle => !this.IsDisposed ?
this.manager.GetInfo(this.path)!.Wrap!.ImGuiHandle : this.manager.GetInfo(this.path).Wrap!.ImGuiHandle :
throw new InvalidOperationException("Texture already disposed. You may not render it."); throw new InvalidOperationException("Texture already disposed. You may not render it.");
/// <inheritdoc/> /// <inheritdoc/>