unify ThrowHelper methods

[danmoseley]

dan@danmoseL:~/git/runtime/src/libraries$ grep -r --include "*.cs" "class.*ThrowHelper"
System.IO.Pipelines/src/System/IO/Pipelines/ThrowHelper.cs:    internal static class ThrowHelper
System.Text.Encodings.Web/src/System/Text/Encodings/Web/ThrowHelper.cs:    internal static class ThrowHelper
System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs:    internal static partial class ThrowHelper
System.Text.Json/src/System/Text/Json/ThrowHelper.cs:    internal static partial class ThrowHelper
System.Text.Json/src/System/Text/Json/ThrowHelper.Node.cs:    internal static partial class ThrowHelper
System.Private.CoreLib/src/System/ThrowHelper.cs:    internal static class ThrowHelper
Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ThrowHelper.cs:    internal static class ThrowHelper
System.Security.Cryptography.Encoding/src/System/Security/Cryptography/Base64Transforms.cs:    internal static class ThrowHelper
System.Linq/src/System/Linq/ThrowHelper.cs:    internal static class ThrowHelper
Microsoft.Extensions.Primitives/src/ThrowHelper.cs:    internal static class ThrowHelper
Common/src/Internal/Cryptography/Windows/CryptoThrowHelper.cs:    internal static class CryptoThrowHelper
Common/src/System/Text/Json/PooledByteBufferWriter.cs:    internal static partial class ThrowHelper
System.Text.RegularExpressions/src/System/Text/RegularExpressions/ThrowHelper.cs:    internal static class ThrowHelper
System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/ThrowHelper.cs:    internal static class ThrowHelper
System.Memory/src/System/ThrowHelper.cs:    internal static class ThrowHelper
System.Collections.Concurrent/src/System/ThrowHelper.cs:    internal static class ThrowHelper

between them there is plenty of duplication, eg

dan@danmoseL:~/git/runtime/src/libraries$ grep -r --include "*.cs" "void\s*ThrowInvalidOperationException("
System.Text.Json/src/System/Text/Json/ThrowHelper.cs:        public static void ThrowInvalidOperationException(int currentDepth)
System.Text.Json/src/System/Text/Json/ThrowHelper.cs:        public static void ThrowInvalidOperationException(string message)
System.Text.Json/src/System/Text/Json/ThrowHelper.cs:        public static void ThrowInvalidOperationException(ExceptionResource resource, int currentDepth, byte token, JsonTokenType tokenType)
System.Private.CoreLib/src/System/ThrowHelper.cs:        internal static void ThrowInvalidOperationException()
System.Private.CoreLib/src/System/ThrowHelper.cs:        internal static void ThrowInvalidOperationException(ExceptionResource resource)
System.Private.CoreLib/src/System/ThrowHelper.cs:        internal static void ThrowInvalidOperationException(ExceptionResource resource, Exception e)
Microsoft.Extensions.Primitives/src/ThrowHelper.cs:        internal static void ThrowInvalidOperationException(ExceptionResource resource)
Microsoft.Extensions.Primitives/src/ThrowHelper.cs:        internal static void ThrowInvalidOperationException(ExceptionResource resource, params object[] args)
System.Memory/src/System/ThrowHelper.cs:        internal static void ThrowInvalidOperationException() { throw CreateInvalidOperationException(); }
System.Memory/tests/SequenceReader/OwnedArray.cs:        public static void ThrowInvalidOperationException()

We should reduce the amount of duplicated code here by at least pulling the common stuff out into a shared file.

Incidentally @BruceForstall do we have an issue for codegen to long term allow us to eliminate throw helpers? I can't find it.

[stephentoub]

This doesn't seem very valuable to me. Is there really a lot that's duplicated? In general each library using this pattern treats it as very fluid and adds only what and exactly what it needs. Devs would now need to think about how that impacts other libraries, and it doesn't seem like there'd be a lot of gain.

I would, however, love to see the need for ThrowHelper go away entirely via IL codegen / an optimization pass.

[BruceForstall]

Incidentally @BruceForstall do we have an issue for codegen to long term allow us to eliminate throw helpers? I can't find it.

I don't recall seeing one, but I didn't check :-) This probably affects inlining as well as other optimizations. cc @AndyAyersMS

[AndyAyersMS]

One route to eliminating the need for throw helpers is to revive the work I did for partial jitting (eg #34522). But we'd also want it to work for optimized code, which is quite a bit more complicated.

I used a prototype of this to project that we could save about ~2% of SPC R2R code size this way.

[jkotas]

I think the best solution for the ThrowHelper problem would be an IL optimization pass at build time that creates them in more compact and optimal shape than humans ever would. .NET Native for UWP had an optimization pass like that.

[danmoseley]

🤷‍♀️ I don't feel strongly.

an IL optimization pass at build time

Do we have tooling for this? That would also fix up symbols for debugging?

Aiming for some generality to any code it would perhaps do something like

1. find all exceptions being constructed in place and thrown that have no or only simple parameters -- primitive types, strings, or simple method call/property get that is only invoked when the exception is to be thrown and whose return value is only passed to the constructor
2. extract fixed strings to an enum
3. replace method call/property get with an enum and a switch.
4. leave all other parameters unchanged
5. extract result into a helper method, with reuse.

something like that?

For fun, I looked at the IL and asm for these (likely counting wrong)

    void F(int i) => throw new ArgumentException(nameof(i), ArgumentNull_Key);   // 16 bytes IL -> 104 bytes x64 

    void G(int i) => ThrowArgumentException(nameof(i), ArgumentNull_Key); // 17 bytes IL -> 43 bytes x64   

    void H(int i) => ThrowArgumentException(ExceptionArgument.obj, ExceptionResource.ArgumentNull_Key); // 9 bytes IL -> 27 bytes x64,

I don't know whether changes would be needed on the codegen side, e.g, to know that these helper methods represent cold code, as unconditional throws.

[jkotas]

We do not have a place to add optimizations like this today. Roslyn and IL trimmer do not want to do optimizations. AOT/R2R compiler has platform specific optimizer that does not rewrite IL. So it would need to be a new step or tool in the build toolchain. Maybe we should start with a simple single-purpose tool based on System.Reflection.Metadata as an experiment and see where it gets us.

Yes, fixing up debug info takes some care, but it has been done before.

extract fixed strings to an enum

Unclear whether enum is the most compact format to use. Each method and each enum value come with extra metadata that also count towards the binary size and memory footprint. The metadata footprint is typically more than code size for small methods. It is one of the reasons why handwriting it in most compact format is hard.

[danmoseley]

If experiments prove value perhaps Roslyn would consider an extension point for IL optimization at their emitter stage, especially if that point made debug symbols fix ups natural. Others might use such a mechanism.

[danmoseley]

Cc @jaredpar

[Joe4evr]

an IL optimization pass at build time

Do we have tooling for this?

@danmoseley FYI, there's been a lot of talk about that kind of thing over here: dotnet/roslyn#15929

[danmoseley]

Thanks @Joe4evr. Seems discussion there migrated to it being done on the linker. Is IL optimization considered in scope for the linker today?

Yet another option is an extension of source generators that can override code. I see a link from that discussion to a proposal for "supersede". I guess I do not know whether LDM will ever support that.

[jaredpar]

I think the best solution for the ThrowHelper problem would be an IL optimization pass at build time that creates them in more compact and optimal shape than humans ever would

What would the structure of such IL look like?

If experiments prove value perhaps Roslyn would consider an extension point for IL optimization at their emitter stage, especially if that point made debug symbols fix ups natural.

The bar for this type of feature is extremely high for the same reason we restrict source generators to adding code only, not modifying it. The moment we allow extension points for modifying existing code then one of the largest value props of C# goes out the window: the ability to have a strongly spec'd language with specified behavior and rules. Arbitrary IL rewriting is essentially "everything becomes undefined" behavior land.

Others might use such a mechanism.

You seemed to have mis-spelled "will" as "might" and "abuse" as "use" 😄

[jkotas]

What would the structure of such IL look like?

Something like this to start with: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq/src/System/Linq/ThrowHelper.cs

• Throw* helper methods would be auto-generated for all places that do throw new SomeException(SR.SomeMessage) or throw new BadArgumentException(SR.BadArgument, "argumentName"), and the original throw statements would be replaced by calls to these methods.
• Throw* methods that are similar, but not exactly the same (e.g. same message, different argument name) would take an extra int argument to make the code more compact where possible. The example that I shared uses enum for this purpose, but that is not as compact as possible.

[jeffhandley]

Closing this as it stalled without reaching agreement that it would be worthwhile.