Skip to content

Add List.chooseV, Seq.tryPickV, etc. for ValueOption - #6781

Closed
cmeeren wants to merge 12 commits into
dotnet:mainfrom
cmeeren:valueoption-pick-choose
Closed

Add List.chooseV, Seq.tryPickV, etc. for ValueOption#6781
cmeeren wants to merge 12 commits into
dotnet:mainfrom
cmeeren:valueoption-pick-choose

Conversation

@cmeeren

@cmeerencmeeren commented May 20, 2019

Copy link
Copy Markdown
Contributor

See fsharp/fslang-suggestions#739

I have simply copied the existing Option-based implementations and tried to adapt them trivially to ValueOption. Note that I was unable to build the solution (before I started working), so I have made these edits without an IDE.

I need help with adding ValueOption equivalents of

  • Microsoft.FSharp.Primitives.Basics.List.choose
  • Microsoft.FSharp.Primitives.Basics.List.unfold

because I don't know where to find them. Other than that, I don't know of anything else that needs to be done here (as long as it compiles).

Note that I have absolutely no need to have my name on these changes; I have only created this PR in the hope that it might speed up the completion of fsharp/fslang-suggestions#739. I'm completely fine with this PR being thrown away in favor of another implementation.

@cartermp

Copy link
Copy Markdown
Contributor

@cmeeren If possible, can you change this PR to a draft? That way it's clear that the PR is for experimentation and information about what this feature could mean.

Once you're comfortable with a trial implementation, it would be great to spin up a few BenchmarkDotNet experiments that attempt to simulate common tasks. A subfolder containing a solution could be placed here: https://github.com/dotnet/fsharp/tree/master/tests/fsharp/perf

This would help really suss out the impact from a CPU time and memory perspective of having these kinds of functions.

@cmeeren

cmeeren commented May 21, 2019

Copy link
Copy Markdown
ContributorAuthor

If possible, can you change this PR to a draft?

I don't know what this means. Edit: I just found out about GitHub's draft PR stuff, but it seems it's not possible to change from non-draft to draft.

Once you're comfortable with a trial implementation, it would be great to spin up a few BenchmarkDotNet experiments that attempt to simulate common tasks.

I have never used BenchmarkDotNet and am quite busy with real-life events the next months, so I can't promise anything (I probably won't be able to delve into it). For the record though, what specifically do you want the tests to measure? Should they compare the option and voption alternatives?

Also, do you know where I can find Microsoft.FSharp.Primitives.Basics.List.choose and Microsoft.FSharp.Primitives.Basics.List.unfold?

@cmeeren

Copy link
Copy Markdown
ContributorAuthor

@cartermp Are you able to provide some clarification on the questions in my last comment?

@cartermp

Copy link
Copy Markdown
Contributor

@cmeeren Sorry for not getting back to you yet.

Re: draft PR - this is fine. It's unfortunate that GitHub doesn't allow this.

As for benchmarking, it's quite easy. I have an example here that demonstrates how you can measure CPU and memory usage for distinct things: https://github.com/cartermp/trees

The idea is that you'd compare the built-in FSharp.Core methods today with your own variants written in a separate file/namepsace/module/etc. It would be good to have a few different benchmark classes:

  • These functions on small lists (<100)
  • These functions on medium lists (<1000)
  • These functions on large lists (>1000)
  • These functions with larger data types being held in the lists

That should give a pretty good view over the impact of using ValueOption for things.

@cmeeren

Copy link
Copy Markdown
ContributorAuthor

Thanks! I still need to know where I can find Microsoft.FSharp.Primitives.Basics.List.choose and Microsoft.FSharp.Primitives.Basics.List.unfold, though (currently it doesn't compile).

@cartermp

Copy link
Copy Markdown
Contributor

Choose is here: https://github.com/dotnet/fsharp/blob/master/src/fsharp/FSharp.Core/local.fs#L187

Unfold is here: https://github.com/dotnet/fsharp/blob/master/src/fsharp/FSharp.Core/local.fs#L781

Note that they ultimately rely on inline IL: https://github.com/dotnet/fsharp/blob/master/src/fsharp/FSharp.Core/local.fs#L96

So you'll have to do the benchmarks with a built FSharp.Core, otherwise your code will be at a disadvantage 🙂

@cmeeren

cmeeren commented Jun 13, 2019

Copy link
Copy Markdown
ContributorAuthor

Note that they ultimately rely on inline IL: https://github.com/dotnet/fsharp/blob/master/src/fsharp/FSharp.Core/local.fs#L96

So you'll have to do the benchmarks with a built FSharp.Core, otherwise your code will be at a disadvantage

@cartermp I'm working on perf tests, but having problems referencing the built FSharp.Core. I've added it in the project file:

<ItemGroup>
<ReferenceInclude="FSharp.Core">
<HintPath>..\..\..\..\..\artifacts\bin\FSharp.Core\Release\netstandard1.6\FSharp.Core.dll</HintPath>
</Reference>
</ItemGroup>

However, I don't have access to the new functions, even though they appear in the corresponding FSharp.Core.xml file.

image

Do you have any tips?

@cartermp

Copy link
Copy Markdown
Contributor

You can look at @dsyme's PR here: #6811

A project file example is here: https://github.com/dotnet/fsharp/pull/6811/files#diff-49b750428326bdfe4736d158ea9d2f78R17

@cmeeren

Copy link
Copy Markdown
ContributorAuthor

A project file example is here: https://github.com/dotnet/fsharp/pull/6811/files#diff-49b750428326bdfe4736d158ea9d2f78R17

Apologies for my confusion, but that's a project reference, and you said I have to

do the benchmarks with a built FSharp.Core

Perhaps I misunderstood that remark? I figured it meant that I had to reference a pre-compiled DLL. Though, come to think of it, what's the difference - the project is compiled anyway, of course. So what did you mean?

@cartermp

Copy link
Copy Markdown
Contributor

Sorry, what I meant was just an updated FSharp.Core. Project reference is likely the best way to do that.

@cmeeren

Copy link
Copy Markdown
ContributorAuthor

I've added performance tests now. Is it what you were after? Again, I've never used BenchmarkDotNet before, so please tell me if I should change anything.

@dsyme

dsyme commented Jul 1, 2019

Copy link
Copy Markdown
Contributor

I've added performance tests now. Is it what you were after? Again, I've never used BenchmarkDotNet before, so please tell me if I should change anything.

Can you paste the perf results here? Thanks

@cmeeren

Copy link
Copy Markdown
ContributorAuthor

Here are the benchmark results:

choose

(Some/ValueSome returned for all items)

BenchmarkDotNet=v0.11.5, OS=Windows 10.0.17763.557 (1809/October2018Update/Redstone5)
Intel Core i5-4690K CPU 3.50GHz (Haswell), 1 CPU, 4 logical and 4 physical cores
.NET Core SDK=3.0.100-preview5-011568
[Host] : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT DEBUG
DefaultJob : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT
| Method | N | Type | Mean | Error | StdDev | Ratio | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------ |------- |------- |----------------:|---------------:|---------------:|------:|---------:|---------:|------:|----------:|
| Option | 10 | int | 93.30 ns | 0.5223 ns | 0.4885 ns | 1.00 | 0.1861 | - | - | 584 B |
| ValueOption | 10 | int | 105.27 ns | 0.1590 ns | 0.1487 ns | 1.13 | 0.1097 | - | - | 344 B |
| | | | | | | | | | | |
| Option | 10 | record | 153.68 ns | 1.0971 ns | 1.0262 ns | 1.00 | 0.1860 | - | - | 584 B |
| ValueOption | 10 | record | 124.45 ns | 0.2014 ns | 0.1572 ns | 0.81 | 0.1097 | - | - | 344 B |
| | | | | | | | | | | |
| Option | 1000 | int | 9,111.92 ns | 11.8979 ns | 10.5471 ns | 1.00 | 17.8528 | - | - | 56024 B |
| ValueOption | 1000 | int | 10,306.70 ns | 34.8596 ns | 30.9022 ns | 1.13 | 10.2081 | - | - | 32024 B |
| | | | | | | | | | | |
| Option | 1000 | record | 14,170.86 ns | 138.4902 ns | 129.5438 ns | 1.00 | 17.8528 | - | - | 56024 B |
| ValueOption | 1000 | record | 11,136.60 ns | 71.5383 ns | 66.9169 ns | 0.79 | 10.2081 | - | - | 32024 B |
| | | | | | | | | | | |
| Option | 100000 | int | 5,457,377.63 ns | 61,807.4797 ns | 57,814.7549 ns | 1.00 | 914.0625 | 453.1250 | - | 5600024 B |
| ValueOption | 100000 | int | 2,318,102.29 ns | 14,083.2867 ns | 13,173.5151 ns | 0.42 | 527.3438 | 253.9063 | - | 3200024 B |
| | | | | | | | | | | |
| Option | 100000 | record | 7,846,356.25 ns | 82,262.9530 ns | 76,948.8173 ns | 1.00 | 914.0625 | 453.1250 | - | 5600024 B |
| ValueOption | 100000 | record | 4,339,809.82 ns | 11,625.5308 ns | 10,305.7302 ns | 0.55 | 523.4375 | 250.0000 | - | 3200024 B |

tryPick

(None/ValueNone returned for all items)

BenchmarkDotNet=v0.11.5, OS=Windows 10.0.17763.557 (1809/October2018Update/Redstone5)
Intel Core i5-4690K CPU 3.50GHz (Haswell), 1 CPU, 4 logical and 4 physical cores
.NET Core SDK=3.0.100-preview5-011568
[Host] : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT DEBUG
DefaultJob : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT
| Method | N | Type | Mean | Error | StdDev | Ratio | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------ |------- |------- |----------------:|---------------:|---------------:|------:|-------:|------:|------:|----------:|
| Option | 10 | int | 18.46 ns | 0.0374 ns | 0.0331 ns | 1.00 | 0.0076 | - | - | 24 B |
| ValueOption | 10 | int | 22.47 ns | 0.0839 ns | 0.0785 ns | 1.22 | 0.0076 | - | - | 24 B |
| | | | | | | | | | | |
| Option | 10 | record | 20.30 ns | 0.0362 ns | 0.0321 ns | 1.00 | 0.0076 | - | - | 24 B |
| ValueOption | 10 | record | 24.53 ns | 0.0886 ns | 0.0829 ns | 1.21 | 0.0076 | - | - | 24 B |
| | | | | | | | | | | |
| Option | 1000 | int | 1,329.67 ns | 3.7526 ns | 3.5102 ns | 1.00 | 0.0076 | - | - | 24 B |
| ValueOption | 1000 | int | 1,552.61 ns | 4.0393 ns | 3.7783 ns | 1.17 | 0.0076 | - | - | 24 B |
| | | | | | | | | | | |
| Option | 1000 | record | 2,885.07 ns | 6.6972 ns | 5.2287 ns | 1.00 | 0.0076 | - | - | 24 B |
| ValueOption | 1000 | record | 3,239.33 ns | 23.6545 ns | 22.1264 ns | 1.12 | 0.0076 | - | - | 24 B |
| | | | | | | | | | | |
| Option | 100000 | int | 133,114.27 ns | 175.1564 ns | 163.8414 ns | 1.00 | - | - | - | 24 B |
| ValueOption | 100000 | int | 156,501.69 ns | 788.5052 ns | 658.4376 ns | 1.18 | - | - | - | 24 B |
| | | | | | | | | | | |
| Option | 100000 | record | 2,198,305.01 ns | 22,732.6208 ns | 21,264.1076 ns | 1.00 | - | - | - | 24 B |
| ValueOption | 100000 | record | 2,206,098.84 ns | 5,867.8472 ns | 5,488.7879 ns | 1.00 | - | - | - | 24 B |

I'm confused by the tryPickV results.

@dsyme

dsyme commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

I'm confused by the tryPickV results.

Likewise! The chooseV case is pretty compelling however

  • Could you also extend the benchmarks to include arrays, where I'd imagine the performance gain would be highest?

  • Could you add unfoldV to the benchmarks please?

thanks!

@thinkbeforecoding

Copy link
Copy Markdown
Contributor

@dsyme any reason choose is faster than chooseV for an int ? Is there a specific optimization in the rest of the compiler ?

@dsyme

dsyme commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

@dsyme any reason choose is faster than chooseV for an int ? Is there a specific optimization in the rest of the compiler ?

No. It could be a number of things - alignments, registers 22.47 ns is very short, perhaps run the operation many more times in a loop rather than relying on the benchmarking to do that.

@cartermp

Copy link
Copy Markdown
Contributor

@dsyme I wouldn't expect much of a change there, as BenchmarkDotNet is running it enough times such that it's statistically sufficient. Putting in a loop won't be testing the invocation of the function, but a loop that invokes the function.

@cartermp

Copy link
Copy Markdown
Contributor

One source of variance is that failing to give your benchmarking process priority on your system can lead to something else getting a higher priority and affecting the benchmark. But since someone's machine isn't necessarily going to assign a similar priority to their own processes, it's arguably not bad to do this. They're close enough in execution time such that users aren't likely to notice either. It's the lack of allocations in the bigger ones where this comes into play.

@cmeeren

cmeeren commented Jul 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Here are all results so far:

results.zip

BenchmarkDotNet=v0.11.5, OS=Windows 10.0.17763.557 (1809/October2018Update/Redstone5)
Intel Core i5-4690K CPU 3.50GHz (Haswell), 1 CPU, 4 logical and 4 physical cores
.NET Core SDK=3.0.100-preview5-011568
[Host] : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT DEBUG
DefaultJob : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT

List.choose

MethodNTypeMeanErrorStdDevRatioGen 0Gen 1Gen 2Allocated
Option10int94.42ns0.1014ns0.0899ns1.000.1861--584B
ValueOption10int106.80ns0.6133ns0.5737ns1.130.1097--344B
Option10record156.01ns0.2104ns0.1757ns1.000.1860--584B
ValueOption10record131.28ns0.6983ns0.6190ns0.840.1097--344B
Option1000int9,182.43ns20.5495ns17.1597ns1.0017.8528--56024B
ValueOption1000int10,437.64ns62.0687ns58.0591ns1.1410.2081--32024B
Option1000record14,540.01ns142.0912ns132.9122ns1.0017.8528--56024B
ValueOption1000record11,293.24ns38.7614ns32.3675ns0.7810.2081--32024B
Option100000int5,471,929.48ns27,408.3314ns25,637.7702ns1.00914.0625453.1250-5600024B
ValueOption100000int2,313,274.69ns4,397.8885ns3,898.6136ns0.42527.3438257.8125-3200024B
Option100000record7,771,535.64ns57,188.7703ns47,755.2133ns1.00914.0625453.1250-5600024B
ValueOption100000record4,310,785.89ns12,708.3983ns11,887.4437ns0.55523.4375250.0000-3200024B

List.tryPick

MethodNTypeMeanErrorStdDevRatioGen 0Gen 1Gen 2Allocated
Option10int19.09ns0.0332ns0.0311ns1.000.0076--24B
ValueOption10int20.50ns0.0454ns0.0403ns1.070.0076--24B
Option10record20.35ns0.0960ns0.0898ns1.000.0076--24B
ValueOption10record24.65ns0.0395ns0.0330ns1.210.0076--24B
Option1000int1,325.98ns0.2503ns0.2342ns1.000.0076--24B
ValueOption1000int1,331.45ns0.1962ns0.1532ns1.000.0076--24B
Option1000record2,764.38ns1.8204ns1.6138ns1.000.0076--24B
ValueOption1000record2,835.19ns9.8126ns7.6610ns1.030.0076--24B
Option100000int132,580.58ns144.3995ns128.0064ns1.00---24B
ValueOption100000int132,973.41ns778.8675ns690.4458ns1.00---24B
Option100000record2,216,955.89ns14,799.4768ns13,119.3507ns1.00---24B
ValueOption100000record2,248,968.39ns16,673.2208ns15,596.1411ns1.01---24B

List.unfoldV

MethodNTypeMeanErrorStdDevRatioGen 0Gen 1Gen 2Allocated
Option10int168.1ns0.3696ns0.3457ns1.000.2880--904B
ValueOption10int130.2ns0.8314ns0.7777ns0.770.2038--640B
Option10record960.9ns0.6222ns0.5196ns1.000.8965--2816B
ValueOption10record925.5ns3.6742ns3.4369ns0.960.8135--2552B
Option1000int14,569.5ns198.3009ns185.4908ns1.0025.5280--80104B
ValueOption1000int10,799.1ns14.3689ns13.4407ns0.7417.8680--56080B
Option1000record123,831.8ns969.5654ns809.6310ns1.0094.23838.4229-311696B
ValueOption1000record119,949.1ns283.9864ns265.6411ns0.9782.275410.1318-287672B
Option100000int8,290,024.1ns28,192.7096ns26,371.4781ns1.001328.1250640.6250140.62508000104B
ValueOption100000int5,288,396.8ns32,413.0518ns30,319.1887ns0.64906.2500453.1250-5600080B
Option100000record83,182,436.7ns314,770.5518ns294,436.5694ns1.005500.00002166.6667833.333331199721B
ValueOption100000record69,257,819.2ns312,446.7299ns292,262.8650ns0.835000.00001875.0000750.000028799694B

Array.choose

MethodNTypeMeanErrorStdDevRatioGen 0Gen 1Gen 2Allocated
Option10int91.53ns0.1451ns0.1133ns1.000.1351--424B
ValueOption10int108.10ns0.5147ns0.4815ns1.180.0587--184B
Option10record133.37ns0.2445ns0.2167ns1.000.1581--496B
ValueOption10record98.35ns0.0876ns0.0777ns0.740.0815--256B
Option1000int4,840.96ns22.3488ns18.6623ns1.0010.2310--32104B
ValueOption1000int7,405.87ns35.0736ns32.8078ns1.532.5787--8104B
Option1000record8,812.95ns54.8451ns51.3021ns1.0012.7563--40096B
ValueOption1000record5,977.79ns38.8219ns36.3141ns0.685.1270--16096B
Option100000int1,043,248.68ns631.5124ns527.3415ns1.00812.500048.828148.82813200104B
ValueOption100000int796,531.26ns3,272.3305ns3,060.9400ns0.7633.203133.203133.2031800362B
Option100000record3,669,541.90ns9,405.0687ns8,797.5071ns1.00855.468893.750093.75004000096B
ValueOption100000record1,807,609.38ns4,121.0920ns3,217.4776ns0.4962.500062.500062.50001600585B

Array.tryPick

MethodNTypeMeanErrorStdDevRatioRatioSDGen 0Gen 1Gen 2Allocated
Option10int20.04ns0.0265ns0.0207ns1.000.000.0076--24B
ValueOption10int24.63ns0.1168ns0.1035ns1.230.010.0076--24B
Option10record22.13ns0.0491ns0.0410ns1.000.000.0076--24B
ValueOption10record26.02ns0.1238ns0.1158ns1.180.010.0076--24B
Option1000int1,341.31ns0.8370ns0.6535ns1.000.000.0076--24B
ValueOption1000int1,604.37ns21.5298ns20.1390ns1.200.020.0076--24B
Option1000record1,345.73ns3.8779ns3.6274ns1.000.000.0076--24B
ValueOption1000record1,564.84ns9.8862ns8.2555ns1.160.010.0076--24B
Option100000int130,971.21ns16.6883ns14.7937ns1.000.00---24B
ValueOption100000int154,674.60ns857.4426ns802.0523ns1.180.01---24B
Option100000record130,992.27ns26.0676ns23.1083ns1.000.00---24B
ValueOption100000record154,725.87ns603.2300ns503.7243ns1.180.00---24B

Array.unfoldV

MethodNTypeMeanErrorStdDevMedianRatioGen 0Gen 1Gen 2Allocated
Option10int193.6ns1.2658ns1.1840ns192.7ns1.000.2677--840B
ValueOption10int159.4ns0.2155ns0.1682ns159.4ns0.820.1836--576B
Option10record1,035.7ns2.1101ns1.6474ns1,036.4ns1.000.9251--2904B
ValueOption10record995.8ns1.2365ns0.9654ns995.7ns0.960.8411--2640B
Option1000int11,266.9ns14.1358ns12.5310ns11,261.3ns1.0019.2871--60528B
ValueOption1000int7,951.1ns37.8169ns35.3740ns7,966.2ns0.7111.6272--36504B
Option1000record111,051.4ns180.9723ns151.1201ns110,995.8ns1.0091.30867.5684-304296B
ValueOption1000record121,403.0ns546.0792ns510.8028ns121,615.5ns1.0986.30373.4180-280272B
Option100000int1,447,572.3ns3,687.5333ns3,449.3209ns1,446,928.7ns1.001599.6094597.6563398.43756249080B
ValueOption100000int1,080,474.8ns3,697.1373ns3,277.4159ns1,081,297.5ns0.75849.6094447.2656398.43753849056B
Option100000record51,535,995.0ns172,388.6166ns152,818.0185ns51,511,740.0ns1.005000.00002100.0000900.000030897251B
ValueOption100000record64,791,745.8ns62,347.7679ns48,677.0361ns64,806,100.0ns1.264625.00002000.0000750.000028497230B

@realvictorprm

Copy link
Copy Markdown
Contributor

Why are there still allocations for the ValueSome implementation 🤔 ?

@KevinRansom

KevinRansom commented May 16, 2020

Copy link
Copy Markdown
Contributor

These changes look good to me, since they are new APIs to fsharp.core we need to add the
ExperimentalAttribute.

SurfaceArea tests fail.

@dsyme , @cartermp, are we good with adding these Apis.

@cartermp
cartermp marked this pull request as draft May 16, 2020 19:28
@cartermp

Copy link
Copy Markdown
Contributor

I've converted this into a draft (feature is available now!)

I think I'm fine with the APIs getting added but @dsyme should have a say and this would need to also undergo an RFC

@KevinRansom

Copy link
Copy Markdown
Contributor

@cartermp, we should try to move this forward, so we can get some preview time in for it before F# 5.0

@KevinRansom

Copy link
Copy Markdown
Contributor

Tests Fixed, and Experimental added.

@KevinRansom

Copy link
Copy Markdown
Contributor

/cc @dsyme , @cartermp

Please review this, I think it is ready to merge.

@KevinRansom
KevinRansom requested review from cartermp and dsymeMay 28, 2020 09:37

@KevinRansomKevinRansom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look good to me.

@KevinRansom
KevinRansom marked this pull request as ready for review May 28, 2020 09:38

@cartermpcartermp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting in a block since fsharp/fslang-suggestions#739 is not an approved change - @dsyme will need to have a say here

@abelbraaksma

abelbraaksma commented Jul 23, 2020

Copy link
Copy Markdown
Contributor

I'm confused by the tryPickV results.

I had a brief look at why tryPickV is measurably slower here. Since the test basically always returns None or ValueNone for the chooser function, this comes down to the way a match over option vs voption gets compiled.

  • voption, like other struct DUs always calls get_Tag() and compares its result to 0.
  • option, as is well known, compiles into a fast null-test.

As a consequence, even after JIT inlining and optimization, the voption being tested against ValueNone is considerably slower. This could potentially be fixed by optimizing this into a brfalse IL test, since the whole underlying struct is only ever zero when it's also ValueNone. I've created #9767 for that.

I tested some other ways of coding tryPickV but none had a significant advantage. All of them did too many copies and re-assignments (like: the chooser gets re-assigned to a new var inside the resulting while loop), but I didn't check if they were properly folded by the JIT.

Note that the normal overhead of option being a ref type and voption not is irrelevant, as when chooser returns Some or ValueSome, the function returns. Hence only the difference in how [Value]None is treated is relevant, which is slower for voption.

@brettfo
brettfo changed the base branch from master to mainAugust 19, 2020 20:04
@KevinRansom

Copy link
Copy Markdown
Contributor

@cmeeren
I'm afraid we are trying to prune the inactive but open PR's once again. Please reopen this PR when you have time to pursue and RFC and continue the development.

Thanks

Kevin

@cmeeren

Copy link
Copy Markdown
ContributorAuthor

I don't see what more I can do; this seems to be approved by everyone except that @dsyme needs to have a look at it (or at least approve fsharp/fslang-suggestions#739).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@cmeeren@cartermp@dsyme@thinkbeforecoding@realvictorprm@KevinRansom@abelbraaksma