search

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

Would there be any benefit to these optimizations? #91

Closed JesseTG closed 2 years ago

JesseTG commented 2 years ago

I'm using this library in my project because I'm fallible, therefore I'd like to make it easy to check my code for simple mistakes like unwanted nulls. But since my project is a Unity game, I also don't want these checks to eat up too many cycles in published builds.

Do you believe using [MethodImpl] to aggressively inline or optimize most of this library's validation methods would have a significant impact on performance?

What about using [Conditional] to strip certain methods? Is that something you think would be useful to configure with a #define?

AArnott commented 2 years ago

The Report class already has [Conditional] on it, so if you'd like debug-only checks, you can use that.

As for MethodImpl to force inlining, I wouldn't do that unless there was evidence that it would make a significant difference. Most of these methods are small and are written with the throw statement in a helper method to encourage the JIT in inline them already.

JesseTG commented 2 years ago

The Report class already has [Conditional] on it, so if you'd like debug-only checks, you can use that.

Report only has a subset of the checks I use. If I submitted a PR that added more (but [Conditional]-gated), would you accept it?

On a related note, how long are [Conditional]-annotated methods kept around? Are they completely stripped when the assembly is built, or is that a decision the JIT makes later?

As for MethodImpl to force inlining, I wouldn't do that unless there was evidence that it would make a significant difference. Most of these methods are small and are written with the throw statement in a helper method to encourage the JIT in inline them already.

Fair enough.

AArnott commented 2 years ago

The [Conditional] behavior is something implemented by the C# compiler that you can read about. But our use of it means that when you compile for Debug, the calls remain. When you compile for Release, those calls disappear.

No, I probably wouldn't take a PR that adds more methods without accompanying data to show how calling the Requires, Verify or other class methods negatively impacts performance. We use these in a lot of perf critical paths and it simply hasn't come up as a problem.

What has shown on perf traces is looking up string resources for error messages, which are passed into the Validation methods even in success cases. To solve this, #83 was merged recently. This was a perf-data informed addition of methods so we know they are worth the added APIs.

JesseTG commented 2 years ago

Gotcha, thank you.