-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Allow for Config per method, introduce OS and OSArchitecture filters #1097
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
Merged
Merged
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
3780ced
remove the extra overload that most probably nobody uses but increase…
adamsitnik 1732cdd
the test code should call a public method without using internal surf…
adamsitnik 6bb0161
allow for config per method
adamsitnik 75d7a9c
allow the FilterConfigBaseAttribute to be applied per method
adamsitnik 59fa565
introduce OperatingSystemsFilterAttribute
adamsitnik dcd8c52
introduce OperatingSystemsArchitectureFilterAttribute
adamsitnik 2422eb7
typo
adamsitnik eec3029
Merge branch 'master' into configPerMethod
adamsitnik 103ed21
fix warnings
adamsitnik 0edd9af
fix compilation errors
adamsitnik de97fca
introduce our own OS enum, add Browser (WebAssembly) as an option
adamsitnik 5140e34
add test
adamsitnik e233279
allow setting mutator job attributes per method
adamsitnik a5c857a
update randomization sample
adamsitnik 502fb17
Merge branch 'master' into configPerMethod
adamsitnik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
25 changes: 25 additions & 0 deletions
25
src/BenchmarkDotNet/Attributes/Filters/OperatingSystemsArchitectureFilterAttribute.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using System.Linq; | ||
using BenchmarkDotNet.Filters; | ||
using JetBrains.Annotations; | ||
using System.Runtime.InteropServices; | ||
|
||
namespace BenchmarkDotNet.Attributes | ||
{ | ||
[PublicAPI] | ||
public class OperatingSystemsArchitectureFilterAttribute : FilterConfigBaseAttribute | ||
{ | ||
// CLS-Compliant Code requires a constructor without an array in the argument list | ||
public OperatingSystemsArchitectureFilterAttribute() { } | ||
|
||
/// <param name="allowed">if set to true, the architectures are enabled, if set to false, disabled</param> | ||
public OperatingSystemsArchitectureFilterAttribute(bool allowed, params Architecture[] architectures) | ||
: base(new SimpleFilter(_ => | ||
{ | ||
return allowed | ||
? architectures.Any(architecture => RuntimeInformation.OSArchitecture == architecture) | ||
: architectures.All(architecture => RuntimeInformation.OSArchitecture != architecture); | ||
})) | ||
{ | ||
} | ||
} | ||
} |
45 changes: 45 additions & 0 deletions
45
src/BenchmarkDotNet/Attributes/Filters/OperatingSystemsFilterAttribute.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
using System; | ||
using System.Linq; | ||
using BenchmarkDotNet.Filters; | ||
using JetBrains.Annotations; | ||
using System.Runtime.InteropServices; | ||
|
||
namespace BenchmarkDotNet.Attributes | ||
{ | ||
[PublicAPI] | ||
public class OperatingSystemsFilterAttribute : FilterConfigBaseAttribute | ||
{ | ||
// CLS-Compliant Code requires a constructor without an array in the argument list | ||
public OperatingSystemsFilterAttribute() { } | ||
|
||
/// <param name="allowed">if set to true, the OSes beloning to platforms are enabled, if set to false, disabled</param> | ||
public OperatingSystemsFilterAttribute(bool allowed, params PlatformID[] platforms) | ||
: base(new SimpleFilter(_ => | ||
{ | ||
return allowed | ||
? platforms.Any(platform => RuntimeInformation.IsOSPlatform(Map(platform))) | ||
: platforms.All(platform => !RuntimeInformation.IsOSPlatform(Map(platform))); | ||
})) | ||
{ | ||
} | ||
|
||
// OSPlatform is a struct so it can not be used as attribute argument and this is why we use PlatformID enum | ||
private static OSPlatform Map(PlatformID platform) | ||
{ | ||
switch (platform) | ||
{ | ||
case PlatformID.MacOSX: | ||
return OSPlatform.OSX; | ||
case PlatformID.Unix: | ||
return OSPlatform.Linux; | ||
case PlatformID.Win32NT: | ||
case PlatformID.Win32S: | ||
case PlatformID.Win32Windows: | ||
case PlatformID.WinCE: | ||
return OSPlatform.Windows; | ||
default: | ||
throw new NotSupportedException($"Platform {platform} is not supported"); | ||
} | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
107 changes: 107 additions & 0 deletions
107
tests/BenchmarkDotNet.Tests/Configs/ConfigPerMethodTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
using System; | ||
using System.Runtime.InteropServices; | ||
using BenchmarkDotNet.Attributes; | ||
using BenchmarkDotNet.Filters; | ||
using BenchmarkDotNet.Running; | ||
using Xunit; | ||
|
||
namespace BenchmarkDotNet.Tests.Configs | ||
{ | ||
public class ConfigPerMethodTests | ||
{ | ||
[Fact] | ||
public void PerMethodConfigsAreRespected() | ||
{ | ||
var never = BenchmarkConverter.TypeToBenchmarks(typeof(WithBenchmarkThatShouldNeverRun)); | ||
|
||
Assert.Empty(never.BenchmarksCases); | ||
|
||
var always = BenchmarkConverter.TypeToBenchmarks(typeof(WithBenchmarkThatShouldAlwaysRun)); | ||
|
||
Assert.NotEmpty(always.BenchmarksCases); | ||
} | ||
|
||
public class ConditionalRun : FilterConfigBaseAttribute | ||
{ | ||
public ConditionalRun(bool value) : base(new SimpleFilter(_ => value)) { } | ||
} | ||
|
||
public class WithBenchmarkThatShouldNeverRun | ||
{ | ||
[Benchmark] | ||
[ConditionalRun(false)] | ||
public void Method() { } | ||
} | ||
|
||
public class WithBenchmarkThatShouldAlwaysRun | ||
{ | ||
[Benchmark] | ||
[ConditionalRun(true)] | ||
public void Method() { } | ||
} | ||
|
||
[Fact] | ||
public void CanEnableOrDisableTheBenchmarkPerOperatingSystem() | ||
{ | ||
var allowedForWindows = BenchmarkConverter.TypeToBenchmarks(typeof(WithBenchmarkAllowedForWindows)); | ||
var notAllowedForWindows = BenchmarkConverter.TypeToBenchmarks(typeof(WithBenchmarkNotAllowedForWindows)); | ||
|
||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
Assert.NotEmpty(allowedForWindows.BenchmarksCases); | ||
Assert.Empty(notAllowedForWindows.BenchmarksCases); | ||
} | ||
else | ||
{ | ||
Assert.Empty(allowedForWindows.BenchmarksCases); | ||
Assert.NotEmpty(notAllowedForWindows.BenchmarksCases); | ||
} | ||
} | ||
|
||
public class WithBenchmarkAllowedForWindows | ||
{ | ||
[Benchmark] | ||
[OperatingSystemsFilter(allowed: true, PlatformID.Win32NT)] | ||
public void Method() { } | ||
} | ||
|
||
public class WithBenchmarkNotAllowedForWindows | ||
{ | ||
[Benchmark] | ||
[OperatingSystemsFilter(allowed: false, PlatformID.Win32NT)] | ||
public void Method() { } | ||
} | ||
|
||
[Fact] | ||
public void CanEnableOrDisableTheBenchmarkPerOperatingSystemArchitecture() | ||
{ | ||
var allowed = BenchmarkConverter.TypeToBenchmarks(typeof(WithBenchmarkAllowedForX64)); | ||
var notallowed = BenchmarkConverter.TypeToBenchmarks(typeof(WithBenchmarkNotAllowedForX64)); | ||
|
||
if (RuntimeInformation.OSArchitecture == Architecture.X64) | ||
{ | ||
Assert.NotEmpty(allowed.BenchmarksCases); | ||
Assert.Empty(notallowed.BenchmarksCases); | ||
} | ||
else | ||
{ | ||
Assert.Empty(allowed.BenchmarksCases); | ||
Assert.NotEmpty(notallowed.BenchmarksCases); | ||
} | ||
} | ||
|
||
public class WithBenchmarkAllowedForX64 | ||
{ | ||
[Benchmark] | ||
[OperatingSystemsArchitectureFilter(allowed: true, Architecture.X64)] | ||
public void Method() { } | ||
} | ||
|
||
public class WithBenchmarkNotAllowedForX64 | ||
{ | ||
[Benchmark] | ||
[OperatingSystemsArchitectureFilter(allowed: false, Architecture.X64)] | ||
public void Method() { } | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could take the
string
of a OSPlatform instead ofPlatformID
? (basically the string that gets passed into https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.osplatform.create)I don't think PlatformID is going to work.
From https://docs.microsoft.com/en-us/dotnet/api/system.platformid
From that, it sounds like you won't be able to distinguish between macOS and Linux on .NET Core using PlatformID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eerhardt I am not sure about string. For example: let's say that I dont't want to run the benchmark on Ubuntu. Should I put
Unix
?Linux
? orUbuntu
? orUbuntu16.04
?But I agree that PlatformID is not perfect, I had to implement a mapping on my own.
Maybe I should just introduce a new enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want that deep of control? Do you want to be able to say "All Unixes" vs. "just Ubuntu, but not RedHat or OpenSUSE"? If so, it seems like a perfect place to use RIDs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I just wanted to show that without an enum, the user needs to guess the right string value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been two years since I created this PR, but now I really need tha for Randomization in the perf repo so I've updated it. I decided to include my own simple enum, just so users don't have to type OS names manually (an avoid mistakes for "OSX" != "macOS" etc)