Consistency: Chains a call to NotNull #45
Comments
|
Actually I think we should go the other way: go ahead and use Do you know how to test what the JIT inlines so we can validate either hypothesis? But if you do make a change to this code, please send a PR to vs-validation as well to help them stay in sync. :) |
|
I can change them to call I don't know how to validate what the JIT inlines, though, short of looking at the compiled code in something like JustDecompile...or is that what you're thinking of? I'll submit a separate PR for this, and then once it gets approved will submit one to vs-validation. That way, any changes that might be needed can be done here first and the PR to vs-validation ends up being cleaner. |
|
So, this is interesting. I used shaplab.io to look at the JIT Asm code for a much slimmed down version of the #nullable enable
namespace Validation
{
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Runtime;
using System.Runtime.CompilerServices;
public static class Requires
{
[TargetedPatchingOptOut("Performance critical to inline across NGen image boundaries")]
public static IEnumerable<string> NotNull([NotNull] IEnumerable<string> value, string? parameterName)
{
if (value is null) throw new ArgumentNullException(parameterName);
return value;
}
public static void NotNullEmptyOrNullElements([NotNull] IEnumerable<string> values, string? parameterName)
{
NotNull(values, parameterName);
bool hasElements = false;
foreach (string? value in values)
{
hasElements = true;
if (value is null) throw new ArgumentException(Format("{0} cannot be null", parameterName), parameterName);
}
if (!hasElements) throw new ArgumentException(Format("{0} cannot be an empty array", parameterName), parameterName);
}
private static string Format(string format, params object?[] arguments) => String.Format(format, arguments);
}
}
#nullable restoreThe generated IL (in Release mode) for That looks like the call to All of the above was using the "Default" compilation option, which is If I change it to use Even if I added a helper method private static void ThrowArgumentNullException(string? parameterName) => throw new ArgumentNullException(parameterName);and changed the code in Unless I'm missing something, or I'm interpreting the results incorrectly, we wouldn't be able to get the calls to |
|
Very interesting. Well, let's move toward writing out the check and throw by hand then instead of calling NotNull. :) |
|
Agreed. I was a bit surprised there was that much difference between There are only a couple of places that I saw where |
To be clear, it isn't .NET Standard that makes the difference in what gets inlined. It's the JIT in the runtime. So it's a comparison of .NET Core and .NET Framework. |
|
True. That's what I meant. I usually think of them as synonymous, but they really aren't. |
|
Should we only focus on the two |
|
Ya, I wouldn't worry about perf for BTW, perf in this area hasn't come up for years. I personally wouldn't bother authoring these micro-optimizations. But if you're passionate about it for whatever reason, so long as it doesn't regress anything, I'm happy to take them. |
|
I only looked at public static void NotNullOrEmpty<T>(ICollection<T>? values)
{
NotNull(values);
True(values.Count > 0);
}won't inline the call to public static void NotNullOrEmpty<T>(ICollection<T>? values)
{
True(values is object);
True(values.Count > 0);
}then it does inline. What's strange is that |
|
Generics might explain it. The PerfView ETW events include explanations from the JIT as to why something won't inline, BTW. |
|
Nice. I didn't know the ETW events included explanations. I'll have to check that out. |
Replaces the call to NotNull with a direct test. Closes #45.
In all of the other methods that validate the parameter is not null, we're explicitly testing
if (value is null)with a comment that readsHowever, in
Requires.NotNullOrEmptywe don't do that and instead, callNotNull. Should we change this so it's consistent?Validation/src/Validation/Requires.cs
Line 249 in 2378284
The text was updated successfully, but these errors were encountered: