VMASharp API Review #595

Perksey · 2021-08-29T15:49:25Z

This will serve as the place where we'll review the VMASharp API.

The public API has been summarised in a single file (in markdown format so we can add words/discussions/conclusions/action items if we want to), which will be the file for reviewing.

Internal APIs have been excluded.

Perksey · 2021-08-29T15:50:36Z

cc @HurricanKai @sunkin351

p3t3rix

Generally speaking, it would help if the main structs/classes had a description of what the intended purpose of the class is.

p3t3rix · 2021-10-02T22:36:47Z

src/Vulkan/VMASharp_API_For_Review.md

+  public sealed class VulkanMemoryAllocator : IDisposable
+  {
+    public Device Device { get; }
+    public VulkanMemoryAllocator(in VulkanMemoryAllocatorCreateInfo createInfo);


Definition of VulkanMemoryAllocatorCreateInfo is missing. Is this intentional (AllocationCreateInfo is listed)?

This could honestly be replaced with { get; init; } properties with a few required constructor arguments. I wrote this before those were a thing.

Depends on whether we want to support NS20 with this library or not - init doesn't work there.

Weren't you planning on being .NET 6 only?

That's 3.0. VMASharp isn't triaged against any release at the moment. It could reasonably be a new feature in 2.X sustaining so the question would be do we want to support this package everywhere Silk.NET.Vulkan is supported today?

Depends on whether we want to support NS20 with this library or not - init doesn't work there.

I think for clarity for users, we should support NS20, and in 3.0 look into rewriting parts to use the newer features of .NET 6(+)

p3t3rix · 2021-10-02T22:40:40Z

src/Vulkan/VMASharp_API_For_Review.md

+    public MapMemoryException(Result res);
+    public MapMemoryException(string message, Result res);
+  }
+  public enum MemoryUsage


So i guess this is a combination of BufferUsageFlags, SharingMode and MemoryPropertyFlags ?

These are presets for MemoryPropertyFlags, essentially.

p3t3rix · 2021-10-02T23:01:45Z

src/Vulkan/VMASharp_API_For_Review.md

+    long SumFreeSize { get; }
+    long UnusedRangeSizeMax { get; }
+    bool IsEmpty { get; }
+    void Validate();


i guess since it is void this throws if it is invalid ?

Yes, that is the design choice, but its only called in Debug Builds

p3t3rix · 2021-10-02T23:04:54Z

src/Vulkan/VMASharp_API_For_Review.md

+    bool IsEmpty { get; }
+    void Validate();
+    void CalcAllocationStatInfo(out StatInfo outInfo);
+    void AddPoolStats(ref PoolStats stats);


What does this method do to the stats ?

Something like this.

p3t3rix · 2021-10-02T23:06:14Z

src/Vulkan/VMASharp_API_For_Review.md

+  }
+  public struct AllocationRequest
+  {
+    public const long LostAllocationCost = 1048576;


What is this magic number for ? Is it a limit or a threshold ?

p3t3rix · 2021-10-02T23:29:50Z

src/Vulkan/VMASharp_API_For_Review.md

+    public int MinBlockCount;
+    public int MaxBlockCount;
+    public int FrameInUseCount;
+    public Func<long, IBlockMetadata>? AllocationAlgorithmCreate;


What does this function do ?

Allows the user to specify what algorithm they want to use for allocating out of each memory block in the pool.

p3t3rix · 2021-10-02T23:30:58Z

src/Vulkan/VMASharp_API_For_Review.md

+    MinFragmentation = WorstFit, // 0x00000002
+  }
+  [Flags]
+  public enum AllocatorCreateFlags


I guess this flag is used in the VulkanMemoryAllocatorCreateInfo ?

p3t3rix · 2021-10-02T23:31:50Z

src/Vulkan/VMASharp_API_For_Review.md

+  {
+    ExternallySyncronized = 1,
+    ExtMemoryBudget = 8,
+    AMDDeviceCoherentMemory = 16, // 0x00000010


Whats so special for AMD ?

That's just what the Vulkan extension is named. AMD (and some other vendors) expose a way to check whether the driver thinks it'd be better for performance if a buffer/image would get a dedicated allocation, instead of bing sub-allocated

p3t3rix · 2021-10-02T23:33:16Z

src/Vulkan/VMASharp_API_For_Review.md

+    public object Item;
+    public object CustomData;
+    public AllocationRequestType Type;
+    public readonly long CalcCost();


I guess the cost will be used internally in the allocator itself to decide something internal ?

p3t3rix · 2021-10-02T23:35:38Z

src/Vulkan/VMASharp_API_For_Review.md

+      int frameInUseCount = 0,
+      Func<long, IBlockMetadata>? allocationAlgorithemCreate = null);
+  }
+  public struct AllocationRequest


What is the purpose of the AllocationRequest compared to a normal allocation, better asked why would i care for the request and not just use an allocation via AllocateMemory?

VMA has this weird internal multi-stage allocation mechanism to support a few key features. This stores state for that.

sunkin351 · 2021-10-03T01:32:34Z

Ok, so, much of the internal logic is semi-documented in VMA, the native library I ported this from. No, I didn't bother to document a lot of this stuff, at the time it was a passion project. I may have done a lot more work on this if I were actively using Vulkan, but alas, my time had other places it needed to be spent. I'm saying this for your benefit and for public record.

Edit: I'll do active feedback on your review tomorrow, as I'm spent today.

p3t3rix · 2021-10-03T08:00:00Z

@sunkin351
This was not meant as a critique and i hope you don't take it personally. The comments i did were purely on this document with the help of some parts of the VMA documentation (when i could find the equivalent). I don't claim to be an expert on this topic and tried to understand the connections of the classes purely by the name of them. Ignore my comments if they sound stupid because i do them from a beginners/users point of view and just try to get the discussion started so we hopefully can all have a good memory allocator as part of Silk.

sunkin351 · 2021-10-03T16:18:26Z

@sunkin351 This was not meant as a critique and i hope you don't take it personally.

Fear not, my fellow developer. I didn't take any of this personally, though I did write that reply while under heavy fatigue, I hope you can forgive me. My only desire is to help turn this into a usable tool that every developer can enjoy.

Also, am I reading some of this right in that there are data structure definitions missing?

Perksey · 2021-10-03T16:19:38Z

I haven't included anything internal, should just be the public (and hopefully protected) APIs in this document.

As well as defragmentation which is being excluded altogether for the time being.

Perksey · 2021-10-03T17:26:36Z

Assigning to Kai as he's the Vulkan area owner.

p3t3rix · 2021-10-03T17:55:51Z

VulkanMemoryAllocatorCreateInfo struct would be nice to have in the document so we can discuss which stuff is actually needed to create one.

p3t3rix · 2021-10-03T19:25:20Z

well i tried to create some kind of basic overview diagram to highlight the public relations of the classes (plantuml source in the spoiler at the end).
So basically it all boils down to the Allocation class which owns a part of the memory. But i am not sure how the IBlockMetadata plays a role in all this (except that it's part of the multi-stage request).
Also how the VulkanMemoryPool relates to the allocation (apart from the AllocationCreateInfo where you can specify a Pool) is the Pool after creation not relevant anymore ?

plantuml source

@startuml

interface IBlockMetadata
{
long Size { get; }
int AllocationCount { get; }
long SumFreeSize { get; }
long UnusedRangeSizeMax { get; }
bool IsEmpty { get; }
void Validate();
void CalcAllocationStatInfo(out StatInfo outInfo);
void AddPoolStats(ref PoolStats stats);
bool TryCreateAllocationRequest(in AllocationContext context, out AllocationRequest request);
bool MakeRequestedAllocationsLost( int currentFrame, int frameInUseCount, ref AllocationRequest request);
int MakeAllocationsLost(int currentFrame, int frameInUseCount);
void CheckCorruption(nuint blockDataPointer);
void Alloc( in AllocationRequest request,SuballocationType type, long allocSize, BlockAllocation allocation);
void Free(BlockAllocation allocation);
void FreeAtOffset(long offset);
}
class Allocation << (A, grey) >>
{
long Size { get; }
int MemoryTypeIndex { get; }
abstract DeviceMemory DeviceMemory { get; }
abstract long Offset { get; internal set; }
object? UserData { get; set; }
abstract IntPtr MappedData { get; }
void Dispose();
Result BindBufferMemory(Buffer buffer);
Result BindBufferMemory(Buffer buffer, long allocationLocalOffset, IntPtr pNext);
Result BindBufferMemory(Buffer buffer, long allocationLocalOffset, void* pNext = null);
Result BindImageMemory(Image image);
Result BindImageMemory(Image image, long allocationLocalOffset, IntPtr pNext);
Result BindImageMemory(Image image, long allocationLocalOffset, void* pNext = null);
bool TouchAllocation();
Result Flush(long offset, long size);
Result Invalidate(long offset, long size);
abstract IntPtr Map();
abstract void Unmap();
bool TryGetMemory(out Memory memory) where T : unmanaged;
bool TryGetSpan(out Span span) where T : unmanaged;
}

class AllocationBudget << (S, gold) >>
{
long BlockBytes;
long AllocationBytes;
long Usage;
long Budget;
}

class AllocationContext << (S, gold) >>
{
int CurrentFrame;
int FrameInUseCount;
long BufferImageGranularity;
long AllocationSize;
long AllocationAlignment;
AllocationStrategyFlags Strategy;
SuballocationType SuballocationType;
bool CanMakeOtherLost;
}
class AllocationCreateInfo << (S, gold) >>
{
AllocationCreateFlags Flags;
AllocationStrategyFlags Strategy;
MemoryUsage Usage;
MemoryPropertyFlags? RequiredFlags;
MemoryPropertyFlags? PreferredFlags;
uint MemoryTypeBits;
VulkanMemoryPool? Pool;
object? UserData;
}
class AllocationPoolCreateInfo << (S, gold) >>
{
int MemoryTypeIndex;
PoolCreateFlags Flags;
long BlockSize;
int MinBlockCount;
int MaxBlockCount;
int FrameInUseCount;
Func<long, IBlockMetadata>? AllocationAlgorithmCreate;
}

class AllocationRequest << (S, gold) >>
{
long Offset;
long SumFreeSize;
long SumItemSize;
long ItemsToMakeLostCount;
object Item;
object CustomData;
AllocationRequestType Type;
readonly long CalcCost();
}

class PoolStats<< (S, gold) >>
{
long Size;
long UnusedSize;
int AllocationCount;
int UnusedRangeCount;
long UnusedRangeSizeMax;
int BlockCount;
}

class StatInfo<< (S, gold) >>
{
int BlockCount;
int AllocationCount;
int UnusedRangeCount;
long UsedBytes;
long UnusedBytes;
long AllocationSizeMin;
long AllocationSizeAvg;
long AllocationSizeMax;
long UnusedRangeSizeMin;
long UnusedRangeSizeAvg;
long UnusedRangeSizeMax;
}
class Stats
{
readonly StatInfo[] MemoryType;
readonly StatInfo[] MemoryHeap;
StatInfo Total;
}
class BlockAllocation { }

class VulkanMemoryAllocator
{
Device Device { get; }
VulkanMemoryAllocator(in VulkanMemoryAllocatorCreateInfo createInfo);
int CurrentFrameIndex { get; set; }
void Dispose();
ref readonly Silk.NET.Vulkan.PhysicalDeviceProperties PhysicalDeviceProperties { get; }
ref readonly PhysicalDeviceMemoryProperties MemoryProperties { get; }
MemoryPropertyFlags GetMemoryTypeProperties(int memoryTypeIndex);
int? FindMemoryTypeIndex(uint memoryTypeBits, in AllocationCreateInfo allocInfo);
int? FindMemoryTypeIndexForBufferInfo( in BufferCreateInfo bufferInfo, in AllocationCreateInfo allocInfo);
int? FindMemoryTypeIndexForImageInfo( in ImageCreateInfo imageInfo, in AllocationCreateInfo allocInfo);
Allocation AllocateMemory( in MemoryRequirements requirements, in AllocationCreateInfo createInfo);
Allocation AllocateMemoryForBuffer( Buffer buffer, in AllocationCreateInfo createInfo, bool BindToBuffer = false);
Allocation AllocateMemoryForImage( Image image, in AllocationCreateInfo createInfo, bool BindToImage = false);
Result CheckCorruption(uint memoryTypeBits);
Buffer CreateBuffer( in BufferCreateInfo bufferInfo, in AllocationCreateInfo allocInfo, out Allocation allocation);
Image CreateImage( in ImageCreateInfo imageInfo, in AllocationCreateInfo allocInfo, out Allocation allocation);
void FreeMemory(Allocation allocation);
Stats CalculateStats();
VulkanMemoryPool CreatePool(in AllocationPoolCreateInfo createInfo);
}

class VulkanMemoryPool
{
VulkanMemoryAllocator Allocator { get; }
string Name { get; set; }
void Dispose();
int MakeAllocationsLost();
Result CheckForCorruption();
void GetPoolStats(out PoolStats stats);
}

enum SuballocationType {}
enum AllocationRequestType{}
enum AllocationStrategyFlags {}
enum AllocatorCreateFlags {}
enum MemoryUsage {}
enum AllocationCreateFlags {}
enum PoolCreateFlags {}

AllocationRequest <-- AllocationRequestType
AllocationPoolCreateInfo <-- PoolCreateFlags

AllocationCreateInfo <-- MemoryUsage
AllocationCreateInfo <-- MemoryPropertyFlags
AllocationCreateInfo <-- AllocationStrategyFlags
AllocationCreateInfo <-- AllocationCreateFlags
AllocationCreateInfo --> VulkanMemoryPool : belongs optional to

AllocationContext <-- AllocationStrategyFlags
AllocationContext <-- SuballocationType
AllocationContext --> IBlockMetadata

Allocation --> Memory : occupies a chunk
Allocation --> Buffer : gets bound to
Allocation --> Image : gets bound to

VulkanMemoryAllocator <-- AllocationCreateInfo : for Allocations
VulkanMemoryAllocator <-- AllocationPoolCreateInfo : for AllocationPool
VulkanMemoryAllocator <-- VulkanMemoryAllocatorCreateInfo
VulkanMemoryAllocator --> Allocation : create
'VulkanMemoryAllocator --> Buffer : creates
'VulkanMemoryAllocator --> Image : creates
VulkanMemoryAllocator --> VulkanMemoryPool : creates

IBlockMetadata --> StatInfo : calculates
IBlockMetadata --> PoolStats : calculates
IBlockMetadata --> AllocationRequest : creates
IBlockMetadata --> BlockAllocation : alloc/free
Stats <--StatInfo

BlockAllocation --> Allocation

hide empty members
remove enum
remove AllocationPoolCreateInfo
remove AllocationCreateInfo
remove PoolCreateFlags
remove AllocationRequestType
remove AllocatorCreateFlags
remove SuballocationType
remove AllocationStrategyFlags
remove MemoryUsage
remove AllocationCreateFlags
remove MemoryPropertyFlags
remove VulkanMemoryAllocatorCreateInfo
remove Stats
remove StatInfo
remove PoolStats
@enduml

sunkin351 · 2021-10-03T21:24:37Z

The IBlockMetadata interface is responsible for book keeping for a singular block of Device Memory. It keeps track of what memory in said block is allocated and what memory is free to be allocated. That's its reason for existing.

p3t3rix · 2021-10-03T22:30:01Z

The IBlockMetadata interface is responsible for book keeping for a singular block of Device Memory. It keeps track of what memory in said block is allocated and what memory is free to be allocated. That's its reason for existing.

Ok i some time to look through the source and the interface is basically the extension point of this library via the AllocationPoolCreateInfo struct where you could specify custom pool allocation strategy. So we can't reduce the surface without sacrificing this feature (IBlockMetadata, allocation request/context would then be internal).

Otherwise i think the api is ok. When it gets in before the big 3.0 it can be battle tested and if there are some api changes that would be beneficial but breaking, they could be postponed to 3.0.

Perksey · 2022-02-20T14:33:57Z

It looks like there isn't a lot of movement on this, and 2.X isn't really where the Silk.NET team themselves are focusing at this time.

We (I) still want this in mainline Silk.NET, though, and we own the code now (#599) so we can always revisit this later down the line!

For now @sunkin351 did you want the original VMASharp to join the Silk.NET community program? Hopefully this will make the community more willing to use VMASharp in the interim period.

Perksey · 2022-05-13T18:02:02Z

I'm closing this as I won't be coming back to this, and with all maintainers being dedicated to 3.0 I'm not sure if anyone else would either. I've protected the branch, so it should stay put should anyone else want to pick this up.

Beyley · 2023-01-23T02:37:52Z

Re-opening since there's active work on 2.x again, and a plan for more often API review meetings and more coffee & code streams, once #1253 is in, ill PR to rebase this branch

…review

Perksey · 2023-01-27T15:21:14Z

Assigning @Beyley for API review given we don't really have any Vulkaneers around atm.

Beyley · 2023-01-27T18:07:00Z

Theres quite a few cases of IntPtr being used, which doesnt fall in line with Silk classes like SilkMarshal which use nint, and the Vulkan bindings in general which seem to like nint and void* and have no uses of IntPtr (posted normally so i dont create 6 review comments for one thing)

sunkin351 · 2023-01-27T18:45:35Z

Bear in mind, this was written in a time before nint and nuint were introduced and likewise has not been updated to reflect the newest standards.

sunkin351 · 2023-01-30T00:48:40Z

The original design of this API revolved around the ability to free "lost" allocations should they not be accessed in X (user defined) frames. If we don't wanna support that, we might be able to simplify the IBlockMetadata interface with a singular bool TryAllocate(...) method.

Edit: I'm probably forgetting some of the other things the convoluted design allowed...

ghost · 2023-09-20T13:51:13Z

Oh wee Oh wee Ifeel it. lol..

Create VMASharp_API_For_Review.md

6e56626

Update VMASharp_API_For_Review.md

db0c0c8

Perksey added area-Vulkan feature New feature. help wanted Extra attention is needed labels Aug 29, 2021

Perksey added this to the 2.X milestone Aug 29, 2021

Perksey mentioned this pull request Sep 4, 2021

Tracking issue for Silk.NET Vulkan Memory Allocator #605

Open

7 tasks

p3t3rix reviewed Oct 2, 2021

View reviewed changes

Update VMASharp_API_For_Review.md

7c72e27

Perksey assigned HurricanKai Oct 3, 2021

Perksey modified the milestones: 2.X, Future Nov 5, 2021

Perksey closed this May 13, 2022

Perksey deleted the feature/vmasharp_public_api_for_review branch November 13, 2022 14:31

Beyley restored the feature/vmasharp_public_api_for_review branch January 23, 2023 02:36

Beyley reopened this Jan 23, 2023

Merge branch 'feature/vmasharp' into feature/vmasharp_public_api_for_…

9015b95

…review

Beyley requested a review from a team as a code owner January 23, 2023 07:34

Perksey assigned Beyley and unassigned HurricanKai Jan 27, 2023

Move VMASharp proposed API over to nint (#1260)

4059d32

sunkin351 approved these changes Jan 28, 2023

View reviewed changes

VMASharp API Review #595

Are you sure you want to change the base?

VMASharp API Review #595

Conversation

Perksey commented Aug 29, 2021

Perksey commented Aug 29, 2021

p3t3rix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunkin351 Oct 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunkin351 commented Oct 3, 2021 • edited Loading

p3t3rix commented Oct 3, 2021

sunkin351 commented Oct 3, 2021

Perksey commented Oct 3, 2021 • edited Loading

Perksey commented Oct 3, 2021

p3t3rix commented Oct 3, 2021

p3t3rix commented Oct 3, 2021

sunkin351 commented Oct 3, 2021

p3t3rix commented Oct 3, 2021

Perksey commented Feb 20, 2022

Perksey commented May 13, 2022

Beyley commented Jan 23, 2023 • edited Loading

Perksey commented Jan 27, 2023

Beyley commented Jan 27, 2023 • edited Loading

sunkin351 commented Jan 27, 2023 • edited Loading

sunkin351 commented Jan 30, 2023 • edited Loading

ghost commented Sep 20, 2023

sunkin351 Oct 3, 2021 •

edited

Loading

sunkin351 commented Oct 3, 2021 •

edited

Loading

Perksey commented Oct 3, 2021 •

edited

Loading

Beyley commented Jan 23, 2023 •

edited

Loading

Beyley commented Jan 27, 2023 •

edited

Loading

sunkin351 commented Jan 27, 2023 •

edited

Loading

sunkin351 commented Jan 30, 2023 •

edited

Loading