@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.

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?

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

@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.

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).

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 🙂

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>
  <Reference Include="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.

Do you have any tips?

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

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?

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

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.

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

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.

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!

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

@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.

@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.

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.

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

List.tryPick

List.unfoldV

Array.choose

Array.tryPick

Array.unfoldV

Why are there still allocations for the ValueSome implementation 🤔 ?

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.

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

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

Tests Fixed, and Experimental added.

/cc @dsyme , @cartermp

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

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.

@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

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).