Skip to content
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

Remove Interlocked.{Compare}Exchange generic constraint #65184

Closed
GSPP opened this issue Feb 11, 2022 · 10 comments
Closed

Remove Interlocked.{Compare}Exchange generic constraint #65184

GSPP opened this issue Feb 11, 2022 · 10 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@GSPP
Copy link

GSPP commented Feb 11, 2022

EDITED by @stephentoub on 7/9/2024 to update the proposal:

Remove the T : class constraint from the existing Interlocked.{Compare}Exchange generic methods. They will be usable with not only reference type Ts but also primitive and enum Ts. Unsupported Ts will result in a NotSupportedException.

namespace System.Threading;

public static class Interlocked
{
    public static T CompareExchange<T> (ref T location1, T value, T comparand)
-       where T : class

    public static T Exchange<T>(ref T location1, T value)
-       where T : class
}

(We could also consider removing / making internal the new public overloads added in .NET 9 for byte/sbyte/ushort/short and just have the generic version to handle those.)


In principle, Interlocked.CompareExchange can be made to work on enums. Enums are just integers in disguise.

Motivating for this request is a repeated need to update a "state" field in a class. The state is most cleanly tracked as an enum. Since this API does not exist yet, I use integers and constants. That's of course viable but it would be cleaner to be able to use enums.

A workaround is to use Unsafe. There is nothing wrong with that in principle. It could be "nicer", though.

There's a recent discussion about small integer types that is pertinent to this issue:

This is related because enums can have small integer types. If we don't allow small types, then the generic constraint : enum would not ensure runtime safety.

So maybe we can add enum overloads iff we add small integer types. The alternative would be to add it even without that and fail at runtime.

Design issues:

  1. What APIs should be augmented like this in addition to CompareExchange. I can tell from the existing discussion that the team only wants to add API scope if there is demand. In principle, all Interlocked APIs could be upgraded.
  2. Should there be an API to process any unmanaged type of at most 64 bits in size?
    a. This would mean that there could be a runtime error depending on T. If T : unmanaged, then the JIT can ensure that this validation is without runtime overhead because each struct type generates specialized code.
    b. Could this result in unaligned access?
  3. Taking this further: There could be a generic type constraint "interlocked". This would then even work with things like ImmutableArray. Most likely, this is not going to happen, but I'm mentioning this for completeness.

For visitors looking for a workaround:

static T InterlockedCompareExchange32<T>(ref T location, T value, T comparand) where T : unmanaged
{
    var result = Interlocked.CompareExchange(ref Unsafe.As<T, int>(ref location), Unsafe.As<T, int>(ref value), Unsafe.As<T, int>(ref comparand));
    return Unsafe.As<int, T>(ref result);
}
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 11, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@MihaZupan
Copy link
Member

MihaZupan commented Feb 11, 2022

For reference / as an example, we are working around the lack of such an API in Uri:

@MihaZupan MihaZupan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 11, 2022
@ghost
Copy link

ghost commented Feb 11, 2022

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

In principle, Interlocked.CompareExchange can be made to work on enums. Enums are just integers in disguise.

Motivating for this request is a repeated need to update a "state" field in a class. The state is most cleanly tracked as an enum. Since this API does not exist yet, I use integers and constants. That's of course viable but it would be cleaner to be able to use enums.

A workaround is to use Unsafe. There is nothing wrong with that in principle. It could be "nicer", though.

There's a recent discussion about small integer types that is pertinent to this issue:

This is related because enums can have small integer types. If we don't allow small types, then the generic constraint : enum would not ensure runtime safety.

So maybe we can add enum overloads iff we add small integer types. The alternative would be to add it even without that and fail at runtime.

Design issues:

  1. What APIs should be augmented like this in addition to CompareExchange. I can tell from the existing discussion that the team only wants to add API scope if there is demand. In principle, all Interlocked APIs could be upgraded.
  2. Should there be an API to process any unmanaged type of at most 64 bits in size?
    a. This would mean that there could be a runtime error depending on T. If T : unmanaged, then the JIT can ensure that this validation is without runtime overhead because each struct type generates specialized code.
    b. Could this result in unaligned access?
  3. Taking this further: There could be a generic type constraint "interlocked". This would then even work with things like ImmutableArray. Most likely, this is not going to happen, but I'm mentioning this for completeness.

For visitors looking for a workaround:

static T InterlockedCompareExchange32<T>(ref T location, T value, T comparand) where T : unmanaged
{
    var result = Interlocked.CompareExchange(ref Unsafe.As<T, int>(ref location), Unsafe.As<T, int>(ref value), Unsafe.As<T, int>(ref comparand));
    return Unsafe.As<int, T>(ref result);
}


<table>
  <tr>
    <th align="left">Author:</th>
    <td>GSPP</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`api-suggestion`, `area-System.Threading`, `untriaged`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@quixoticaxis
Copy link

quixoticaxis commented Apr 3, 2022

Is it even possible to implement this API effectively without making it a special case in the compiler?
AFAIU, you want the API to implicitly cast the enum to its underlying type, and it cannot be done without accessing the type information through Enum.GetUnderlyingType and switching on the result in runtime which may not be fast enough.
The only other way is to force the compiler to inject no-op casts at CompareExhange call sites.

And sure the shorter integer types would have to be supported first anyway.

@jkotas
Copy link
Member

jkotas commented Apr 3, 2022

Is it even possible to implement this API effectively without making it a special case in the compiler?

Right, the implementation of this API would have to be special-cased as runtime intrinsic. It is not something that can be done efficiently in pure C# today.

@janvorli janvorli added this to the Future milestone Jun 15, 2022
@janvorli janvorli removed the untriaged New issue has not been triaged by the area owner label Jun 15, 2022
@NinoFloris
Copy link
Contributor

This results in the desired codegen though, we've been using this for a while in our codebase.
Optionally you could add the Enum constraint, does this approach have some fundamental flaw I'm unaware of?

using System;
using System.Runtime.CompilerServices;
using System.Threading;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
static unsafe T CompareExchange<T>(ref T location, T value, T comparand)
    where T: unmanaged
{
    switch (sizeof(T))
    {
        case sizeof(int):
            var x = Interlocked.CompareExchange(ref Unsafe.As<T, int>(ref location), Unsafe.As<T, int>(ref value), Unsafe.As<T, int>(ref comparand));
            return Unsafe.As<int,T>(ref x);
        default:
            ThrowUnsupported();
            return default;
    }

    static void ThrowUnsupported() => throw new NotSupportedException();
}

Sharplab against StringComparison:
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4a9JqwAqAC1jYAJry4BzDbUay92KGIAy2YO258BlmgG0AUrwwA4jBcMsoAFBgAnmIwEABmYbIYUKZmIuJ2vLgQXACUuWgAsjAYehBGAJLikmHFpeVVYpIA8mJ8ObgsAIJmZrC4uLwKMBVckqapuQC6muRIDNy42HEwDDoM6WJ2MACiCGC25jAAPDoAfGGwcWsMkhBg2O1caDcK2JIcMC/rkBlQ2FwjLkaAxQQwAO56GSrHQgBZcfAA7BmGBGGgAbxBYNw4P8BwYYUGAC9YgkdPksaDMdQwbSGA9cKtiaSwqYMLkQJS6bS3lAGAgGABeBijDAyO5gADWqOEoi2sD2BwBKMuMGuAFUuEsVt1cKcXmyLldbvdHrwcgUGJrtYIunqdAaeEa1Qw3h8YJbrctbfbHRhnddfvKAUDcpZudziAB2K1a72645stDnVXXBBhrm0oxq7AcSRYTMR/RQCDg60cMRiaBioxhDM0iO06MMbNxXP58NggC+NC5zHmxBQawMpfLlerqLrQrODFKJfBDBCC4AchAMLIK1WoDXFTA2uauHXLF2gA===

@MichalPetryka
Copy link
Contributor

Is it even possible to implement this API effectively without making it a special case in the compiler?

Right, the implementation of this API would have to be special-cased as runtime intrinsic. It is not something that can be done efficiently in pure C# today.

AFAIR at this moment all there's needed is available in pure C#.

does this approach have some fundamental flaw I'm unaware of?

Since CompareExchange does bitwise equality, this will break for non bitwise equatable types like floats for example.

@stephentoub
Copy link
Member

I think we should do this not by adding new APIs, but by removing the class constraint on the existing Interlocked.CompareExchange<T>. We then make it work for primitive Ts, including enums, and any reference type T, and throw a NotSupportedException when used with other Ts.

@stephentoub stephentoub added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 10, 2024
@stephentoub stephentoub modified the milestones: Future, 9.0.0 Jul 10, 2024
@stephentoub stephentoub changed the title Interlocked.CompareExchange on enums Remove Interlocked.{Compare}Exchange generic constraint Jul 10, 2024
@terrajobst
Copy link
Member

terrajobst commented Jul 16, 2024

Video

  • Looks good as proposed
namespace System.Threading;

public static class Interlocked
{
    public static T CompareExchange<T> (ref T location1, T value, T comparand);
    public static T Exchange<T>(ref T location1, T value);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 16, 2024
@stephentoub
Copy link
Member

Addressed by #104558

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

No branches or pull requests

10 participants