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

add OSPlatform.macOS, switch to OrdinalIgnoreCase for OSPlatform comparisons #39209

Merged
merged 10 commits into from
Jul 14, 2020

Conversation

adamsitnik
Copy link
Member

I tried to introduce as small overhead as possible.

On Windows, there is basically no difference. On OSX there is no difference for "OSX" (old check), but the "macOS" check takes twice the time because we need two comparisons.

I've added the test cases that were missing in #39005 (ignore case comparison was not enabled then)

Windows

private readonly OSPlatform _notCurrent = OSPlatform.Create("NOPE");
private readonly OSPlatform _current = OSPlatform.Windows;

[Benchmark]
public bool IsOSPlatform_True() => RuntimeInformation.IsOSPlatform(_current);

[Benchmark]
public bool IsOSPlatform_False() => RuntimeInformation.IsOSPlatform(_notCurrent);
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.900 (1909/November2018Update/19H2)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores

Before:

Method Mean Error StdDev Median Min Max Gen 0 Gen 1 Gen 2 Allocated
IsOSPlatform_True 3.160 ns 0.0116 ns 0.0103 ns 3.158 ns 3.145 ns 3.183 ns - - - -
IsOSPlatform_False 4.592 ns 0.0139 ns 0.0130 ns 4.590 ns 4.571 ns 4.613 ns - - - -

After:

Method Mean Error StdDev Median Min Max Gen 0 Gen 1 Gen 2 Allocated
IsOSPlatform_True 3.177 ns 0.0210 ns 0.0197 ns 3.171 ns 3.152 ns 3.213 ns - - - -
IsOSPlatform_False 4.739 ns 0.0126 ns 0.0112 ns 4.741 ns 4.722 ns 4.761 ns - - - -

OSX

BenchmarkDotNet=v0.12.1, OS=macOS Mojave 10.14.5 (18F132) [Darwin 18.6.0]
Intel Core i7-5557U CPU 3.10GHz (Broadwell), 1 CPU, 4 logical and 2 physical cores
private readonly OSPlatform _notCurrent = OSPlatform.Create("NOPE");
private readonly OSPlatform _current = OSPlatform.OSX;
private readonly OSPlatform _macOS = OSPlatform.macOS;

[Benchmark]
public bool IsOSPlatform_True() => RuntimeInformation.IsOSPlatform(_current);

[Benchmark]
public bool IsOSPlatform_macOS() => RuntimeInformation.IsOSPlatform(_macOS );

[Benchmark]
public bool IsOSPlatform_False() => RuntimeInformation.IsOSPlatform(_notCurrent);

Before:

Method Mean Error StdDev Median Min Max Gen 0 Gen 1 Gen 2 Allocated
IsOSPlatform_True 2.901 ns 0.0071 ns 0.0063 ns 2.901 ns 2.892 ns 2.915 ns - - - -
IsOSPlatform_macOS 4.091 ns 0.0092 ns 0.0077 ns 4.089 ns 4.081 ns 4.106 ns - - - -
IsOSPlatform_False 4.415 ns 0.0209 ns 0.0163 ns 4.411 ns 4.402 ns 4.463 ns - - - -

After:

Method Mean Error StdDev Median Min Max Gen 0 Gen 1 Gen 2 Allocated
IsOSPlatform_True 2.887 ns 0.0057 ns 0.0048 ns 2.886 ns 2.880 ns 2.897 ns - - - -
IsOSPlatform_macOS 7.972 ns 0.0146 ns 0.0130 ns 7.968 ns 7.956 ns 7.996 ns - - - -
IsOSPlatform_False 9.689 ns 0.0378 ns 0.0335 ns 9.682 ns 9.627 ns 9.751 ns - - - -

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm going to add @jkotas as a reviewer too though; we should wait for his approval before merging.

@jeffhandley jeffhandley requested a review from jkotas July 13, 2020 18:27
adamsitnik and others added 2 commits July 14, 2020 10:02
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@adamsitnik adamsitnik merged commit 2c573b5 into dotnet:master Jul 14, 2020
@adamsitnik adamsitnik deleted the 33331_macOS_ignoreCase branch July 14, 2020 17:06
@jeffhandley
Copy link
Member

Thanks, @adamsitnik and @jkotas!

@safern safern mentioned this pull request Jul 16, 2020
7 tasks
safern added a commit that referenced this pull request Jul 16, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants