Added SIMD path for Vector3, Quaternion and Matrix4x4 operations

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.

[martenf]

Added SIMD path using Vector128 (and Vector256 for Matrix4x4) for:

Vector3.Cross(Vector3 vector1, Vector3 vector2)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	1.3289 ns	0.0531 ns	0.0708 ns	1.00	0.00
After	0.6613 ns	0.0399 ns	0.0833 ns	0.51	0.05

Vector3.Transform(Vector3 value, Quaternion rotation)

Method	Mean	Error	StdDev	Ratio	RatioSD
Before	3.922 ns	0.1034 ns	0.1380 ns	1.00	0.00
After	1.258 ns	0.0508 ns	0.0776 ns	0.32	0.02

Quaternion operator *(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
dotnet8	4.0531 ns	0.1008 ns	0.0990 ns	1.00	0.00
dotnet9	0.7203 ns	0.0312 ns	0.0292 ns	0.18	0.01
after	0.5752 ns	0.0185 ns	0.0173 ns	0.14	0.00

Quaternion operator /(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	6.010 ns	0.1399 ns	0.1240 ns	1.00	0.00
after	2.377 ns	0.0334 ns	0.0296 ns	0.40	0.01

Quaternion Concatenate(Quaternion value1, Quaternion value2)

Method	Mean	Error	StdDev	Ratio	RatioSD
before	4.166 ns	0.0344 ns	0.0321 ns	0.69	0.02
after	1.266 ns	0.0443 ns	0.0414 ns	0.21	0.01

Impl operator *(in Impl left, in Impl right)

Method	Mean	Error	StdDev	Ratio
before	8.009 ns	0.0856 ns	0.0801 ns	1.00
afterVector128	5.407 ns	0.0604 ns	0.0565 ns	0.68
afterVector256	3.142 ns	0.0616 ns	0.0546 ns	0.39

[martenf]

@dotnet-policy-service agree

[martenf]

@dotnet/area-system-numerics

[tannergooding]

I'm not immediately following where this implementation is from...

The standard naive algorithm is typically broken down to something like:

return((Quaternion.Conjugate(rotation)*newQuaternion(value,0.0f))*rotation).AsVector128().AsVector3();

(noting that Quaternion.operator *(Quaternion, Quaterion) is not currently marked as aggressively inline)

[martenf]

I got it from a private C library I wrote a decade ago which quotes a dead link.
The closest I could find on the internet after a quick search is this forum post which gets 95% of the way there and just misses the last 'simplification for readability' step.

[tannergooding]

Can we update this to follow the "standard" algorithm I gave above.

I expect we'll get better support for things like constant folding, more consistent performance across a range of hardware, and we can accurately quote the root implementation to a known source (which avoids any potential licensing issues): https://github.com/microsoft/DirectXMath/blob/main/Inc/DirectXMathVector.inl#L10307-L10317

IIRC, glm uses a similar algorithm for it's implementation

[martenf]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication. It will also always be slower then the reduced version because it contains significantly more math operation.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

To reduce any variation between different archs the implementation could be changed to

return (vVector + 2 * Cross(rVector, Vector128.Create(rotation.W) * vVector + Cross(rVector, vVector))).AsVector3();

which would give the JIT a bit more leeway, but I doubt it makes any differens.

[tannergooding]

The naive implementation is even slower then the dotnet8 implementation, even with the dotnet9 improvements to quaternion multiplication

This is in part because quaternion multiplication is not being inlined currently. But, also because it's doing some "unnecessary" work, as you indicated. But that can be accounted for as well.

I also don't see any potential licensing problem. I wrote the c# code. Btw it looks nothing like c code I based it on which I also wrote. Both version are based on math that cannot be copyrighted and that has been floating around the internet for at least decade at this point.

In general we require correct attribution to exist and based on the statement given above, the code (even if you authored it) was itself based on some internet blog/forum/site/etc post that can no longer be referenced (due to a dead link). Such a post may have itself may have been based on something else which overall makes it very difficult for us to use "safely" as the chain of citations is broken.

The actual requirements for attribution, whether or not something can be copyrighted, and the like can get quite complex. When there isn't a clear chain anymore, then it requires additional effort on our part, potentially even requiring checks with legal, so an actual lawyer can make the determination on whether or not it is safe to do.

.NET is a huge open source project depending on by millions. We must do the due diligence and ensure that all the boxes are checked.

[martenf]

What makes the result of Q*vQ any more correct that any of the other ways you could use to rotate a vector by the rotation saved in a quaternion? Rodrigues' formula is actually older than Hamilton's, but it's not like any of this is a universal law written in the stars. This entire conversation feels pointless so I will stop wasting everyone's time, close this pull request and just use a private library.

[tannergooding]

From a mathematical perspective they all produce the "same" result. The consideration is that they are not the same from a programmatic perspective due to additional considerations.

The intent was to push this PR in a direction where we could still improve the performance but without changing certain invariants that are extremely important to maintain. That requires more in depth consideration and documentation to show how those requirements are being met.

The intent was not to waste time or get into an argument over what is right vs wrong. The contribution was appreciated and was otherwise nearly in the right shape to be taken.

[martenf]

I know and appreciate that you weren't trying to waste any time, but I just don't have the spare time to have long debates about computational vs mathematical correctness right now, so I think it's just easier if I leave it at that so that someone else can make a pull request if they run into the same bottlenecks.

[tannergooding]

👍, are you fine with me cherry-picking your commits into a PR so I can fix the last couple bits of feedback and get merged? That way you can still get credit for the overall contribution

[martenf]

Sure

[tannergooding]

This new code is much harder to read for basically a cycle difference in perf (numbers above report 0.7 -> 0.5 ns). It may also not optimize as well when considering all platforms or ISA baselines (SSE2, SSE4.1, AVX2, AVX512F, AdvSimd, WASM, etc).

It's also substantially different from Concatenate despite them doing the "same thing" (just one is x * y and the other is y * x)

[martenf]

It's an operation that tends to be in the hot path for 3d applications so thought the reduced readability might be worth the extra performance on all the platforms I tested it on.

[tannergooding]

Its not that hot for 3D apps. The actual hot code tends to happen on the GPU instead.

Its especially not so hot that the complexity is worth 0.15ns (which is effectively 1 CPU cycle).

[tannergooding]

This comment is still pending. I don't think the new code is worth the 0.15ns savings (it's notably slightly slower on my box too). The code is more complex, harder to follow, and so I don't think the justification is there to take this part of the change.

.NET is huge, it is millions of lines of code. We ultimately have to balance readability, maintainability, security, perf, etc.

While Vector3 is itself a performance oriented type, we still have to factor in the other considerations and balance them out. It also applies beyond what a simple benchmark may measure, as we have multiple platforms, ISAs, and ABIs to consider. We also have to consider how certain optimizations may function (such as inlining and constant folding).

As such, even with it being a performance oriented type, there's many places where we are happy to give up a half nanosecond (which is in practice 1-5 instruction cycles) to improve the other areas.

[tannergooding]

@martenf is this something you're still working on? There's still some pending feedback above that needs to be resolved, mostly just around the tradeoff of additional complexity vs perf increase (that is, where the additional complexity isn't worth the 1-2 instruction or 0.1-0.2 nanosecond savings, so using a more readable/maintainable/portable implementation is preferred).

[martenf]

Sorry I was quite busy the last couple of weeks.
I think that except for Vector3.Transform they are all resolved, just not marked as resolved.