Replace ImmutableAttribute with IImmutable interface

[jason-bragg]

The immutable attribute is used to indicate that the data in the class being provided to a grain call will not be modified, so it's safe to pass it without making a copy of the data. This is a grain call optimization. There are two ways to mark data as immutable for grain calls.

Immutable attribute marker

[Serializable]
[Immutable]
public class ImmutableBlarg
{
    public int A { get; private set; }
    public ImmutableBlarg(int aval)
    {
        A = aval;
    }
}

The attribute marks the ImmutableBlarg class as immutable so when it's used in a grain call it will not be copied. This is the preferred method as it is safer, as the type itself enforces the immutability, but it requires that the type be modifiable, so it can't be used if one wants to pass a basic type like byte[] or third party types.

Generic wrapper marker

public interface ISomeGrain : IGrainWithIntegerKey
{
    Task DoSomething(Immutable<Blarg> data);
}

The Immutable generic wrapper indicates that the values in Blarg will not be changed, so it's safe to not make a copy of the object during the call. This is useful for objects that are not inherently immutable or those who's type information cannot be modified.

The use of immutable data is a valuable optimization, but using an attribute is an inefficient approach. The issue is that getting attributes is one of the more expensive reflection calls, and the use of this attribute means that we must incur this call on every argument on every grain call, regardless if the attribute is use or not.

I propose replacing the ImmutableAttribute with an IImmutable interface, and use an inheritance check (which is cheaper) instead of an attribute check.

Something like:

// Immutable marker interface
public class IImmutable
{
}

// Use Immutable interface to mark classes as immutable
[Serializable]
public class ImmutableBlarg : IImmutable
{
    public int A { get; private set; }
    public int B { get; private set; }

    public ImmutableBlarg(int aval, int bval)
    {
        A = aval;
        B = bval;
    }
}

The use of an interface rather than an attribute reduces the overhead of checking for immutable data. This should result in faster grain calls.

I'd like to introduce the IImmutable interface and mark the ImmutableAttribute as deprecated for Orleans 1.2.0 and remove ImmutableAttribute entirely in 1.3.0.

Thoughts/Opinions

[ReubenBond]

We could cache the result in IsOrleansShallowCopyable like we do with non-generic value types, although that might not be faster than using IImutable + an assignability check.

[jason-bragg]

@ReubenBond
Yeah, that could work. I'm kinda allergic to the locks in that code and wouldn't want to add more, but I've not spent time figuring out a better approach. My suspicion is same as yours; that an inheritance check would be faster than a lock+cache check, but the approach is worth consideration.

[gabikliot]

I would not use interface for that.

1. We have already been asked to reduce the dependence on Orleans.dll - to make this attribute PCL complaint - look at Immutable attribute in PCL #210. Turning it into an attribute will move us away from that.

2. Using an empty interface to describe a behavior is non-idiomatic in .NET (I finally learned what this word means). Interfaces describe activity, dynamic behaviors, attributes describe static capabilities. Look at:
   https://msdn.microsoft.com/en-us/library/ms229022.aspx " AVOID using marker interfaces (interfaces with no members).
   If you need to mark a class as having a specific characteristic (marker), in general, use a custom attribute rather than an interface."

3. I know @jason-bragg you did a micro test at some point and saw that attribute has a higher cost, but I was not convinced then, and not now, that in the end to end test it will matter. So this sounds like maybe a small not worth optimization at the price of the programming model.
   Instead, I would experiment with caching, like @ReubenBond suggested, and compare the option with ConcurrentDictionary vs. attribute. There are multiple other ways we can do it at runtime as well, with combination of immutable dictionaries and ConcurrentDictionary .
   I would start with having an end to end workload benchmark where you tried both approaches - interface and attribute, just to compare the total potential saving cost.

I am however NOT a strong opposer to this idea. If you think it is still worth it, and the programming model "cost" is insignificant, I will not object. I just think this is the wrong place to spend cycles. There are other places where we can save much more.

[veikkoeeva]

As every class in the hierarchy inherits the marker interface, would it be obvious for the user that all the classes in the hierarhcy should be immutable?

[sergeybykov]

I am with @ReubenBond and @gabikliot here - caching is fast and relatively easy. We do that for a lot of other type info. If we need to refactor and improve the type info caching code, that a different question to me that shouldn't have any weight on the conceptual decision.

@veikkoeeva brought a subtle but important point that I think shows that the interface approach may be prone to dev mistakes due to not realizing that a particular derived type is considered immutable by the runtime via non-obvious inheritance.

[jason-bragg]

Agreed. An interface, while cheaper perf wise, is probably not the right approach.

Consensus seems to be that if we want to optimize this, which is not a given, we should continue to use attribute, but cache the results so we don't have to incur the cost of the reflection hit each call. If caching is slower than reflection, we can discard change or optimize caching of type information.

Any objections / alternatives we should consider?

[gabikliot]

I think that has now became mostly irrelevant, after @dVakulen's major improvement in #1586. So closing for now.