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

Maybe<T> have reference to Option<T> instance #38

Closed
Opiumtm opened this issue Jun 13, 2017 · 8 comments
Closed

Maybe<T> have reference to Option<T> instance #38

Opiumtm opened this issue Jun 13, 2017 · 8 comments

Comments

@Opiumtm
Copy link

Opiumtm commented Jun 13, 2017

https://github.com/DavidArno/SuccincT/blob/master/src/SuccincT/Options/Maybe%7BT%7D.cs

Although Maybe<T> appears as struct, it have internal reference to instance of reference type

internal Option<T> Option { get; }

/// <summary>
/// The value held (if created by Some()). Will throw an InvalidOperationException if created via None().
/// </summary>
public T Value => Option.Value;

Major purpose of structs isn't to avoid "Null reference exceptions". Major purpose of structs is to avoid memory allocations on heap which can be critical under heavy load.

Option<T> and Maybe<T> are small enough to be structs as heap allocations and then GC to release references are more performance-impacting than copy of such small structs around.

Maybe<T> retain unnecessary GC handle (internal reference-type instance of Option<T>), so it don't provide performance benefits of structs.

@DavidArno
Copy link
Owner

@Opiumtm,

Unfortunately, just removing the reference wouldn't achieve much as many features of Succinc<T> return an Option<T>, so there would still be a GC overhead of a temporary object from which the struct would be created.

To properly address this, those features would need to change to return Maybe<T>. So I'm going to schedule looking at this as part of the V3.2.0 release (the one after the next release).

@DavidArno DavidArno added this to the V3.2.0 milestone Jun 13, 2017
@megafinz
Copy link

I'd suggest to write performance benchmark tests first. For example, introduction of singleton value for Option.None() cases significantly improved performance of our code which was creating tens of thousands of those objects. I'm pretty sure turning Option into structs will increase the overhead of passing them around.

@DavidArno
Copy link
Owner

Good point, @megafinz. I shall do that.

@Opiumtm
Copy link
Author

Opiumtm commented Jun 13, 2017

@megafinz as a general rule, small enough structs are considered better than small classes for performance purposes.
It's why ValueTask and ValueTuple was introduced in C#7.

overhead of passing them around

Often that overhead can be mitigated via passing structs by ref.
C#7 provide extended support for by ref structs passing, allow ref struct locals and ref struct return.

@megafinz
Copy link

Often that overhead can be mitigated via passing structs by ref.
C#7 provide extended support for by ref structs passing, allow ref struct locals and ref struct return.

Good point, thanks, that simplifies things quite a bit.

@DavidArno
Copy link
Owner

@Opiumtm,

I'm uncomfortable with the idea of having to replace classes with structs and lots of ref returns as I feel that would complicate things, just to provide performance enhancements. But I shall experiment once v3.1 is released.

Of course, if either you or @megafinz wish to submit any performance tests for me to base my experiments on, then those PR's will be very gratefully received, as performance testing is not a strong point for me.

@Opiumtm
Copy link
Author

Opiumtm commented Jun 14, 2017

@DavidArno I propose to have two versions of it. After all, there are Tuple<> and ValueTuple<>, Task<T> and ValueTask<T> - you understood the idea. Developer would choose what is appropriate in his specific case.

and lots of ref returns

Generally small enough structs such as Maybe<T> do not require ref to pass data around as this struct is already small and cheap to copy. For the most cases Maybe<T> would contain reference type as T (so struct keep and copy only small reference to original T-value), but having additional reference with Option<T> inside (or with standalone use of Option<T>) require runtime to allocate and keep two GC handles instead of just one for original T-typed reference. It's 100% more GC handles and that 100% more GC handles are essentially excessive and truly the waste of resources.

@DavidArno DavidArno modified the milestones: V3.2.0, V4.0 Nov 30, 2017
@DavidArno
Copy link
Owner

Maybe<T> has been removed and Option<T> is now a read-only struct. Closing this as no longer required therefore.

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

No branches or pull requests

3 participants