Add new factory method for ModelInspector that accepts predefined ClassMapping/EnumMapping/PropertyMapping to skip reflection/assembly scanning overhead

almostchristian commented 4 months ago

Our company is exploring on using our FHIR web api framework inside an AWS Lambda function, but after investigating the slowness in our cold starts, we found that 40% of the cold start duration was spent on the initializing ModelInfo.ModelInspector.

Flame graph below Screenshot 2024-05-27 235203

Most of the time is spent doing assembly scanning for types, nested types and recursively scanning referenced assemblies., there should be a new constructor for ModelInspector that will create a fully configured ModelInspector without assembly scanning and type scanning for nested types/enums. As further enhancement, we can add a source generator that generates code that calls this new constructor instead of ModelInspector.ForAssembly(typeof(ModelInfo).GetTypeInfo().Assembly).

Below is the lambda code I used to generate the above graph:

var builder = WebApplication.CreateBuilder(args);
builder.Services.ConfigureHttpJsonOptions(o => o.SerializerOptions.ForFhir());
builder.Services.AddSingleton(sp => new CapabilityStatement { Kind = CapabilityStatementKind.Capability });
builder.Services.AddAWSLambdaHosting(LambdaEventSource.HttpApi);

var app = builder.Build();

app .UseRouting()
    .UseEndpoints(endpoints =>
    {
        endpoints.MapPost("/Appointment", (Appointment appt) => appt);
        endpoints.MapGet("/metadata", ([FromServices] CapabilityStatement cap) => cap);
    });

await app.RunAsync();

Proposed API changes

ModelInspector

public static ModelInspector ForPredefinedMappings(string fhirVersion, IEnumerable<ClassMapping> classMappings, IEnumerable<EnumMapping> enumMappings);

ClassMapping

Also, properties in ClassMapping, EnumMapping and PropertyMapping will be updated to be required init instead of private setters or get only properties.

The constructor for ClassMapping will have a propertyMappingFactory argument.

public ClassMapping(Func<ClassMapping, PropertyMapping[]>? propertyMappingFactory = null)
   : this(propertyMapFactory != null ? cm => new PropertyMappingCollection(propertyMappingFactory(cm)): inspectProperties)
{
}

private ClassMapping(Func<ClassMapping, PropertyMappingCollection> propertyMappingFactory)
{
    _propertyMappingFactory = propertyMappingFactory;
 }

EnumMapping

There will be a new constructor for EnumMapping that accepts a memberMappingFactory so that reflection can be skipped.

public EnumMapping(string? defaultCodeSystem)
{
    _mappings = new(() => mappingInitializer(defaultCodeSystem));
}

public EnumMapping(Func<IReadOnlyDictionary<string, EnumMemberMapping>> memberMappingFactory)
{
    _mappings = new(valueFactory: memberMappingFactory);
}

PropertyMapping

The constructor for PropertyMapping will accept getter and setter arguments. When these are missing, they will be generated from the NativeProperty property.

private PropertyMapping(PropertyInfo nativeProperty)
{
    _nativeProperty = nativeProperty;
}

public PropertyMapping(
    Func<object, object?> getter,
    Action<object, object?> setter)
{
    _getter = getter;
    _setter = setter;
}

private PropertyInfo? _nativeProperty;

public PropertyInfo NativeProperty => _nativeProperty ?? LazyInitializer.EnsureInitialized(
    ref _nativeProperty,
    () => Array.Find(ImplementingType.GetProperties(BindingFlags.Public | BindingFlags.Instance), x => x.GetCustomAttribute<FhirElementAttribute>()?.Name == Name)!)!;

public object? GetValue(object instance) => ensureGetter()(instance);

private Func<object, object?>? _getter;

private Func<object, object?> ensureGetter()
    => _getter ?? LazyInitializer.EnsureInitialized(ref _getter, NativeProperty.GetValueGetter)!;

public void SetValue(object instance, object? value) => ensureSetter()(instance, value);

private Action<object, object?>? _setter;

private Action<object, object?> ensureSetter()
    => _setter ?? LazyInitializer.EnsureInitialized(ref _setter, () => NativeProperty.GetValueSetter())!;

Benchmarks

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Gen1	Allocated	Alloc Ratio
ScanAssemblies	10,201.36 μs	167.577 μs	139.934 μs	1.000	0.00	375.0000	140.6250	3130.62 KB	1.00
ImportTypeAllResources	7,651.36 μs	150.712 μs	263.961 μs	0.735	0.02	343.7500	156.2500	2924.1 KB	0.93
SourceGenMappingsAllResources	241.15 μs	4.638 μs	4.763 μs	0.024	0.00	70.8008	27.3438	578.79 KB	0.18
ImportType4Resources	1,298.84 μs	22.972 μs	36.436 μs	0.126	0.00	66.4063	17.5781	542.83 KB	0.17
SourceGenMappings4Resources	39.77 μs	0.785 μs	0.806 μs	0.004	0.00	10.6201	1.7090	86.95 KB	0.03

*updated proposed new api and benchmark numbers

ewoutkramer commented 4 months ago

Yes, our current setup is a bit skewed towards preferring upfront setup cost (like what you would want in a server or a client). With this new solution you would have to know the types in advance - is that something you can guarantee in your case?

mmsmits commented 4 months ago

Thanks for this! We will make this possible in the SDK. Does ReadOnlySpan<Type> make a difference here? We don't use that anywhere else in the SDK, but if this makes a difference we can use it here.

We should also document how to use this.

almostchristian commented 4 months ago

I don't think ReadOnlySpan<> really helps since you do this only once, only a minor reduction in allocation. Maybe just sticking with IEnumerable<T> should be enough.

To guarantee all the types are present, I think using source generation would be the best approach. I tried my hand creating a source generator for this and it seems to work well.

almostchristian commented 4 months ago

Also in the future, we can create a source generator that creates the ClassMapping and EnumMapping removing the need for reflection at runtime.

almostchristian commented 4 months ago

I've improved my source generator to generate the ClassMapping and EnumMapping and I get the results below:

Method	Mean	Error	StdDev	Median	Gen0	Gen1	Allocated
ScanAssemblies	11,000.4 μs	520.96 μs	1,536.05 μs	11,137.1 μs	375.0000	218.7500	3061.86 KB
ImportTypeAllResources	7,208.8 μs	195.20 μs	556.92 μs	7,054.3 μs	328.1250	171.8750	2797.81 KB
ImportType4Resources	155.6 μs	2.99 μs	3.67 μs	155.0 μs	12.6953	1.7090	104.98 KB
NewWithSourceGenMappings	171.0 μs	2.96 μs	6.05 μs	169.8 μs	46.1426	15.3809	376.91 KB
NewWithTypesAllResources	4,208.6 μs	84.09 μs	220.05 μs	4,159.2 μs	273.4375	140.6250	2277.65 KB
NewWithTypes4Resources	132.1 μs	2.61 μs	4.29 μs	131.2 μs	10.4980	0.4883	85.77 KB

The SourceGenMappings gives a very hefty performance improvement (171us vs 4208.6us)

Some issues I found in the current API is that ClassMapping and EnumMapping have private constructors and private property setters that make it difficult for any source generator to work with.

Please try out my branch to see if this is something the team is interested in integrating

almostchristian commented 4 months ago

@mmsmits

I've refined the new API that would be helpful for source generating the ModelInspector below:

Proposed new API

ModelInspector

public ModelInspector(string fhirVersion, IEnumerable<ClassMapping> classMappings, IEnumerable<EnumMapping> enumMappings)
{
    _classMappings = new ClassMappingCollection(classMappings);
    _enumMappings = new EnumMappingCollection(enumMappings);
    FhirVersion = fhirVersion;
    FhirRelease = FhirReleaseParser.Parse(fhirVersion);
}

public static ModelInspector ForTypes(string version, ReadOnlySpan<Type> types)
{
    var fhirRelease = FhirReleaseParser.Parse(version);
    var classMappings = new List<ClassMapping>(types.Length);
    var enumMappings = new List<EnumMapping>(types.Length);
    foreach (var type in types)
    {
        if (!type.IsEnum && ClassMapping.TryCreate(type, out var classMapping, fhirRelease))
        {
            classMappings.Add(classMapping);
        }
        else if (type.IsEnum && EnumMapping.TryCreate(type, out var enumMapping, fhirRelease))
        {
            enumMappings.Add(enumMapping);
        }
    }

    return new ModelInspector(version, classMappings, enumMappings);
}

ClassMapping

public static ClassMapping Build(
    string name,
    Type nativeType,
    FhirRelease release,
    bool isResource = false,
    bool isCodeOfT = false,
    bool isFhirPrimitive = false,
    bool isPrimitive = false,
    bool isBackboneType = false,
    string? definitionPath = null,
    bool isBindable = false,
    string? canonical = null,
    ValidationAttribute[]? validationAttributes = null,
    Func<ClassMapping, PropertyMapping[]>? propertyMapFactory = null)
{
    Func<ClassMapping, PropertyMappingCollection>? propMappingFactory = null;
    if (propertyMapFactory != null)
    {
        propMappingFactory = cm => new PropertyMappingCollection(propertyMapFactory(cm));
    }

    var mapping = new ClassMapping(name, nativeType, release, propMappingFactory)
    {
        IsResource = isResource,
        IsCodeOfT = isCodeOfT,
        IsFhirPrimitive = isFhirPrimitive,
        IsPrimitive = isPrimitive,
        IsBackboneType = isBackboneType,
        DefinitionPath = definitionPath,
        IsBindable = isBindable,
        Canonical = canonical,
        ValidationAttributes = validationAttributes ?? [],
    };

    _mappedClasses.GetOrAdd((nativeType, release), mapping);

    return mapping;
}

private ClassMapping(string name, Type nativeType, FhirRelease release, Func<ClassMapping, PropertyMappingCollection>? propertyMappingFactory = null)
{
    Name = name;
    NativeType = nativeType;
    Release = release;
    _propertyMappingFactory = propertyMappingFactory ?? inspectProperties;
}

EnumMapping

public static EnumMapping Build(string name, string? canonical, Type nativeType, FhirRelease release, string? defaultCodeSystem = null)
    => new EnumMapping(name, canonical, nativeType, release, defaultCodeSystem);

I created a source generator that generates the ClassMappings and EnumMappings and was able to achieve the numbers below:

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
ScanAssemblies	8,240.16 μs	163.741 μs	212.909 μs	375.0000	140.6250	3081.86 KB
ImportTypeAllResources	6,619.96 μs	126.732 μs	296.233 μs	343.7500	156.2500	2909.46 KB
SourceGenMappingsAllResources	161.95 μs	3.177 μs	6.562 μs	47.8516	14.8926	392.86 KB
NewWithTypesAllResources	3,972.23 μs	77.484 μs	64.703 μs	281.2500	117.1875	2304.15 KB
ImportType4Resources	858.91 μs	16.936 μs	35.352 μs	64.4531	15.6250	535.49 KB
SourceGenMappings4Resources	28.14 μs	0.537 μs	0.503 μs	7.3242	0.7935	60.01 KB
NewWithTypes4Resources	352.53 μs	6.576 μs	12.511 μs	28.3203	3.9063	235.47 KB

Generated ModelInspector https://gist.github.com/almostchristian/81a18cfa62816213e9d3133ee2b0cf7e

https://github.com/almostchristian/firely-net-sdk/tree/feature/model-inspector-sourcegen

mmsmits commented 4 months ago

Wow, thanks for this! We will definitely take a good look at this!

ewoutkramer commented 3 months ago

Ok, I think we should do this. Our goal for a new PR would be to change the ClassMapping/PropertyMapping/ModelInspector in a way that we:

Don't make breaking changes
Make them more useable for a code generator.

Allow me to suggest some modifications:

ModelInspector API changes

I don't see you need a new ForTypes() overload, at least, it's not used in the generated code (https://gist.github.com/almostchristian/81a18cfa62816213e9d3133ee2b0cf7e) - do we still need it?
Instead of adding a new constructor to ModelInspector, I think we should do it as a static factory method - I'd like to keep the constructors for simple, common constructions. This is quite a "specialty" constructor, so an explicit factory method can described the intent better. E.g. what about public ForPredefinedMappings(string version, List<ClassMapping> classMappings, List<EnumMapping> enumMappings)?

ClassMapping

I don't think we need a new Build() method. This code was from before we had the required keyword, so I needed a constructor with the "required" properties + the rest was initializable. As far as I am concerned, we can now have a parameterless constructor + make the properties in the original constructor required.
I like the property factory parameter. but since I don't think it should be a property, can we pass this as the only parameter to the constructor?

PropertyMapping

Same story - just a new no-param constructor with required properties.

Agree? Suggestions?

almostchristian commented 2 months ago

The ForTypes method is no longer necessary since using it will inevitably require reflection. I agree with using the ForPredefinedMappings factory method.

For ClassMapping and PropertyMapping, yes I agree to make the private set properties be required init so we can remove the need for the Build factory methods.

However PropertyMapping has some tricky issues:

Release is a public readonly field, and changing it to an required init property which may break compatibility.
The NativeProperty, which is also a public readonly field, is not required to be set and is only required to generate the getter and setter implementation. Since we can generate the getter and setter via source gen, we can remove the need for this field and in my test source generator, this is always null. We can add a function to retrieve the PropertyInfo for backward compatibility but then we'd have to save the actual property name so it can be retrieved later.

public class PropertyMapping
{
   private PropertyMapping(PropertyInfo nativeProperty)
   {
      _nativeProperty = nativeProperty;
   }

   public PropertyMapping(
      Func<object, object?> getter,
      Action<object, object?> setter)
   {
      _getter = getter;
      _setter = setter;
   }

   public PropertyInfo NativeProperty => _nativeProperty ?? LazyInitializer.EnsureInitialized(
      ref _nativeProperty,
      () => ImplementingType.GetProperties(BindingFlags.Public | BindingFlags.Instance).First(x => x.GetCustomAttribute<FhirElementAttribute>()?.Name == Name)!)!;

   private Func<object, object?> EnsureGetter()
      => _getter ?? LazyInitializer.EnsureInitialized(ref _getter, NativeProperty.GetValueGetter)!;

   private Action<object, object?> EnsureSetter()
      => _setter ?? LazyInitializer.EnsureInitialized(ref _setter, () => NativeProperty.GetValueSetter())!;
}

Also, EnumMapping and EnumMemberMapping also needs to be updated to be similar to ClassMapping in that the properties should be updated to be required init properties instead of get only properties. Also, EnumMapping will have a constructor that accepts a memberMapping factory.

public EnumMapping(string? defaultCodeSystem)
{
    _mappings = new(() => mappingInitializer(defaultCodeSystem));
}

public EnumMapping(Func<IReadOnlyDictionary<string, EnumMemberMapping>> memberMappingFactory)
{
    _mappings = new(valueFactory: memberMappingFactory);
}

ewoutkramer commented 1 day ago

I have further (breaking) improvements to make, so I am now planning to integrate this PR in the SDK 6.0

FirelyTeam / firely-net-sdk