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.

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

[API Proposal]: GetLongOffset and GetLongOffsetAndLength #102114

Open
rick-de-water opened this issue May 11, 2024 · 8 comments
Open

[API Proposal]: GetLongOffset and GetLongOffsetAndLength #102114

rick-de-water opened this issue May 11, 2024 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime untriaged New issue has not been triaged by the area owner

Comments

@rick-de-water
Copy link

Background and motivation

I am building C# wrappers around modern graphics APIs (DirectX/Vulkan/Metal), which allow the user to allocate large blocks of GPU memory directly. These sizes can go beyond the capacity of a 32 bit integer, so the APIs always use 64 bit integers, even if the application itself is compiled as 32 bit.

To make interacting with these APIs more ergonomic, I wanted to add support for Index and Range. And while Index and Range won't be suitable for situations where you indeed exceed the maximum size of an int, most of the time your buffers and textures will be smaller than that.

Index and Range have a couple of convenience functions, specifically Index.GetOffset and Range.GetOffset. I would like to use these, but because they operate on int instead of long, they are unusable for APIs with 64 bit integers (at least without additional checks and casting).

My proposal is to add long versions of these APIs called GetLongOffset and GetLongOffsetAndLength, similar to other long versions of APIs such as LongLength, LongCount, etc.

API Proposal

The following methods are copies from their non long counterparts, just with int replaced with long.

namespace System;

public readonly struct Index
{
    public long GetLongOffset(long length)
    {
        long offset = _value;
        if (IsFromEnd)
        {
            offset += length + 1;
        }
        return offset;
    }
}

public readonly struct Range
{
    public (long Offset, long Length) GetLongOffsetAndLength(long length)
    {
        long start = Start.GetLongOffset(length);
        long end = End.GetLongOffset(length);

        if ((ulong)end > (ulong)length || (ulong)start > (ulong)end)
        {
            ThrowArgumentOutOfRangeException();
        }

        return (start, end - start);
    }
}

API Usage

This is how I currently use it, with GetLongOffsetAndLength implemented as and extension method.

public IMappedMemory Map(Range range)
{
    var (offset, length) = range.GetLongOffsetAndLength(memory.Description.Size);
    return memory.Map(offset, length);
}

Alternative Designs

  • Overload the methods: This would probably break some code somewhere that uses reflection, and is inconsistent with the long methods in other APIs.
  • Generic math: You could change the method to support generic math so that it works for any integer type. This does change the original signature however, probably breaking some existing code. I would also worry about the complexity and the performance impact that is brings.

Risks

This change would be purely additive, so there should be no risks.

@rick-de-water rick-de-water added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 11, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 11, 2024
@DaZombieKiller
Copy link
Contributor

Related: NIndex and NRange from #100924

@huoyaoyuan
Copy link
Member

LongLength/LongCount are legacy pattern. The latest pattern is to use nint.

Index/Range themselves are less usable because common collections/arrays in the BCL are still limited to 32 bit. Native-sized span is typically more useful.

@colejohnson66
Copy link

colejohnson66 commented May 11, 2024

With .NET 9, you’ll be able to could convert a Range into an NRange (from #100924), then use the new GetOffsetAndLength() to get a pair of nint:

(nint offset, nint length) = ((NRange)myRange).GetOffsetAndLength();

@rick-de-water
Copy link
Author

rick-de-water commented May 11, 2024

nint does not solve this problem, as it is 32 bit when compiling for 32 bit architectures. This is accessing the GPUs address range, which is separate from the CPUs address range, which nint represents. It would be like saying that FileStream.Seek should use nint, despite file sizes having nothing to do with the CPUs address range.

@vcsjones vcsjones added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 14, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

@tannergooding
Copy link
Member

This is accessing the GPUs address range, which is separate from the CPUs address range

Accessing the GPU memory requires mapping it into CPU memory, in which case you're still limited to 32-bits at a time (much less in practice due to general system overhead). This is why, for example, the D3D12_RANGE type operates over SIZE_T (nuint) despite things like D3D12_GPU_VIRTUAL_ADDRESS and D3D12_GPU_VIRTUAL_ADDRESS_RANGE operating over ulong (64-bits).

There is a very small subset of GPU commands which operate exclusively on the GPU side, such as AtomicCopyBuffer*, which take 64-bit ranges/slices instead. Those are really specialized enough that having your own helper API seems the best option given the specialized domain and general considerations over working with such addresses. Having a general purpose API in the BCL is overall error prone and likely to push typical users towards a pit of failure more than it will help users in the specialized domain (especially given you can trivially define an extension method that does this for your own codebase).

@rick-de-water
Copy link
Author

rick-de-water commented May 15, 2024

That's a fair assessment, most operations are indeed related to mapping to CPU memory. The main issue is the length parameter of GetOffset and GetOffsetAndLength, since the original memory allocation IS done with ulong, as seen in D3D12_HEAP_DESC, vkMemoryAllocateInfo, etc. Then you have to start building in checks since it could technically be larger than 2gb, which quickly gets frustrating and error prone.

I also have to say that implementing these extension methods yourself can be a bit trickier than initially expected. The value that is used internally by Index and the one that is exposed are not the same. GetOffset uses the internal value for what I assume is optimization purposes, but this is not available from the extension method. There is also a lot of [AggressiveInlining] going on, so making an extension method that is of the same quality and performance as one implemented by the runtime can be challenging.

@tannergooding
Copy link
Member

tannergooding commented May 15, 2024

The base address being 64-bits doesn't matter, you aren't passing the base address into GetOffset. You're computing an offset given the length of the underlying allocation and that length is fundamentally going to be restricted to 32-bits for all CPU scenarios on a 32-bit system. It could be more than 32-bits for a GPU exclusive scenario, but as indicated above those are much rarer.

In many cases, the allocations even for the GPU are similarly going to be restricted. A typical GPU has 8GB of VRAM and while some have more, it becomes less common. In practice, you can't and will never actually get the entire memory space in a single allocation due to general system overhead. In many cases the actual GPU APIs have restrictions on the maximum sizes allowed as well (for textures, buffers, etc; often well below even 1GB), often causing these buffers to be far smaller than the theoretical limit. For practicality reasons, you're going to be chunking and sharing that VRAM space across many allocations and will almost certainly never encounter one in a typical real world production app that is even close to 2GB in size.

If you're working in such a specialize domain where it is necessary, extension methods are perfectly suitable and that just solidifies such an API being a bad fit for the BCL. -- Plus if you're on a 64-bit computer, then NIndex and NRange will work just fine, even for these theoretical scenarios. Such specialized scenarios are likely going to require a 64-bit OS anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

6 participants