Skip to content

Switching quantity to class from struct #500

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

Closed
wants to merge 1 commit into from

Conversation

tmilnthorp
Copy link
Collaborator

No description provided.

@angularsen
Copy link
Owner

I'm not too sure about including this one for the 4.0 release?
I still have too many questions about class vs struct I'm not confident about the answer to yet.
Some rambling thoughts trying to clear up the idea for myself here, please help me fill the gaps.

Reference vs value semantics

Can we cause a lot of trouble changing this, such as producing null reference exceptions where they prior relied on default value (zero and base unit)?
There is different behavior for myObj.MyValue.x = "new value"; if MyValue is a value type vs reference type, but our quantities are immutable so I believe we avoid that problem.

Is the performance/memory impact significant?

Does it add significant CPU overhead when constructing instances?
Does class overhead take up significantly more memory?
For what scenarios does it significantly increase garbage collection pressure beyond Gen1?
Can we simply take the stance that this lib is not intended for memory/cpu intensive applications?

What does class gain us?

Please refresh my memory, what was the motivation for considering class again?
I can think of these:

  • Generics: Allows us to support any numeric type and arithmetic operations on it internally, whereas QuantityValue currently only saves us from N method overloads for N number types when constructing quantities
  • Inheritance
    • Allows us to share code, but we can also share with delegation/composition.
    • Can serve as a shared type, but we can use interfaces to get the same benefit with structs

@tmilnthorp
Copy link
Collaborator Author

I think regarding reference vs value semantics, it's best not to worry too much. You could indeed have null with using classes, but you could before using the nullable types that we removed. In one way, changing to class would allow null to continue to exist for "uninitialized" values while still keeping the library size small.

Microsoft has said that the stack vs heap is an implementation detail, and I doubt there's much CPU or memory difference between the two. We could profile to know for sure.

In addition to allowing null listed above, it would allow us to use generics as well as inheritance, correct. The interface + struct route does indeed work, but that definitely has performance implications as using an interface on the left-hand-side introduces boxing considerations.

@angularsen
Copy link
Owner

I think class introduces null issues more subtly than nullable value types, where you get warnings if you don't check for null and you typically explicitly created the nullable quantity to begin with. As an analog, I would be very much caught off guard if .NET suddenly decided to change DateTime to class, but I guess it is a fair point that I would probably not have been relying on uninitialized values of DateTime so any null issues would be due to my own (or others') code returning null instead of a DateTime instance.

Boxing overhead would be limited to when you work generically with quantities and it would not be any worse than working with class where that same overhead is the default.

I want to pursue this idea further, but I think it's maybe a mistake to shoehorn it into 4.0 since at least for me I'm still too undecided on whether I think this is a better choice or not. Let's focus on wrapping up this release with reasonably small breaking changes and get the binary size down this time around. After that, we can take our time and focus on the bigger picture of how we want to restructure our types and prototype it up before we make a more informed decision on what would be the best?

@tmilnthorp
Copy link
Collaborator Author

Ok, waiting for another time works for me!

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

Successfully merging this pull request may close these issues.

2 participants