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

System.Numerics.Vectors and Span<T> #25608

Closed
dangi12012 opened this issue Mar 24, 2018 · 14 comments · Fixed by #50062
Closed

System.Numerics.Vectors and Span<T> #25608

dangi12012 opened this issue Mar 24, 2018 · 14 comments · Fixed by #50062
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics Cost:S Work that requires one engineer up to 1 week
Milestone

Comments

@dangi12012
Copy link

dangi12012 commented Mar 24, 2018

Please think about implementing this operator:

public static partial struct Vector2
{
    public Vector2(ReadOnlySpan<byte> values);
    public Vector2(ReadOnlySpan<T> values);

    public readonly void CopyTo(Span<byte> destination);
    public readonly void CopyTo(Span<T> destination);
}

public static partial struct Vector3
{
    public Vector3(ReadOnlySpan<byte> values);
    public Vector3(ReadOnlySpan<T> values);

    public readonly void CopyTo(Span<byte> destination);
    public readonly void CopyTo(Span<T> destination);
}

public static partial struct Vector4
{
    public Vector4(ReadOnlySpan<byte> values);
    public Vector4(ReadOnlySpan<T> values);

    public readonly void CopyTo(Span<byte> destination);
    public readonly void CopyTo(Span<T> destination);
}

Since the span can be a stackallocated Array the performance gain should be huge! Also Vertical Operators are missing from Vector like T Min(Vector vector)

@ahsonkhan
Copy link
Member

ahsonkhan commented Mar 24, 2018

Thanks for the suggestion @dangi12012. Can you please refer to the API review process guidelines here.

The issue description should contain a speclet that represents a sketch of the new APIs, including samples on how the APIs are being used. The goal isn't to get a complete API list, but a good handle on how the new APIs would roughly look like and in what scenarios they are being used. Here is a good example.

For reference. here are the existing CopyTo methods on Vector3:

    public struct Vector3 : IEquatable<Vector3>, IFormattable
    {
        public void CopyTo(float[] array);
        public void CopyTo(float[] array, int index);
    }

cc @eerhardt

@dangi12012
Copy link
Author

dangi12012 commented Mar 25, 2018

If you add the following code to Vector2,3,4 and Vector:

public Vector3(Span<float> Elements)
{
    ElementPtr = Elements.ToArray();
}

public static implicit operator Vector3((float x, float y, float z) elements)
{
    return new Vector3(elements.x, elements.y, elements.z);
}

public void Deconstruct(out float x, out float y, out float z)
{
    x = X;
    y = Y;
    z = Z;
}
static void Showcase()
{
    Vector3 v = (3, 2, 1); //Asume we have vector or vector<T> SIMD

    Span<float> span = stackalloc float[3]; //Now we want to implement our own operator not implemented in SIMD
    v.CopyTo(span); //getting single element is very slow for vector<T>. Creating an array is also VERY slow compared to c++ SIMD. Solution: SPAN<T>

    //Take modulus
    span[0] = span[0] % 2.0f;
    span[1] = span[1] % 2.0f;
    span[2] = span[2] % 2.0f;

    Vector3 result = new Vector3(span); //Fast modulus. If you make Vectors special types like span no copy is needed here. (Span as backing field)
    var (x, y, z) = v; //This is support for Deconstruct
}

This is faster than what we currently have for not unsupported Vectormath. (Vector is very lacking with horizontal operations and intrinsics)
An update to support CopyTo(span) would be needed for perfromance critical code.

The speed can even be improved to 0 copy, with the new ref return in C# 7:
public ref float X() => ref ElementPtr.Span[0];

@msedi
Copy link

msedi commented Jun 26, 2019

I would widen this request to initialize a Vector with Span. Currently you always have to allocate an array to feed the new instance of Vector with new data.

In some scenarios where you really need performance (e.g. bilinear interpolation) you need to fill a lot of interpolation partners. Currently, first you have to load your interpolation partners in a newly allocated arrays, which you only need to create your Vector.

For this situations it makes much more sense to stackalloc the memory for your interpolation partners that you refill with new data in a loop. If the stackalloc'ed memory can be Spanned there's no additional cost in allocating memory.

@tannergooding
Copy link
Member

Vector<T> already has CopyTo(System.Span<T>). This was added in dotnet/coreclr#23333.

Vector2/3/4 do not have it and I would be fine with adding them, but would like the existing proposal (or a new one given it hasn't been updated by the OP in over a year), following the good example @ahsonkhan listed https://github.com/dotnet/corefx/issues/28433#issuecomment-375849157 put up before I mark this as api-ready-for-review.

@tannergooding
Copy link
Member

Tuple deconstruction should be in its own issue and follow the good example as well @dangi12012.

@msedi
Copy link

msedi commented Jun 26, 2019

@tannergooding: You are right, CopyTo is there, but correct me if I'm wrong there's nothing for the constructor, right? So I cannot do something like

Span<float> x = stackalloc float[]{10,20,30,40};
Vector<float> v = new Vector<float>(x);

Currently only this is possible:

float[] x = new float[]{10,20,30,40};
Vector<float> v = new Vector<float>(x);

which could cause pressure on the GC when doing this in a loop in x/x direction over many data points.

@tannergooding
Copy link
Member

The Vector(Span<T>) constructor has existed for over a year now and was added in dotnet/coreclr#16733.

The Vector(ReadOnlySpan<T>) constructor was added in the dotnet/coreclr#23333 PR I linked above.

@msedi
Copy link

msedi commented Jun 26, 2019

@tannergooding: Thanks a lot for pointing this out. I did it today, but I looked it up again. According to the docs it only works for .NET Standard 2.1 Preview, whereas I am on .NET Standard 2.0.

@benaadams
Copy link
Member

But you can do

float* x = stackalloc float[] { 10, 20, 30, 40 };
Vector<float> v = Unsafe.ReadUnaligned<Vector<float>>(x);

@msedi
Copy link

msedi commented Jun 26, 2019

@benaadams 👍 This might help. I'll give it a try.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@tannergooding
Copy link
Member

@dangi12012, could you please update the original post to follow the "good example" listed under step 1 of our API Review Process? You don't need to provide all sections, but ideally you would provide the rationale and proposed API surface sections as those help the API review process.

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Mar 4, 2020
@tannergooding tannergooding modified the milestones: 5.0.0, Future Jun 23, 2020
@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Jan 15, 2021
@bartonjs
Copy link
Member

bartonjs commented Jan 15, 2021

Video

We think spanifying the constructor and CopyTo methods makes sense, but don't think adding the byte-based variants makes sense at the time.

Semantics of these members should match what Vector<T> does.

namespace System.Numerics
{
    public static partial struct Vector2
    {
        public Vector2(ReadOnlySpan<float> values);

        public readonly void CopyTo(Span<float> destination);
        public readonly bool TryCopyTo(Span<float> destination);
    }

    public static partial struct Vector3
    {
        public Vector3(ReadOnlySpan<float> values);

        public readonly void CopyTo(Span<float> destination);
        public readonly bool TryCopyTo(Span<float> destination);
    }

    public static partial struct Vector4
    {
        public Vector4(ReadOnlySpan<float> values);

        public readonly void CopyTo(Span<float> destination);
        public readonly bool TryCopyTo(Span<float> destination);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 15, 2021
@tannergooding tannergooding added Cost:S Work that requires one engineer up to 1 week help wanted [up-for-grabs] Good issue for external contributors labels Jan 15, 2021
@C-xC-c
Copy link
Contributor

C-xC-c commented Mar 19, 2021

I would like to work on this if the issue could be assigned to me, thanks.

@danmoseley
Copy link
Member

Done! Let us know if you need help, @C-xC-c

@danmoseley danmoseley removed the help wanted [up-for-grabs] Good issue for external contributors label Mar 19, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 22, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics Cost:S Work that requires one engineer up to 1 week
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants