Changed IntPtr's internal value to readonly (and therefore immutable)

[tgiphil]

No description provided.

[benaadams]

Should it be a readonly struct then?
readonly struct IntPtr

[tgiphil]

@benaadams Oh yeah! That's a new feature in C# v7.2.

[jkotas]

As you can tell from these comments, the primitive types are treated in special way by tools our there. I wonder what all can break with this change.

[tgiphil]

Me too. Would the existing automated test suites catch this if it were a breaking change?

[jkotas]

The existing tests may not be sufficient to catch the issues. Changes like this introduce new combinations that the existing tests were not designed to exercise.

We have tried to mark Nullable<T> as readonly. All existing tests were green on the change. But it had to be rolled back because of it was identified as breaking change later. dotnet/corefx#24997 (comment)

[tgiphil]

Interesting! Although it appears System.Nullable may have been a special case.

However, I defer to your more qualified judgement in dropping this PR.

[jkotas]

What are the benefits of this change? If this change has any significant benefits, it should be done for all primitive types (Int*, Float, Double, Boolean, Char) for consistency and we need to understand the breaking potential of this change.

Also, a matching PR needs to be made in corefx to make the same change in contracts.

[tgiphil]

@jkotas I made the change for all the other primitive types, except Double and Single because their implementation of GetHashCode() passes a reference to the internal value to Unsafe.As().

This change is mostly semantic sugar as it would add an immutable constraint to the internal type. (+1) The field would also be marked initonly in the IL metadata. If the compiler has any optimizations dependent the initonly field, this may unlock an compiler optimization or two. (+1).

[jkotas]

compiler has any optimizations dependent the initonly field, this may unlock an compiler optimization or two

Do you have any real example of optimization that it unlocks?

[tgiphil]

@jkotas I don't have any real examples. In fact, I was reviewing the source code to better understand how the backend compiler could optimize away the load for static primitive types (like, address == UIntPtr.Zero). It seems the compiler could reason the load would be unnecessary because in this case UIntPtr.Zero static field is (should be) immutable and a fixed value (zero). In fact, why even store the field in memory when the value must always be zero.

Maybe one of the backend compiler gurus can provide some input?

[jkotas]

The JIT does the IntPtr.Zero optimization you are describing already. It treats it as intrinsic

coreclr/src/System.Private.CoreLib/shared/System/IntPtr.cs

Line 28 in 85374ce

[Intrinsic]

.

[tgiphil]

@jkotas Thanks! I missed that --- I thought [Intrinsic] were only for methods & properties.

[jkotas]

This does actually allow C# to generate more efficient IL in some cases. For example:

readonly struct Test
{
    public readonly uint x;

    static void Method() {
          Test x = default;
          x.x.ToString();
    }
}

Without this change, there will be a temp created on the stack that to store the field x. With this change, the temp goes away.

[jkotas]

I think this is interesting change to take. I would like it to be enabled uniformly for all primitive types, including signed types (Int32, Int64, etc.) and floating point types (Single, Double).

@tgiphil Are you still interested in working on this? Thank you!

[jkotas]

except Double and Single because their implementation of GetHashCode() passes a reference to the internal value to Unsafe.As()

We can add Unsafe.AsRef(in T source) to CoreLib to fix this problem.

[tgiphil]

@jkotas Yes, I'd be interested in working on this. I may need help with floating point types (Single, Double) because their implementation of GetHashCode() passes a reference to the internal value to Unsafe.As().

[tannergooding]

@tgiphil, there should be an Unsafe.AsRef method which takes in T and strips away readonly (returning ref T) just for cases like this, where the value isn't actually mutated. (there might also be an overload of As that just takes in, but I don't recall).

[tgiphil]

@jkotas The rest of the primitive types have been updated. The Unsafe.As calls were avoided by using BitConverter.DoubleToInt64Bits and BitConverter.SingleToInt32Bits methods instead.

[jkotas]

DoubleToInt64Bits, SingleToInt32Bits

What does that do to the GetHashCode performance?

[jkotas]

Also, could you please rebase or merge with current master to resolve the conflicts?

[tgiphil]

The rebase didn't work out as expected. I opened up another PR instead:

#18645

[jkotas]

Superseded by #18645