Check BenchmarkRunner arguments #1911
Open
+120
−10
Conversation
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
|
While you're at it, there is also this:
I made a note of it a while ago with the intent to fix it myself, but haven't gotten around to it yet. Thought I mention it, since you're working in that area... No pressure, though 😉 |
|
@mawosoft Sorry for stealing the issue :)
Сheck was added.
If I understand you correctly, does this apply to this code? BenchmarkRunner.Run(typeof(BenchmarkClass), new MethodInfo[] {
BenchmarkClass.CorrectMethod,
BenchmarkClass.PrivateMethod,
string.Concat,
OtherBenchmarkClass.CorrectMethod });I though it just filtering somewhere in For reviewers:We can't add checks to
Oh, after today's commit by Adam it became much more safe. |
My bad. I only skimmed through it and didn't realize you've already done it.
Yeah, particularly the last one. The other two were skipped I think (I don't exactly remember the finer details). |
adamsitnik
reviewed
Aug 24, 2022
There was a problem hiding this comment.
@YegorStepanov big thanks for your contribution and apologies for such a delay in reviewing!
PTAL at my comments and let me know if you have the time to fix them.
| private static BenchmarkRunInfo[] TypeToBenchmarks(Type type, IConfig config) | ||
| { | ||
| if (type is null) | ||
| throw new InvalidBenchmarkDeclarationException("Type not provided."); |
There was a problem hiding this comment.
For nulls we should throw ArgumentNullException
Suggested change
| throw new InvalidBenchmarkDeclarationException("Type not provided."); | |
| throw new ArgumentNullException(paramName: nameof(type)); |
There was a problem hiding this comment.
Done (for arguments only, not for null values of the array) if I understood you correctly
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| private static Summary RunWithDirtyAssemblyResolveHelper(Type type, IConfig config, string[] args) | ||
| => (args == null |
There was a problem hiding this comment.
All the checks are performed only when args are null`. We should rather have something like:
$SomeType $SomeMethod($arguments, string[] args)
{
Validate($arguments); // throws when something is wrong
if (args is null)
BenchmarkSwitcher...
else
BenchmarkRunner...
}
There was a problem hiding this comment.
Done.
I wanted to mimic the behavior of BenchmarkSwitcher (BenchmarkRunner.Run used it when args != null).
But sometimes it would write a nice message and then throw exception (see test cases 6,7,8 in my comment)
| var wrongMethods = methods.Except(publicBenchmarkMethodsOfType).ToArray(); | ||
| if (!wrongMethods.IsEmpty()) | ||
| throw new InvalidBenchmarkDeclarationException(string.Join(", ", wrongMethods.Select(m => m.Name)) + | ||
| $" are wrong. Methods should be with [Benchmark] attribute of {containingType} type."); |
There was a problem hiding this comment.
nit: I would add "annotated" here
Suggested change
| $" are wrong. Methods should be with [Benchmark] attribute of {containingType} type."); | |
| $" are wrong. Methods should be annotated with [Benchmark] attribute of {containingType} type."); |
There was a problem hiding this comment.
There are already several different versions of this message. I found the most powerful (longest) of them in the library and used it.
|
Hi Adam, no problems, I will gladly take the time for this. |
YegorStepanov
force-pushed
the
check-BenchmarkRunner-arguments
branch
from
c15aa8a to
d9cb000
Compare
Codepublic class Program
{
static int benchmarkNumber = 0;
static string[] ARGS = null;
static string[] ARGS = Array.Empty<string>();
static void Main()
{
Console.WriteLine("null:");
RunType(null as Type);
RunTypes(null as Type[]);
RunAssembly(null as Assembly);
RunInfo(null as BenchmarkRunInfo);
RunInfos(null as BenchmarkRunInfo[]);
RunTypeMethods(null as Type, null as MethodInfo[]);
Console.WriteLine("RunT");
RunT<string>(DefaultConfig.Instance);
RunT<string>();
RunT<SealedBenchmarkClass>();
Console.WriteLine("RunTypes");
RunTypes(Array.Empty<Type>());
RunTypes(new Type[] { typeof(string) });
RunTypes(new Type[] { typeof(string), typeof(Benchmark1), typeof(object), typeof(List<string>) });
RunTypes(new Type[] { typeof(string), null });
RunTypes(new Type[] { null, typeof(Benchmark1) });
Console.WriteLine("RunMethods");
MethodInfo strMethod = typeof(string).GetMethods().First(m => m.Name == "Remove");
MethodInfo benchmarkMethod = typeof(Benchmark1).GetMethod("M1");
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { benchmarkMethod, null });
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { strMethod });
RunTypeMethods(typeof(string), new MethodInfo[] { benchmarkMethod });
RunTypeMethods(typeof(string), new MethodInfo[] { });
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { });
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { benchmarkMethod, typeof(Benchmark2).GetMethod("M1") });
RunTypeMethods(typeof(Benchmark1), typeof(Benchmark2).GetMethods());
Console.WriteLine("RunAssembly");
RunAssembly(typeof(string).Assembly);
Console.WriteLine("RunInfo");
var config = DefaultConfig.Instance.CreateImmutableConfig();
RunInfo(new BenchmarkRunInfo(null, null, null));
RunInfo(new BenchmarkRunInfo(null, null, config));
RunInfo(new BenchmarkRunInfo(null, typeof(Benchmark1), null));
RunInfo(new BenchmarkRunInfo(null, typeof(Benchmark1), config));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), null, null));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), null, config));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), typeof(Benchmark1), null));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), typeof(Benchmark1), config));
}
static void RunT<T>(IConfig config = null) =>
Catch(() => BenchmarkRunner.Run<T>(config, args: ARGS));
static void RunType(Type type) =>
Catch(() => BenchmarkRunner.Run(type, args: ARGS));
static void RunTypes(Type[] types) =>
Catch(() => BenchmarkRunner.Run(types, args: ARGS));
static void RunTypeMethods(Type type, MethodInfo[] methods) =>
Catch(() => BenchmarkRunner.Run(type, methods)); //no args overload
static void RunAssembly(Assembly assembly) =>
Catch(() => BenchmarkRunner.Run(assembly, args: ARGS));
static void RunInfo(BenchmarkRunInfo info) =>
Catch(() => BenchmarkRunner.Run(info)); //no args overload
static void RunInfos(BenchmarkRunInfo[] infos) =>
Catch(() => BenchmarkRunner.Run(infos)); //no args overload
static void Catch(Action action)
{
Console.Write(benchmarkNumber++ + ":");
try
{
action();
}
catch (Exception e)
{
Console.WriteLine(e.GetType().Name);
}
}
}
[DryJob]
public class Benchmark1
{
[Benchmark] public void M1() { }
}
[DryJob]
public class Benchmark2
{
[Benchmark] public void M1() { }
}
[DryJob]
public sealed class SealedBenchmarkClass
{
[Benchmark]
public void M1() { }
}The Run* methods catch exceptions and write numbers (to make it easier to find an empty output)
The output is the same for PR's nulls:RunType(null as Type);
RunTypes(null as Type[]);
RunAssembly(null as Assembly);
RunInfo(null as BenchmarkRunInfo);
RunInfos(null as BenchmarkRunInfo[]);
RunTypeMethods(null as Type, null as MethodInfo[]);Master args=null:Master args=emptyPRRunT == BenchmarkRunner.Run<T>();RunT<string>(DefaultConfig.Instance);
RunT<string>();
RunT<SealedBenchmarkClass>();Master args=null:Master args=emptyPRRunTypes == BenchmarkRunner.Run(Type[]);RunTypes(Array.Empty<Type>());
RunTypes(new Type[] { typeof(string) });
RunTypes(new Type[] { typeof(string), typeof(Benchmark1), typeof(object), typeof(List<string>) });
RunTypes(new Type[] { typeof(string), null });
RunTypes(new Type[] { null, typeof(Benchmark1) });Master args=null:Master args=emptyPRRunTypeMethods == BenchmarkRunner.Run(Type, MethodInfo[]);MethodInfo strMethod = typeof(string).GetMethods().First(m => m.Name == "Remove");
MethodInfo benchmarkMethod = typeof(Benchmark1).GetMethod("M1");
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { benchmarkMethod, null });
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { strMethod });
RunTypeMethods(typeof(string), new MethodInfo[] { benchmarkMethod });
RunTypeMethods(typeof(string), new MethodInfo[] { });
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { });
RunTypeMethods(typeof(Benchmark1), new MethodInfo[] { benchmarkMethod, typeof(Benchmark2).GetMethod("M1") });
RunTypeMethods(typeof(Benchmark1), typeof(Benchmark2).GetMethods());Master (no args overload):PRRunAssembly == BenchmarkRunner.Run(Assembly);RunAssembly(typeof(string).Assembly);Master args=null:Master args=emptyPRRunInfo == BenchmarkRunner.Run(BenchmarkRunInfo); var config = DefaultConfig.Instance.CreateImmutableConfig();
RunInfo(new BenchmarkRunInfo(null, null, null));
RunInfo(new BenchmarkRunInfo(null, null, config));
RunInfo(new BenchmarkRunInfo(null, typeof(Benchmark1), null));
RunInfo(new BenchmarkRunInfo(null, typeof(Benchmark1), config));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), null, null));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), null, config));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), typeof(Benchmark1), null));
RunInfo(new BenchmarkRunInfo(Array.Empty<BenchmarkCase>(), typeof(Benchmark1), config));Master (no args overload):PRRunInfos == BenchmarkRunner.Run(BenchmarkRunInfo[]);To construct BenchmarkRunInfo we should call BenchmarkCase.Create (because ctor is internal), which is checking arguments itself. |
|
A real PR output (without test numbers):
I like multiline with 2 spaces for invalid types/methods. |
YegorStepanov
commented
Aug 25, 2022
| var invalidMethods = methods.Except(benchmarkMethods).ToArray(); | ||
| if (!invalidMethods.IsEmpty()) | ||
| { | ||
| var invalidNames = string.Join("\n", invalidMethods.Select(m => $" {m.ReflectedType?.FullName}.{m.Name}")); |
There was a problem hiding this comment.
Maybe someone know, how to get "global" methods that return null for m.ReflectedType?
Module.GetMethods() returns empty array for BDN, BCL(string/object) modules.
MSDN says:
If the MemberInfo object is a global member (that is, if it was obtained from the Module.GetMethods method, which returns global methods on a module), the returned DeclaringType will be null.
There was a problem hiding this comment.
To check if given method belongs to the provided type you should be able to use method.DeclaringType
There was a problem hiding this comment.
@adamsitnik DeclaringType returns "System.Object.ToString", but we need to display "MyNameSpace.Benchmark2.ToString".
If we pass the Benchmark1.ToString and Benchmark2.ToString methods, we get:
DeclaringType:
System.Object.ToString
System.Object.ToString
ReflectedType:
NS.Benchmark1.ToString
NS.Benchmark2.ToString
There was a problem hiding this comment.
Found the answer when it returns null (this applies to DeclaringType as well):
It can be null if the property is defined in a module. In C# you cannot define such methods and properties without reflection (see PropertyBuilder). However, if you reference a VB.NET assembly, it can have such members.
|
There is a breaking change here: (Adam wrote half a year ago that this is expected) var methods = typeof(MyClass).GetMethods(); // returns M() GetType() ToString() Equals() GetHashCode()
BenchmarkRunner.Run(typeof(MyClass));
public class MyClass{ [Benchmark] public void M() { } }
// Current: execute only correct benchmark methods: M()
// PR: display an error that GetType(), ToString(), Equals(), GetHashCode() are not correct benchmark methods |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
It affects BenchmarkRunner.Run* with
args==nullonly.The first commit is not breaking change. It fast exits with message when no benchmarks to execute or when the benchmark crushes (when arguments are null).
The second commit adds fast exit when MethodInfo or Types are not benchmarkable.
Types
Now, the last line shows message too.
MethodInfo
In master, code below silently filters methods, now ALL methods should be benchmarkable and being of BenchmarkClass.
closes #1899 closes #1983