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

Why not store the instance in an object field instead of a bunch of fields? #158

Open
NickStrupat opened this issue Aug 17, 2023 · 7 comments

Comments

@NickStrupat
Copy link

NickStrupat commented Aug 17, 2023

Like this:

readonly struct OneOf<T0, T1>
{
    private readonly Object? value;
    private readonly Int32 index;
    public OneOf(T0 value) => (this.value, index) = (value, 1); // Start at 1 since all structs can be default constructed
    public OneOf(T1 value) => (this.value, index) = (value, 2);

    public T Match<T>(Func<T0, T> f1, Func<T, T> f2) => index switch
    {
        0 => throw new InvalidOperationException("This instance was not constructed with a value."),
        1 => f1((T0)value!),
        2 => f2((T)value!),
        _ => throw new InvalidOperationException()
    };

    public void Switch(Action<T0> a1, Action<T1> a2)
    {
        switch (index)
        {
            case 0: throw new InvalidOperationException("This instance was not constructed with a value.");
            case 1: a1((T0)value!); break;
            case 2: a2((T1)value!); break;
            default: throw new InvalidOperationException();
        }
    }
}
@bartelink
Copy link

Because you'd be boxing what are value types etc, and generally wasting space, causing extra allocations etc
That's a problem if you use DUs in the mainline paths of an app
Have a play with the Object Layout Inspector ref'd in https://ayende.com/blog/199777-A/struct-memory-layout-and-memory-optimizations?key=bb987d033f60428698a31b9a4a1b1e58

@NickStrupat
Copy link
Author

NickStrupat commented Aug 18, 2023

It's my understanding that any Ts that are value types will be copied into their respective fields in the OneOf<> instance. Boxing them will allocate sizeof(T) + 16 bytes on the GC'd heap, but that's quite small IMO.

The big win would be when there are multiple value types. Consider a OneOf<T, U, V...>, where each type is a good-sized value type. With the current implementation the sizeof(OneOf<T, U, V...>) is sizeof(T) + sizeof(U) + sizeof(V)..., no matter what it's initialized to. That's potentially really big.

Another thought: there could be an size optimization for value type Ts which are also unmanaged and, let's say, a maximum of 8 bytes or something. Their (unmanaged) byte representation could be stored in a ulong beside the object, which would be for for ref types and (boxed) larger val types.

readonly struct OneOf<T0, T1>
{
    private readonly Object? value;
    private readonly UInt64 smallUnmanagedValueTypeRawBytes;
    private readonly Int32 index;
    ...

@bartelink
Copy link

Apologies for coming over all conclusive on this - I was merely trying to give you an answer as to why I would expect the proposed mechanism to not be a win. Please note I'm not the maintainer and/or a contributor, and my only intent is to move the ball forward in terms of moving from "why didnt you do this instead" onto either "ah, I see now" or "actually, look - it works this much better , see the benchmark.net test"


I can't say I've particularly thought about in depth.
However, the bottom line is that the specifics of the representation were very likely done intentionally
Replacing the impl with something with different perf and/or allocation implications bears a burden of proof
The linked article links to the ObjectLayoutInspector that will enable you to provide such proof
That removes all the "it's my understanding" aspect.
If you then concluded that there's a sufficient benefit, then the next part would be for others to check other scenarios that may not have been considered.

@NickStrupat
Copy link
Author

No problem. Thank you for your responses and help so far :)

@mcintyre321
Copy link
Owner

mcintyre321 commented Aug 18, 2023 via email

@lookbusy1344
Copy link

lookbusy1344 commented Sep 19, 2023

if you’re happy with boxing and casting, you could just use an object and a switch expression, in vanilla C#?

object obj = 1; 
…
string s = obj switch
{
  int i => $"The object is an integer: {i}",
  string s => $"The object is a string: {s}",
  double d => $"The object is a double: {d}",
  _ => throw new Exception(“invalid type”)
};

@cremor
Copy link

cremor commented Oct 7, 2023

Here is a new proposal for the C# language to implement unions with an object field: dotnet/csharplang#7544
It also mentions some upsides and downsides vs explicitly typed fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants