Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decouple image loading from stb_image; tidy up YARGImage and FixedArray + its derivatives #207

Open
wants to merge 4 commits into
base: native
Choose a base branch
from
Open

Decouple image loading from stb_image; tidy up YARGImage and FixedArray + its derivatives #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40d17c7
Fix FixedArray disposable implementation
TheNathannator Aug 28, 2024
10b3a4c
Tidy up FixedArray and its derivatives
TheNathannator Aug 28, 2024
81686b0
Decouple YARGImage from stb_image
TheNathannator Aug 28, 2024
56ee987
Tidy up YARGImage
TheNathannator Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 19 additions & 28 deletions YARG.Core/IO/Disposables/AllocatedArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,21 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.InteropServices;
using System.Text;
using YARG.Core.Logging;

namespace YARG.Core.IO.Disposables
{
public sealed unsafe class AllocatedArray<T> : FixedArray<T>
where T : unmanaged
{
public static AllocatedArray<T> Alloc(long length)
{
var ptr = (T*)Marshal.AllocHGlobal((int)(length * sizeof(T)));
return new AllocatedArray<T>(ptr, length);
}

public static AllocatedArray<T> ReAlloc(AllocatedArray<T> original, long length)
{
GC.SuppressFinalize(original);
var ptr = (T*) Marshal.ReAllocHGlobal(original.IntPtr, (IntPtr) (length * sizeof(T)));
return new AllocatedArray<T>(ptr, length);
}

public static AllocatedArray<T> Read(Stream stream, long length)
{
var buffer = Alloc(length);
stream.Read(new Span<byte>(buffer.Ptr, (int)(length * sizeof(T))));
return buffer;
}
public Span<T> Span => new(Ptr, (int) Length);

private AllocatedArray(T* ptr, long length)
: base(ptr, length) { }

public Span<T> Span => new(Ptr, (int)Length);
protected override void DisposeUnmanaged()
{
Marshal.FreeHGlobal(IntPtr);
}

public Span<T> Slice(long offset, long count)
{
Expand All @@ -44,15 +26,24 @@ public Span<T> Slice(long offset, long count)
return new Span<T>(Ptr + offset, (int) count);
}

public override void Dispose()
public static AllocatedArray<T> Alloc(long length)
{
Marshal.FreeHGlobal(IntPtr);
GC.SuppressFinalize(this);
var ptr = (T*) Marshal.AllocHGlobal((int) (length * sizeof(T)));
return new AllocatedArray<T>(ptr, length);
}

public static AllocatedArray<T> ReAlloc(AllocatedArray<T> original, long length)
{
GC.SuppressFinalize(original);
var ptr = (T*) Marshal.ReAllocHGlobal(original.IntPtr, (IntPtr) (length * sizeof(T)));
return new AllocatedArray<T>(ptr, length);
}

~AllocatedArray()
public static AllocatedArray<T> Read(Stream stream, long length)
{
YargLogger.LogWarning("Dev warning: only use AllocatedArray IF YOU MANUALLY DISPOSE! Not doing so defeats the purpose!");
var buffer = Alloc(length);
stream.Read(new Span<byte>(buffer.Ptr, (int) (length * sizeof(T))));
return buffer;
}
}
}
49 changes: 29 additions & 20 deletions YARG.Core/IO/Disposables/FixedArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Runtime.InteropServices;
using YARG.Core.Logging;

namespace YARG.Core.IO.Disposables
Expand All @@ -12,7 +9,7 @@ namespace YARG.Core.IO.Disposables
/// A wrapper interface over a fixed area of unmanaged memory.
/// Provides functions to create spans and span slices alongside
/// basic indexing and enumeration.<br></br><br></br>
///
///
/// For serious performance-critical code, the raw pointer to
/// the start of the memory block is also provided.<br></br>
/// However, code that uses the value directly should first check
Expand All @@ -25,11 +22,6 @@ namespace YARG.Core.IO.Disposables
public unsafe class FixedArray<T> : IDisposable
where T : unmanaged
{
public static FixedArray<T> Alias(T* ptr, long length)
{
return new FixedArray<T>(ptr, length);
}

/// <summary>
/// Pointer to the beginning of the memory block.<br></br>
/// DO NOT TOUCH UNLESS YOU ENSURE YOU'RE WITHIN BOUNDS
Expand All @@ -41,12 +33,6 @@ public static FixedArray<T> Alias(T* ptr, long length)
/// </summary>
public readonly long Length;

protected FixedArray(T* ptr, long length)
{
Ptr = ptr;
Length = length;
}

/// <summary>
/// Provides the pointer to the block of memory in IntPtr form
/// </summary>
Expand All @@ -55,9 +41,14 @@ protected FixedArray(T* ptr, long length)
/// <summary>
/// Provides a ReadOnlySpan over the block of memory
/// </summary>
public ReadOnlySpan<T> ReadOnlySpan => new(Ptr, (int)Length);
public ReadOnlySpan<T> ReadOnlySpan => new(Ptr, (int) Length);

public UnmanagedMemoryStream ToStream() => new((byte*)Ptr, (int)(Length * sizeof(T)));
public UnmanagedMemoryStream ToStream() => new((byte*) Ptr, (int) (Length * sizeof(T)));

public static FixedArray<T> Alias(void* ptr, long length)
{
return new FixedArray<T>((T*) ptr, length);
}

/// <summary>
/// Indexer into the fixed block of memory
Expand All @@ -76,16 +67,34 @@ public ref T this[long index]
}
}

public virtual void Dispose()
protected FixedArray(T* ptr, long length)
{
GC.SuppressFinalize(this);
Ptr = ptr;
Length = length;
}

~FixedArray()
{
Dispose();
YargLogger.LogFormatWarning("Dev warning: only use {0} IF YOU MANUALLY DISPOSE! Not doing so defeats the purpose!", GetType());
Dispose(false);
}

public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}

private void Dispose(bool disposing)
{
if (disposing)
DisposeManaged();
DisposeUnmanaged();
}

protected virtual void DisposeManaged() { }
protected virtual void DisposeUnmanaged() { }

private sealed class FixedArrayDebugView
{
private readonly FixedArray<T> array;
Expand Down
47 changes: 19 additions & 28 deletions YARG.Core/IO/Disposables/MemoryMappedArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,10 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.IO;
using System.IO.MemoryMappedFiles;
using System.Text;
using YARG.Core.Logging;

namespace YARG.Core.IO.Disposables
{
public sealed unsafe class MemoryMappedArray : FixedArray<byte>
{
public static MemoryMappedArray Load(FileInfo info)
{
return Load(info.FullName, info.Length);
}

public static MemoryMappedArray Load(in AbridgedFileInfo_Length info)
{
return Load(info.FullName, info.Length);
}

public static MemoryMappedArray Load(string filename, long length)
{
var file = MemoryMappedFile.CreateFromFile(filename, FileMode.Open, null, 0, MemoryMappedFileAccess.Read);
var accessor = file.CreateViewAccessor(0, 0, MemoryMappedFileAccess.Read);
byte* ptr = null;
accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref ptr);
return new MemoryMappedArray(file, accessor, ptr, length);
}

private readonly MemoryMappedFile _file;
private readonly MemoryMappedViewAccessor _accessor;

Expand All @@ -38,17 +15,31 @@ private MemoryMappedArray(MemoryMappedFile file, MemoryMappedViewAccessor access
_accessor = accessor;
}

public override void Dispose()
// Note: not DisposeUnmanaged, as object references are not guaranteed to be valid during finalization
protected override void DisposeManaged()
{
_accessor.SafeMemoryMappedViewHandle.ReleasePointer();
_accessor.Dispose();
_file.Dispose();
Comment on lines +18 to 23
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now if you forget to call Dispose(), you've potentially introduced a memory leak. Any usage of AcquirePointer must always be paired with a call to ReleasePointer. That's no longer the case.

And you can't just put it into the DisposeUnmanaged function either because DisposeManaged is called first, meaning the disposals would be out of order. The only solution is to put into DisposeUnmanaged... which makes DisposeManaged completely useless... which brings us right back to what it was before.

I am not accepting this change whatsoever.

Copy link
Collaborator Author

@TheNathannator TheNathannator Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It cannot be in DisposeUnmanaged because managed object references are not guaranteed to be in a valid state in finalizers. As it was implemented previously, depending on runtime implementation details it very well could fail regardless because there are no guarantees made for anything.

The IDisposable best-practices post on StackOverflow I like to point people to when discussing how to implement it points to Eric Lippert's When Everything You Know is Wrong blog post as extra reading, which explains the pitfalls of finalizers. The second part covers why we have to use DisposeManaged:

image

Just because this did work previously doesn't mean it always will. Finalizers are a last-ditch effort, not a reliable mechanism.

Copy link
Collaborator

@sonicfind sonicfind Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should explicitly suppress the finalization of the those member variables on construction instead. Force GC to handle it in the order we expect. We just cannot forgo releasing the pointer in any situation.

GC.SuppressFinalize(this);
}

~MemoryMappedArray()
public static MemoryMappedArray Load(FileInfo info)
{
YargLogger.LogWarning("Dev warning: only use MemoryMappedArray IF YOU MANUALLY DISPOSE! Not doing so defeats the purpose!");
return Load(info.FullName, info.Length);
}

public static MemoryMappedArray Load(in AbridgedFileInfo_Length info)
{
return Load(info.FullName, info.Length);
}

public static MemoryMappedArray Load(string filename, long length)
{
var file = MemoryMappedFile.CreateFromFile(filename, FileMode.Open, null, 0, MemoryMappedFileAccess.Read);
var accessor = file.CreateViewAccessor(0, 0, MemoryMappedFileAccess.Read);
byte* ptr = null;
accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref ptr);
return new MemoryMappedArray(file, accessor, ptr, length);
}
}
}
Loading