feat: Add numerics type conversions

[Doprez]

PR Details

This PR adds some useful extensions that are commonly used with external libraries since Numerics has become the standard.

Related Issue

#2131 this is a great example where something like this could be used.

Motivation and Context

the Bepu branch has this built into the library but its also used in the model importer as well as any library that may need these fast conversions.

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have built and run the editor to try this change out.

[timcassell]

I think if you pass by ref (use in and Unsafe.AsRef) and apply [MethodImpl(MethodImplOptions.AggressiveInlining)], the conversions should be basically free.

[Doprez]

I did some quick tests with Inlining and the performance was the same or similar on a few runs. It seems like the compiler does not make any significant changes with or witthout it to the compiled code, I tested with the Bepu library mentioned above.

Unless Im missing something I cant seem to use Unsafe.AsRef for conversion to another type?

[Eideren]

Here's a couple of variants with the jitted asm shown:
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABABgAIA5ANRjA2gCZyBec4gRgDpKBXfGFACWYXF1r0mAbgCwAKDLtuAJV4A7DEIFcAwhHwAHIQBtBAZUEA3ETFyy584gGYlSdsx3kA3vPJ/yvv7O5LgYULz05BIMUIyBfj5y/snsLgBmxhDYGOQIaOQAnvbJAL7xAUlBLpxu0UzkAEJ4MAAUGAAWQrhUdbHklgCUrAB85ACqarjYaTBcAIK4ADw0dDGM+b2Mwy2waf0Dxf7lwTXk6lMzUav1AAphOngYbZ3dK5J9gyPkAFQtm98DABklkOfmO1Q4tWufWUMDSzy65F2PWhzE+LFGEwuswWy02G1R22Rg1BFWSAG0ALIwDoQAAmAElDMYWtTaYzmQB5AyaCCTeYAcwFsFwuCElhgDLUxiEallAoGAF1wa4ru9mLC0lKZWoYHSEd1kW81vsvljpjilsamAT1US4ftSSrTptGkIMGBHgaUerTRjxpMLVwGu6HqE8ajbWttiTys7IWqTSGPY8pS1ZeQOojrR8hv7zTNg6HHhH1VGmDGDnHKmCa6lVa6ACq2DBNXCtHNogblRIpfyuqCsfrB5otKt15INWDYADWYwMjfaME5N1wY9JyWIAHYkaSynX41Dfc3QncoGGnp39j3yskB0PLFwzxf17f/FOYLP54vl6vXxOgh3KA92rTcISPE0TwwTUWivQYbwAvx7zYR8YPHPs/A/L8FyXFc13QjDt13cp92SQ9E3qKDNW1WU9Vg114LrXsMOQ4dqOlWj9QIvssLnHDf3wjdAOIutSKOOsTgTJsW2TT1Qno1FryYt8kMUwcUKLFN5O4lJeO/XC/x0zcgJAg8JPAii+ig2TUzUBTfUY5JmL7VjHxs0I0yM99pz4n88P/DD2BMkjQKCBNiBQRofP0gSxwQwLOC4TgAE4WgAIgAEX4fACnIAR2UzCByBgSZeFgcg8HwchEVwGUBXaDBjFy/BoBgJEYAMEUSowbJxTaiA9mwdrsGMcg0nUeghD5NKdP3EogA==

As you can see, the jit tends to inline such a small function regardless of the attribute.
The newly introduced BitCast seems like the best of both world here as the input doesn't need to be passed as ref/in to have improved assembly (see asm for C.TestBitcast).

[timcassell]

Ah nice, BitCast looks awesome. I was imagining it like this, which it looks like BitCast creates the same code in a cleaner way.

public static Vector2 ToStride(this in NVector2 v) => Unsafe.As<NVector2, Vector2>(ref Unsafe.AsRef(v));

The in modifier is to prevent pass-by-value copies (which you can see in the asm of TestBase vs TestRef).

as the input doesn't need to be passed as ref/in to have improved assembly (see asm for C.TestBitcast).

The assembly for Bitcast vs BitcastIn is different though. It looks like it's simply better with in (in case the JIT doesn't actually inline it, which is not guaranteed).

C.Bitcast(System.Numerics.Vector2)
    L0000: vzeroupper
    L0003: vmovq xmm0, rcx
    L0008: vmovq rax, xmm0
    L000d: ret

C.BitcastIn(System.Numerics.Vector2 ByRef)
    L0000: mov rax, [rcx]
    L0003: ret

[Doprez]

I went ahead and added both for user convenience.

There have already been many complaints in some of the existing libraries for confusion with the reference based Stride math so hopefully this will avoid confusion for users who just need the fast conversion even if they are missing out on a little bit of extra performance.

Also sorry for the late update, I have promised way to much with very little time but this was an easy fix I remembered about today 😆

[Doprez]

Nevermind on having both! I forgot that extensions dont care and the user should not notice it when converting.

[Eideren]

Should we implement this as an explicit conversion operator instead ?

[Doprez]

#1161 This is the only reason I am hesitant, I dont know what could have broken for other platforms due to this PR.

Otherwise I loved the idea since it was by far the simplest to use from a user perspective.

[ericwj]

RyuJIT doesn't recognize Stride's vectors as vectors. They are considered structs with 4-byte fields and the assembly generated for them reflects that.

There is not a relevant difference between using pointers, refs, or reinterpretation of bits with bitcast. Still a method body using x+y is about 100x faster using System.Numerics.Vector2 than with Stride.Core.Mathematics.Vector2. Although this should be ammortized at least partially for larger method bodies, the problem is that the compiler sees Stride vectors as regular structs instead of vectors.

It could be a serious boon to performance to add fields of type System.Numeric.Vector to the Stride types using FieldOffset(0). This will cause the alignment to be correct at the very least, such that e.g. Stride vectors never have to be loaded field-by-field, as I see happen in some benchmarks.

After which the conversion is just reading the vector-sized field and no byref, pointer or bitcast is necessary at all.

[Doprez]

Would it make sense to have the struct just be a wrapper around a private field of type System.Numerics? That way all internal math would be done on System.Numerics types.

public struct Vector2
{
    [DataMember(1)]
    public float X
    {
        get
        {
            return _internalVector.X;
        }
        set
        {
            _internalVector.X = value;
        }
    }
     
    [DataMember(1)]
    public float Y
    {
        get
        {
            return _internalVector.Y;
        }
        set
        {
            _internalVector.Y = value;
        }
    }

    private System.Numerics.Vector2 _internalVector;
}

Just spitballing for potential solutions of moving towards System.Numerics I did not run this through any tests to prove it would be better in performance.

[ericwj]

That is the general idea, although changing Vector2.X to be a property rather than a field is a binary breaking change.

So what I meant to do is like this:

[StructLayout(LayoutKind.Explicit)]
public struct Vector2 {
    [FieldOffset(0)] private System.Numerics.Vector2 v;
    [FieldOffset(0)] public float X;
    [FieldOffset(4)] public float Y;
}

With the added benefit that it will immediately compile all existing code and the move to use System.Numerics is entirely optional and doesn't have to be all at once.

[Eideren]

For the matrix PR, I planned on having it as a baked field offset as well, issue was that the jited asm for constructor append a setter for that field as well even if the rest of the setter takes care of setting the bytes of the other fields overlapping that one. So we do accelerate access to the numerics but we may introduce some performance degradation for ctor.
It's also a breaking change, with that field added in, the following is not supported anymore since all fields are not set before exiting the method:

Vector2 d;
d.X = 10;
d.Y = 20;
return d;

[ericwj]

This case is what Unsafe.SkipInit covers @Eideren.

In the example above one constructor would become:

Vector2(float x, float y) {
    X = x;
    Y = y;
    Unsafe.SkipInit(out v);
}

This sort of code would have to be repeated everywhere the compiler complains. And if you mean Stride users can't do that, since v is private, then perhaps adding deconstruction and conversion from tuple where suitable, such as for Vector2, could alleviate the problem. Still a breaking change, but its going to be fairly nice code.

[Eideren]

Right, sounds good, although, if there had to be breaking changes related to vectors, I would rather we introduce them all at once. We could add this optimization in as part of the move to numerics/silk math instead ?

[ericwj]

If the change is from your four-liner to Vector2 v = (20, 30), I wouldn't care if it was my code.

The suggestion here was instead of Bitcast or As<T, U>, with the added benefit that the JIT will know what the type really is and keeping it in appropriate registers or aligning it on a 16-byte stack address whenever reasonable.

On the general move from Stride's math to the builtin types, I would start yesterday with deprecations since not doing those keeps everything around for year three in a row. Whether with the extra field - which I wouldn't hesitate to make public either - or with more verbosity literally everywhere, the smoothest path is to just fix the old, non-vectorized code, perhaps even add some API's, so as to make an eventual transition nearly entirely code compatible and of equal performance.

[Eideren]

@ericwj sure, but stride doesn't just target programmers, and some of the programmers are just starting out. We can merge this in with a bitcast to have a non-breaking version and have another PR marked for 4.3 replacing the bitcast ?

I would start yesterday with deprecations

We're all stuck on other areas right now, feel free to start working on that if you want to :)

[Doprez]

I don't think there is any way around either a 4.3/5.0 release for a breaking change especially involving math. I also have another PR being worked on that will also be a pretty big breaking change for game start up including windowing so maybe we should start pooling these together as potential goals? Some sort of tagging or project tracking maybe @VaclavElias already started something like that?

Whichever is decided I have been trying to keep track of these conversations in discussions as much as possible. I feel like one of the issues with these conversations seeming to pop up often over a long period of time is with the part time nature of all of the devs. Coming back after breaks things are often forgotten or lost to the bowels of Discord or scatterd in issues/PRs.

[Doprez]

@Eideren The implicit conversions should be there from Ykafia, I left in the extensions as well but I can remove them if the implicit conversions work.

[Eideren]

All green on TC's side, awaiting your last change and @ericwj 's feedback

[Doprez]

Sweet, Ill remove the extensions today then. Should I move the implicit conversions to BitCast or are we happy with Unsafe.As for now?

[Eideren]

You can wait for Eric's feedback if you would rather not come back to this one twice, otherwise feel free to swap to bitcast.

[Doprez]

Im ok to come back and change again if needed, this is a pretty small PR for me to make changes to if I need to.

[ericwj]

This is in fact a transpose followed by a cast. Better to write it like that, remove the code duplication. Better yet, while you're at it, cast it first, then transpose, so the transpose is optimized. Little effort to then go and replace the Transpose method doing it like that, too, with 2 casts - which should both be free.

[ericwj]

We can merge this in with a bitcast to have a non-breaking version and have another PR marked for 4.3 replacing the bitcast?

Yes. Sure. With regards to the method of doing the cast. There is no difference between Unsafe.As, Bitcast and pointer cast. As is by-ref, *(U*) is the unsafe version of the same and Bitcast does it by-value. Whatever how it is written, it should be inlined and vanish from disassembly in the vast majority of cases. Basically all they do is modify the type of the stackslot in the IL. None of them do something imperitively, like touching registers or memory.

I don't think there is any way around either a 4.3/5.0 release for a breaking change especially involving math.

I believe still the promise is that there are no breaking changes unless the API has been deprecated for a year, or one whole release cycle - not sure of the exact wording here. That is why the deprecations are so important to do.

[Doprez]

Transposing with Stride and using the Stride subtract method for the benchmark

public static SMatrix NumericToStrideUnsafe(this NMatrix v)
{
    SMatrix nm = Unsafe.As<NMatrix, SMatrix>(ref v);
    nm.Transpose();
    return nm;
}

transposing with Numerics before casting and using the Numerics subtract method for the benchmark

public static SMatrix NumericToStrideUnsafe(this NMatrix v)
{
    v = NMatrix.Transpose(v);
    SMatrix nm = Unsafe.As<NMatrix, SMatrix>(ref v);
    return nm;
}

That's quite the difference... I guess that's what you were saying with the 2 casts in the Stride transpose as well?

[Doprez]

Doing a few more tests, I think I see what you mean with the methods we can deprecate and maybe even just use Numerics math for the extra benefits without things breaking or even changing for the end user. I'll save it for a second PR but I have another thing I can work towards for #2576 with a much better understanding now.

One of the quick tests using the Numerics subtract method is absolutely insane even with the conversion from Stride to Numerics Matrix:

[Benchmark]
public void Matrix_TestNumericToStrideWithUnsafeAs()
{
	for (int i = 0; i < Iterations; i++)
	{
		NMatrix.Subtract(SMatrix.StrideToNumericUnsafe(), SMatrix.StrideToNumericUnsafe());
	}
}

[Doprez]

So if we convert the math methods to use Numerics as the parameters and use the Numerics math as the underlying method body we can still return a Stride value and gain some benefits.

public static Matrix Subtract(MatrixDotnet left, MatrixDotnet right)
{
    return MatrixDotnet.Subtract(left, right);
}

The 3ms record is with the new subtract method still returning the Stride value and the 6ms is the default Stride subtract method. if we only use Numerics it seems to consistently be 2ms which is insane to me.

We could also just return a Numeric value and it seems to be free like Eric was saying but returning a different type might be very confusing to the end user even though it should work the same due to the implicit conversions.

[ericwj]

Yes, although

• I wouldn't add any surface area to Stride's math in terms of parameters it accepts. It accepts Stride types and returns Stride types now. Whatever how it is implemented is a private detail and that can use System.Numerics types. In addition these conversion methods what this PR is about make it a lot easier to mix-and-match, so let people use them.
• Don't deprecate math that is perfectly fine and which can be optimized by changing private implementation. Deprecate performance smells such as by-ref and in-place modification (i.e. void Transpose() and void Add(ref ro, ref ro, out), Subtract(in, in) etc)
• One goal is to get Stride's math DRY. It is so freaking silly to have a handful of implementations for one operation! So if any Stride math is touched, for the purpose of deprecations or otherwise, have them all delegate to the most performant implementation. This will be the by-value one, which would work for readonly struct. And let the JIT handle the ref in ref ro dilemmas - it should often be able to inline and hide the smell.

[timcassell]

Deprecate performance smells such as by-ref

How is that a performance smell? Passing large structs by ref is recommended for performance. Passing by value causes it to be copied.

[Eideren]

The JIT has a specialization for numerics, it shoves them directly in wide registers when passed as arguments afair

[Eideren]

Thanks !