mapperly icon indicating copy to clipboard operation
mapperly copied to clipboard

Extract exception throwing to throw helper

Open jnyrup opened this issue 8 months ago • 4 comments

Is your feature request related to a problem? Please describe. When using [MapDerivedType] a switch expression is emitted with a throw expression for the default case. This is necessary for correctness, but the compiler emits quite some IL for creating and throwing the exception.

public class Base { }

public class Derived : Base { }

[Mapper]
public static partial class A
{
    [MapDerivedType<Derived, Derived>]
    public static partial Base Map(Base source);
}

Generates

// <auto-generated />
#nullable enable
public static partial class A
{
    [global::System.CodeDom.Compiler.GeneratedCode("Riok.Mapperly", "4.2.1.0")]
    public static partial global::Base Map(global::Base source)
    {
        return source switch
        {
            global::Derived x => x,
            _ => throw new global::System.ArgumentException($"Cannot map {source.GetType()} to Base as there is no known derived type mapping", nameof(source)),
        };
    }
}

Describe the solution you'd like If we extract the throw expression to it's own method Throw the Map method becomes considerably smaller. Smaller is not always better, if the extracted method is a hot path, as we then do an extra function call. But for throw expressions we're already about to create and throw an Exception which by far over shadows the cost of the extra function call. In .NET6 and upwards we can even use [StackTraceHidden] to exclude Throw from the stack trace to prettify it.

public static class B
{
    public static Base Map(Base source)
    {
        return source switch
        {
            Derived x => x,
            _ => Throw(source),
        };

        [StackTraceHidden]
        static Base Throw(Base source) => throw new ArgumentException($"Cannot map {source.GetType()} to Base as there is no known derived type mapping", nameof(source));
    }
}

Additional context Here's a benchmark of using the proposed throw helper.

| Method | Mean      | Error     | StdDev    | Ratio | RatioSD | Code Size |
|------- |----------:|----------:|----------:|------:|--------:|----------:|
| MapA   | 0.8429 ns | 0.0158 ns | 0.0148 ns |  1.00 |    0.02 |     739 B |
| MapB   | 0.6197 ns | 0.0105 ns | 0.0093 ns |  0.74 |    0.02 |     155 B |
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using Riok.Mapperly.Abstractions;
using System.Diagnostics;

BenchmarkRunner.Run<CloneBenchmarks>();

[DisassemblyDiagnoser(maxDepth: 2)]
public class CloneBenchmarks
{
    public Base Value { get; set; }

    [GlobalSetup]
    public void GlobalSetup()
    {
        Value = new Derived();
    }

    [Benchmark(Baseline = true)]
    public Base MapA() => A.Map(Value);

    [Benchmark]
    public Base MapB() => B.Map(Value);
}

jnyrup avatar May 04 '25 17:05 jnyrup

That sounds like a great improvement — thanks for suggesting it! I'd be happy to accept a PR implementing this. A good place to get started is our contributor documentation. Let me know if you decide to work on it 😊

latonz avatar May 06 '25 13:05 latonz

Great to hear. I'd like to give this a shot 🤞

I got it working for DerivedTypeSwitchMapping in https://github.com/jnyrup/mapperly/tree/issue1827. It clearly needs a lot of cleanup. I'll try to study the code base to make it a better fit, but if you have any pointers, I'll appreciate it.

jnyrup avatar May 25 '25 14:05 jnyrup

Thanks for jumping in on this & great to hear you’ve already got something working! I’ve assigned the issue to you 👍

Here are a few pointers that might help as you continue:

  • We still support .NET 5 and .NET Framework, so you'll need to check whether [StackTraceHidden] is available. To handle this, add a new member to SupportedFeatures, similar to how it's done for UnsafeAccessors.
  • Does it make sense to apply this to all exceptions thrown from generated code? You could start by looking at the references in SyntaxFactoryHelper.Exception.cs.
  • If we go with a general approach, generating a file-scoped helper class (rather than a local function per case) is likely cleaner and easier. You can use UnsafeAccessorContext as a reference.
  • Here’s a rough outline:
    • Add a new ExceptionContext (exact name TBD) to the DescriptorBuilder
    • For each exception helper to be generated, add an entry to this new context
    • Pass the context to the MapperDescriptor
    • InSourceEmitter, emit the required helper methods and apply [StackTraceHidden] only if supported (check SupportedFeatures).

Let me know if anything’s unclear or if you hit any roadblocks. Thank you for your efforts!

latonz avatar May 26 '25 07:05 latonz

I won't have time any time soon to make progress on this. So if anyone wants to take over, just go ahead.

jnyrup avatar Jul 09 '25 16:07 jnyrup