AArnott / Validation

Method input validation and runtime checks that report errors or throw exceptions when failures are detected.
Microsoft Public License
131 stars 22 forks source link

Massive String allocation in Requires #73

Closed AnakinRaW closed 3 years ago

AnakinRaW commented 3 years ago

Hey Andrew, I'm not sure if this is an design choice but i though i'll just leave that here.

I'm using Requires in many methods which get called on multiple threads/tasks a massive amount of times. Since all methods in Requires have a string in their parameter i end up allocating thousands if not millions of string objects. Are there any thoughts to mitigate this for this library or am I just "abusing" this library ;)

Cheers!

AArnott commented 3 years ago

Are the strings your message arguments, because you look them up from your string resources? There's nothing that can be done in the library to avoid that at this point. But the way I handle it is change the hot paths to check the condition first, then call into the library's Fail method only on failure.

AArnott commented 3 years ago

Or maybe we could add overloads to support this syntax:

Requires.Argument(a >= 0, nameof(a), Strings.ResourceManager, nameof(Strings.MustBePositive));
AArnott commented 3 years ago

We could even develop an analyzer with a code fix to convert the less efficient syntax to the more efficient syntax.

AnakinRaW commented 3 years ago

My common usage is just Requires.NotNull(param, nameof(param));

i currently ended up to replace this call in hot methods with classic

if (param is null)
    throw new ArgumentNullException(nameof(param));

since this only allocates when the param is actually null.

My guess is that all the Requires methods mostly will get a nameof(param) expression as their message argument. In that case there could be a stringless method overload which avoids allocation until the condition fails.

AArnott commented 3 years ago

The syntax you mention does not allocate anything. nameof does not allocate anything. Check the IL.

AnakinRaW commented 3 years ago

Hmm. I'm not sure then, if I'm on the right track. I profiled my application with the VS .NET Object allocation tracking tool. It showed String had the highest value in all Ctros where I had Requires.IsNull<T> used. When i replaced it with the manual if check the value of String in the diagnostic tool dropped.

I must be honest, I'm not very experienced in that matter, so I don't really know if all this causes perf problems. So if you say that's all normal and it won't harm my application then i think this issue can be closed safely and i lerned something :)

AArnott commented 3 years ago

There is nothing to allocate strings in that method:

https://github.com/AArnott/Validation/blob/da8ec460a32c52c10af0dad07f0b0edb855b4559/src/Validation/Requires.cs#L30-L39

Maybe something else about your code change removed an allocation.

AnakinRaW commented 3 years ago

Alrights. thanks for your assitance, I'm not sure what acutally causes my allocations but apparently it's not due to this framework. I'll close the issue. Cheers 😄