Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Proposal: by-ref struct operator parameters #6628

Closed
jeske opened this issue Jul 6, 2016 · 11 comments
Closed

Proposal: by-ref struct operator parameters #6628

jeske opened this issue Jul 6, 2016 · 11 comments

Comments

@jeske
Copy link

jeske commented Jul 6, 2016

Related to: dotnet/roslyn#165, dotnet/roslyn#115

One substantial performance gap between C# and C++ is C#'s high overhead of using struct operators, because of their pass by value copying semantics. In a simple overhead test for a relatively small vect3d type (vector of 3 doubles), the cost of pass-by-value is 3x vs implementing the same function with ref arguments. As the struct types become larger, the overhead of pass-by-value grows even higher.

public struct vect3d
{
    public double x, y, z;
    public static vect3d operator +(vect3d a, vect3d b)
    {
        a.x += b.x;
        a.y += b.y;
        a.z += b.z;
        return a;
    }
}

While C# offers many advantages over C++, software which spends a significant amount of time performing mathematical operations on struct datatypes is taxed by their performance overhead. Using non-operator functions is not a practical solution, as mathematical code becomes too hard to read.

One possibility is to combine by-ref operators with dotnet/roslyn#115 (readonly parameters), to enable higher performance struct operator implementations while enforcing (or encouraging) immutable value-type semantics for the caller.

public struct vect3d
{
    public double x, y, z;
    public static vect3d operator +(readonly ref vect3d a, readonly ref vect3d b)
    {
        vect3d r;
        r.x = a.x + b.x;
        r.y = a.y + b.y;
        r.z = a.z + b.z;
        return r;
    }
}

If readonly is merely a construct of the C# compiler, then the parameters are not immutable WRT to CLR. This is analogous to the const references used in C++ operators, which are not strictly enforced. In practice, C++ operators are generally well behaved in respecting immutable intent of these const arguments, and I believe C# "readonly ref" operator overloads would also be well behaved - while achieving significant performance improvement from avoiding copies.

It may be advantageous to allow value-copy and ref overloads of the same operator, to maintain backward compatibility with already compiled code. For new resolutions, the compiler would prefer the ref version of the operator overload. One option would be for the compiler to only allow a single matching overload (either readonly-ref or non-ref), and in the case of "readonly ref" it could generate the non-ref version for compatibility.

If the CLR and verifier are taught about "readonly ref", and they are allowed more generally in function parameters, the compiler could allow the user to omit the "ref" decorator for any "readonly ref" parameter in a function invocation, since "readonly" assures that parameter will maintain value type semantics. In this case, it might be advantageous to look for alternative syntax options, to avoid a noisy proliferation of "readonly ref" on many value-type parameter declarations. One option is to make non-async struct parameters pass as "readonly ref" by default. However, for very small structs (like IntPtr) pass by reference is marginally slower than pass by value. (20% slower in one trivial test) Another option is to use a shorter keyword, such as "in", matching up "in/out/ref".

@HaloFour
Copy link
Contributor

HaloFour commented Jul 6, 2016

readonly parameters, as proposed, is nothing more than a compiler check to ensure that the parameter (or local) in question is not reassigned. That fact would be lost during compilation since it's not considered relevant to the caller. To change this as your proposal indicates would require adding new metadata around parameters and modifying the CLR to understand and enforce the concept of readonly parameters.

That said, I don't think that said metadata or enforcement is really necessary to provide ref operators on value types as you've described. If it's improper to allow the operator to modify the operand the C# compiler could enforce that the ref parameters are readonly, either effectively or explicitly, otherwise error on the definition of the operator. That does leave some window for abuse of modifying the operand by operators defined outside of C#, but I imagine that the potential would be pretty low. Then there is the question of overloading. Can the same operator be defined with a ref type and a non-ref type as operands? If so, which does the compiler prefer during overload resolution?

@jeske
Copy link
Author

jeske commented Jul 6, 2016

I didn't mean to imply that CLR changes are required. Compiler enforced "readonly" for ref operator parameters seems worthwhile and sufficient. However, if the CLR and verifier were changed to understand and enforce "readonly ref", it might be worthwhile to use it for all non-async struct parameters rather than require the noisy declarations. I've reworded to clarify.

I suspect, for backward compatibility, we'd want to allow the same operator (and types) to be defined as both ref and non-ref. This would allow pre-existing binaries to continue to link against the non-ref version of the operator. The compiler overload resolution would pref the ref version of operators. An alternative would be for the compiler to only allow the ref versions, and generate the non-ref versions for compatibility.

@qrli
Copy link

qrli commented Jul 7, 2016

1st: Should it also use ref return?
JIT may optimize the return copy away. But I'm not sure it will optimize all copies for the return.

2nd: Does readonly ref parameter accept r-value argument?
r-value is common with operators. If we don't allow ref return, it should be safe to accept r-value.

@orthoxerox
Copy link

@qrli Why should a ref-return be useful for value types at all? Unless it's a cast operator, of course, where I wouldn't mind being able to pass a reference from an inner struct, e.g.

public struct SmallerStruct
{
    ... some fields
}

public struct LargerStruct
{
    public readonly SmallerStruct BaseVersion;
    ... more fields

    public static implicit operator ref SmallerStruct(readonly ref LargerStruct @this) => @this.SmallerStruct;
}

@qrli
Copy link

qrli commented Jul 9, 2016

@orthoxerox Same reason as ref parameters, to remove redundent copies to get better performance.
If the JIT can optimize that return copy away, then it is not necessary.

    public static vect3d operator +(readonly ref vect3d a, readonly ref vect3d b)
    {
        vect3d r; // local variable
        r.x = a.x + b.x;
        r.y = a.y + b.y;
        r.z = a.z + b.z;
        return r; // r is copied to caller
    }

@jeske
Copy link
Author

jeske commented Jul 9, 2016

While the ref-returns proposal (#118) doesn't make this clear, AFAIK it's not safe for CLR ref returns to refer to values on the stack, as those values will be potentially over-written after returning.

In order to safely avoid the return copy for general struct operators, the caller would need to pre-allocate the return space and pass in a reference. This might look something like:

public static void operator +(readonly ref vect3d a, readonly ref vect3d b, out vect3d result)

@Thaina
Copy link

Thaina commented Oct 1, 2016

Well I wish we could have readonly ref but, as always, I wish it could be use keyword const instead

It shorter and could be assume being ref

public static void operator +(const vect3d a, const vect3d b, out vect3d result)

Also I love @jeske idea that making operator function with out
But I think it might be a lot more complicate than that if we like to implement. It would be another huge change

Simple problem is, what it would look like if

var a = vec3d.one;
var b = vec3d.one;

var c = a + b; // easy , we can tranform this code to vec3d.+(a,b,outc)
var d = (a + b) + (a - b); // This is hard parts

We can't use d to temp (a + b) and (a - b) at the same time. So it need to alloc at least another vec3d automatically

Aside from that I think we have another way to deal with return ref, using this or base keyword in static function

public static out vec3d operator +(const vect3d a, const vect3d b)
{
    // Normally static function can't use base or this keyword
    // But with that out keyword present, then this static function can use base keyword
    base.X = a.X + b.X;
    base.Y = a.Y + b.Y;
    base.Z = a.Z + b.Z;
}

var a = vec3d.one;
var b = vec3d.one;
var c = a + b; // It pick this c tobe base

@orthoxerox
Copy link

I've done some checking, and there's nothing in the compiler that prevents you from having a ref operator other than the spec itself:

• The parameter(s) of an operator must be value parameters (§5.1.4). It is a compile-time error for an operator declaration to specify ref or out parameters.

I've replaced the check with an out restriction, and the compiler works rather well, replacing ldloc with ldloca:

// {
IL_0000 nop       
// var a = new RefVector3f(1, 2, 3);
IL_0001 ldloca.s  a
IL_0003 ldc.r4    1
IL_0008 ldc.r4    2
IL_000D ldc.r4    3
IL_0012 call      System.Void Sandbox.RefVector3f::.ctor(System.Single,System.Single,System.Single)
// var b = new RefVector3f(4, 5, 6);
IL_0017 ldloca.s  b
IL_0019 ldc.r4    4
IL_001E ldc.r4    5
IL_0023 ldc.r4    6
IL_0028 call      System.Void Sandbox.RefVector3f::.ctor(System.Single,System.Single,System.Single)
// var c = a + b;
IL_002D ldloca.s  a
IL_002F ldloca.s  b
IL_0031 call      Sandbox.RefVector3f Sandbox.RefVector3f::op_Addition(Sandbox.RefVector3f&,Sandbox.RefVector3f&)
IL_0036 stloc.2   
// var d = RefVector3f.Add(a, b);
IL_0037 ldloc.0   
IL_0038 ldloc.1   
IL_0039 call      Sandbox.RefVector3f Sandbox.RefVector3f::Add(Sandbox.RefVector3f,Sandbox.RefVector3f)
IL_003E stloc.3   
// var e = RefVector3f.Add(ref a, ref b);
IL_003F ldloca.s  a
IL_0041 ldloca.s  b
IL_0043 call      Sandbox.RefVector3f Sandbox.RefVector3f::Add(Sandbox.RefVector3f&,Sandbox.RefVector3f&)
IL_0048 stloc.s   e
// }
IL_005D ret    

It even supports stuff like one parameter being ref, and the other not automatically (useful for things like int*Vector). Declaring two operators with one having ref parameters and the other not results in a call site error. Making this a declaration site error is probably the only change that has to be done to the compiler to support ref operators, if the LDM is willing to change the spec, of course.

@jeske
Copy link
Author

jeske commented May 25, 2019

It looks like this issue was addressed in C# 7.2's Safe Efficient Code Enhancements. See also writing safe efficient C#.

It looks like the technique requires declaring the struct readonly (immutable), and then using "in" modifiers on arguments. The result looks like this:

readonly public struct Point3D
{
    double X, Y, Z;
    public Point3D(double x, double y, double z)
    {   this.X = x;   this.Y = y;   this.Z = z;   }

    public static Point3D operator +(in Point3D a, in Point3D b) => 
       new Point3D(a.X+b.X,a.Y+b.Y,a.Z+b.Z);
}

You can also declare return values as readonly ref to avoid extra copies.. but I'm not sure how this works with operators.

@RayKoopa
Copy link

RayKoopa commented May 18, 2020

This would be very useful for Matrix4x4 or similar large structs, to get them up to speed with a Matrix4x4 operator*(const Matrix4x4& lhs, const Matrix4x4& rhs) C++ operator for example (given RVO). But making them readonly to ensure no copies happen when passing them as in operator parameters is awkward as sometimes single value manipulation is needed without creating a completely new matrix.

Or is the compiler or runtime clever enough to see fields of passed-in matrices are only read from, preventing a defensive copy to happen?

So far I always have to use extremely ugly static methods to prevent needless copies, like void Matrix4x4.Multiply(out Matrix4x4 result, ref Matrix4x4 lhs, ref Matrix4x4 lhs);, where I cannot even pass in useful constants like public static readonly Matrix4x4.Identity = ... - what the hell? :D

@ritchiecarroll
Copy link

ritchiecarroll commented Jul 10, 2020

How about an overall ref operator for a class:

public sealed class MyClass<T>
{
    private T m_value;

    public MyClass(in T value) => m_value = value;

    public ref T Value => ref m_value;

    // Example of new proposed "ref" return operator for class:
    public static ref T operator ref(MyClass<T> value) => ref value.m_value;
}

Now code like this:

    var x = new MyClass<int>(12);
    ref var y = ref x.Value;
    y = 13;

Could become:

    var x = new MyClass<int>(12);
    ref var y = ref x;
    y = 13;

This basically proxies ref return operations on a class, that would normally not allow this, to its ref operator.

Per previous comment by @RayKoopa, this would allow results for large structures by doing something like the following:

public unsafe struct Matrix4x4
{
    public fixed double Values[16];
    public ref double this[int x, int y] => ref Values[x * y];
}

public class MatrixMultiplyOp
{
    private Matrix4x4 m_result;

    public MatrixMultiplyOp(in Matrix4x4 lhs, in Matrix4x4 rhs)
    {
        for (int i = 0; i < 4; i++)
            for (int j = 0; j < 4; j++)
                m_result[i, j] = lhs[i, j] * rhs[i, j];
    }

    public ref Matrix4x4 Result => ref m_result;

    // Example of new proposed "ref" return operator for class:
    public static ref Matrix4x4 operator ref (MatrixMultiplyOp value) => ref value.m_result;
}

public static class Matrix
{
    public static MatrixMultiplyOp Multiply(in Matrix4x4 lhs, in Matrix4x4 rhs) => new MatrixMultiplyOp(lhs, rhs);

    // FYI - something like this already works today for this example, but the above code is more simple
    //public static ref Matrix4x4 Multiply(in Matrix4x4 lhs, in Matrix4x4 rhs) => ref new MatrixMultiplyOp(lhs, rhs).Result;
}

static void Main()
{
    // Used like this:
    var lhs = new Matrix4x4();
    var rhs = new Matrix4x4();
    ref var result = ref Matrix.Multiply(lhs, rhs);
}

@dotnet dotnet locked and limited conversation to collaborators Oct 28, 2022
@CyrusNajmabadi CyrusNajmabadi converted this issue into discussion #6629 Oct 28, 2022
@CyrusNajmabadi CyrusNajmabadi transferred this issue from dotnet/roslyn Oct 28, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants