Skip to content

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Aug 7, 2024

Description

  • We are already checking Nullable<'T>.HasValue before accessing Nullable<'T>.Value, so it is safe to call Nullable<'T>.GetValueOrDefault. Nullable<'T>.Value does a redundant check of HasValue, while Nullable<'T>.GetValueOrDefault does not. Using GetValueOrDefault is significantly faster when no value is present.

Note: these functions are marked inline — does this change have any binary compatibility implications?

Benchmarks

Method Mean Error StdDev Ratio RatioSD
Value_NoValue 0.2210 ns 0.0140 ns 0.0117 ns 1.00 0.07
GetValueOrDefault_NoValue 0.0368 ns 0.0106 ns 0.0089 ns 0.17 0.04
Value_Value 3.1128 ns 0.0731 ns 0.0610 ns 1.00 0.03
GetValueOrDefault_Value 3.0675 ns 0.0107 ns 0.0084 ns 0.99 0.02
Source
module OfNullable.Program

open System
open BenchmarkDotNet.Attributes
open BenchmarkDotNet.Configs
open BenchmarkDotNet.Running

[<GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)>]
type Benchmarks () =
    let noValue = Nullable<int> ()
    let value = Nullable 3

    [<Benchmark(Baseline = true)>]
    [<BenchmarkCategory("NoValue")>]
    member _.Value_NoValue () = ValueOption.ofNullable noValue

    [<Benchmark>]
    [<BenchmarkCategory("NoValue")>]
    member _.GetValueOrDefault_NoValue () = if noValue.HasValue then ValueSome (noValue.GetValueOrDefault ()) else ValueNone

    [<Benchmark(Baseline = true)>]
    [<BenchmarkCategory("Value")>]
    member _.Value_Value () = ValueOption.ofNullable value

    [<Benchmark>]
    [<BenchmarkCategory("Value")>]
    member _.GetValueOrDefault_Value () = if value.HasValue then ValueSome (value.GetValueOrDefault ()) else ValueNone

ignore (BenchmarkRunner.Run<Benchmarks> ())

Checklist

  • Test cases added.
  • Performance benchmarks added in case of performance changes.
  • Release notes entry updated.

* We are already checking `Nullable.HasValue` before accessing
  `Nullable.Value`, so it is safe to call `Nullable.GetValueOrDefault`,
  which does not do an additional check, unlike `Nullable.Value`.
Copy link
Contributor

github-actions bot commented Aug 7, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@vzarytovskii vzarytovskii added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Aug 7, 2024
@vzarytovskii
Copy link
Member

That's nice, thanks. I didn't even know about this API existence.

@brianrourkeboll brianrourkeboll marked this pull request as ready for review August 7, 2024 17:49
@brianrourkeboll brianrourkeboll requested a review from a team as a code owner August 7, 2024 17:49
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Thanks!

@KevinRansom KevinRansom enabled auto-merge (squash) August 9, 2024 16:53
auto-merge was automatically disabled August 12, 2024 08:12

Pull request was closed

@vzarytovskii vzarytovskii reopened this Aug 12, 2024
@T-Gro T-Gro enabled auto-merge (squash) August 12, 2024 10:29
@psfinaki
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants