Merge remote-tracking branch 'origin/develop' into fix/serialization-improvements-2

Author: andrzejskowronski

Directory.Packages.props

@@ -8,18 +8,18 @@
 	<!-- Testing Packages (all frameworks) -->
 	<ItemGroup>
 		<PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.13.0" />
-		<PackageVersion Include="MSTest.TestFramework" Version="4.3.2" />
-		<PackageVersion Include="MSTest.TestAdapter" Version="4.3.2" />
+		<PackageVersion Include="MSTest.TestFramework" Version="4.3.3" />
+		<PackageVersion Include="MSTest.TestAdapter" Version="4.3.3" />
 		<PackageVersion Include="xunit" Version="2.9.2" />
 		<PackageVersion Include="xunit.runner.visualstudio" Version="3.1.0" />
 		<PackageVersion Include="FluentAssertions" Version="7.2.2" />
 		<PackageVersion Include="NSubstitute" Version="6.0.0" />
-		<PackageVersion Include="Verify.MSTest" Version="31.27.0" />
+		<PackageVersion Include="Verify.MSTest" Version="31.28.0" />
 	</ItemGroup>
 
 	<!-- Main Dependencies (all frameworks) -->
 	<ItemGroup>
-		<PackageVersion Include="Fhir.Metrics" Version="1.3.1" />
+		<PackageVersion Include="Fhir.Metrics" Version="1.4.0" />
 		<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="10.0.301" />
 		<PackageVersion Include="Newtonsoft.Json" Version="13.0.4" />
 		<PackageVersion Include="System.ComponentModel.Annotations" Version="5.0.0" />

src/Hl7.Fhir.Base/Introspection/ClassMapping.cs

@@ -187,22 +187,35 @@ internal static bool TryCreate(ModelInspector parent, Type type, [NotNullWhen(tr
     // This list is created lazily. This not only improves initial startup time of
     // applications but also ensures circular references between types will not cause loops.
     private PropertyMappingCollection? _mappings;
-
-    private PropertyMappingCollection PropertyMappingsInternal
+    private object? _mappingsLock;
+
+    // Note on the `_field ?? LazyInitializer.EnsureInitialized(...)` shape of this member, the
+    // other lazily initialized members below and the equivalents in PropertyMapping and
+    // PropertyMappingCollection: calling EnsureInitialized unconditionally would allocate its
+    // factory delegate on *every* access, including the warm path where the field has long been
+    // initialized, and these members are read per element on the (de)serialization path. The
+    // null-coalescing read short-circuits that: once the field is initialized, an access is an
+    // ordinary read that allocates nothing, and the factory delegate is only created on the
+    // once-only cold path. The ordinary read is enough because the runtime guarantees that
+    // object reference stores are release stores and that data-dependent reads are ordered, so a
+    // thread observing the field non-null also observes the fully initialized object; see
+    // https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md.
+    //
+    // This member uses the EnsureInitialized overload with a syncLock, so that the (possibly
+    // user-supplied) property mapper building this mutable collection runs exactly once. The other
+    // lazy members have idempotent factories whose results are never mutated afterwards, so they
+    // use the lock-free overload, where racing factories may run concurrently but only a single
+    // result is ever published and handed out.
+    private PropertyMappingCollection PropertyMappingsInternal =>
+        _mappings ?? LazyInitializer.EnsureInitialized(ref _mappings, ref _mappingsLock, createPropertyMappings)!;
+
+    private PropertyMappingCollection createPropertyMappings()
     {
-        get
-        {
-            return LazyInitializer.EnsureInitialized(ref _mappings, createCollection)!;
+        var properties = propertyMapper(this).ToList();
+        if(properties.FirstOrDefault(m => m.DeclaringClass != this) is {} errorMapping)
+            throw new InvalidOperationException($"PropertyMapping '{errorMapping.Name}' is already used for another ClassMapping '{errorMapping.DeclaringClass.Name}'.");
 
-            PropertyMappingCollection createCollection()
-            {
-                var properties = propertyMapper(this).ToList();
-                if(properties.FirstOrDefault(m => m.DeclaringClass != this) is {} errorMapping)
-                    throw new InvalidOperationException($"PropertyMapping '{errorMapping.Name}' is already used for another ClassMapping '{errorMapping.DeclaringClass.Name}'.");
-
-                return new PropertyMappingCollection(properties);
-            }
-        }
+        return new PropertyMappingCollection(properties);
     }
 
     /// <summary>
@@ -215,7 +228,7 @@ PropertyMappingCollection createCollection()
     /// property will also be present in the PropertyMappings collection. If this class has
     /// no such property, it is null.
     /// </summary>
-    public PropertyMapping? PrimitiveValueProperty => PropertyMappings.SingleOrDefault(pm => pm.RepresentsValueElement);
+    public PropertyMapping? PrimitiveValueProperty => PropertyMappingsInternal.PrimitiveValueProperty;
 
     /// <summary>
     /// This indicates that this class is representing the Patient data (and implements <see cref="IPatient"/>).
@@ -225,7 +238,7 @@ PropertyMappingCollection createCollection()
     /// <summary>
     /// Whether the reflected type has a member that represent a primitive value.
     /// </summary>
-    public bool HasPrimitiveValueMember => PropertyMappings.Any(pm => pm.RepresentsValueElement);
+    public bool HasPrimitiveValueMember => PropertyMappingsInternal.HasPrimitiveValueMember;
 
     /// <summary>
     /// Returns the mapping for an element of this class by its name.
@@ -358,7 +371,7 @@ IReadOnlyCollection<IElementDefinitionSummary> IStructureDefinitionSummary.GetEl
     /// <remarks>If not set, the default constructor for the <see cref="NativeType"/> will be used.</remarks>
     public Base CreateInstance()
     {
-        var factory = LazyInitializer.EnsureInitialized(ref _factory, NativeType.BuildFactoryMethod)!;
+        var factory = _factory ?? LazyInitializer.EnsureInitialized(ref _factory, NativeType.BuildFactoryMethod)!;
         var newInstance = factory();
         if (newInstance is IDynamicType idt) idt.DynamicTypeName = Name;
         return (Base)newInstance;
@@ -372,7 +385,7 @@ public Base CreateInstance()
     /// <remarks>If not set, the default List constructor for the <see cref="NativeType"/> will be used.</remarks>
     public IList CreateList()
     {
-        var factory = LazyInitializer.EnsureInitialized(ref _listFactory, NativeType.BuildListFactoryMethod)!;
+        var factory = _listFactory ?? LazyInitializer.EnsureInitialized(ref _listFactory, NativeType.BuildListFactoryMethod)!;
         return factory();
     }
 

src/Hl7.Fhir.Base/Introspection/PropertyMapping.cs

@@ -192,14 +192,12 @@ private static ClassMapping findMappingOrThrow(ClassMapping declaringClass, stri
     /// <see cref="FhirType"/> it can distinguish between custom types that share the
     /// same (dynamic) .NET type. For mappings created by reflection, the list is resolved
     /// lazily from <see cref="FhirType"/>.</remarks>
-    public IReadOnlyList<ClassMapping> FhirTypeMappings
-    {
-        get
-        {
-            LazyInitializer.EnsureInitialized(ref _fhirTypeMappings, buildFhirTypeMappings);
-            return _fhirTypeMappings!;
-        }
-    }
+    public IReadOnlyList<ClassMapping> FhirTypeMappings =>
+        // GetInstantiableType() reads this for every element with an abstract property type while
+        // parsing, so LazyInitializer.EnsureInitialized() - and the factory delegate it allocates
+        // per call - is only reached when the field is still null; see the note on
+        // ClassMapping.PropertyMappingsInternal.
+        _fhirTypeMappings ?? LazyInitializer.EnsureInitialized(ref _fhirTypeMappings, buildFhirTypeMappings)!;
 
     private ClassMapping[]? _fhirTypeMappings;
 

src/Hl7.Fhir.Base/Introspection/PropertyMappingCollection.cs

@@ -47,6 +47,7 @@ private void clearCaches()
     {
         _byOrder = null;
         _choice = null;
+        _valueElements = null;
     }
 
     /// <summary>
@@ -74,8 +75,12 @@ public void Clear()
     public bool Remove(PropertyMapping item)
     {
         if (!_byName.TryRemove(item.Name, out _)) return false;
-        _byOrder?.Remove(item);
-        _choice?.Remove(item);
+
+        // Note: removing `item` from the cached lists is not enough - since the name dictionary is
+        // case-insensitive, the mapping just evicted may be a *different* instance that happens to
+        // share `item`'s name, which would leave that evicted mapping behind in the caches. Drop
+        // the caches instead, so they are rebuilt from the name dictionary on the next read.
+        clearCaches();
 
         return true;
     }
@@ -90,20 +95,51 @@ public bool Remove(PropertyMapping item)
     public IReadOnlyDictionary<string, PropertyMapping> ByName => _byName;
     private readonly ConcurrentDictionary<string, PropertyMapping> _byName = new(StringComparer.OrdinalIgnoreCase);
 
+    // The lazily computed lists below only call LazyInitializer.EnsureInitialized() when the field
+    // is still null, so that reading them on the warm path allocates nothing; see the note on
+    // ClassMapping.PropertyMappingsInternal for the reasoning. All three are dropped by
+    // clearCaches() when the collection changes.
+
     /// <summary>
     /// List of the properties, in the order of appearance.
     /// </summary>
-    public IReadOnlyList<PropertyMapping> ByOrder => LazyInitializer.EnsureInitialized(ref _byOrder,
-        () => ByName.Values.OrderBy(pm => pm.Order).ToList())!;
+    public IReadOnlyList<PropertyMapping> ByOrder =>
+        _byOrder ?? LazyInitializer.EnsureInitialized(ref _byOrder,
+            () => ByName.Values.OrderBy(pm => pm.Order).ToList())!;
+
     private List<PropertyMapping>? _byOrder;
 
     /// <summary>
     /// The list of properties that represent choice elements.
     /// </summary>
-    public IReadOnlyList<PropertyMapping> ChoiceProperties => LazyInitializer.EnsureInitialized(ref _choice,
-        () => ByName.Values.Where(pm => pm.Choice == ChoiceType.DatatypeChoice).ToList())!;
+    public IReadOnlyList<PropertyMapping> ChoiceProperties =>
+        _choice ?? LazyInitializer.EnsureInitialized(ref _choice,
+            () => ByName.Values.Where(pm => pm.Choice == ChoiceType.DatatypeChoice).ToList())!;
+
     private List<PropertyMapping>? _choice;
 
+    /// <summary>
+    /// The property that represents the value of a FHIR primitive, or <c>null</c> when this
+    /// collection has no such property.
+    /// </summary>
+    /// <remarks>Determining which properties are value elements requires a scan of the collection,
+    /// and this is read for every element encountered during (de)serialization, so the scan is done
+    /// once and its (near-always empty or single-entry) result cached. Picking the single value
+    /// element out of that cached result is what still happens per read - including the throw when
+    /// a malformed mapping declares more than one value element.</remarks>
+    public PropertyMapping? PrimitiveValueProperty => valueElements.SingleOrDefault();
+
+    /// <summary>
+    /// Whether this collection contains a property that represents the value of a FHIR primitive.
+    /// </summary>
+    public bool HasPrimitiveValueMember => valueElements.Count > 0;
+
+    private List<PropertyMapping> valueElements =>
+        _valueElements ?? LazyInitializer.EnsureInitialized(ref _valueElements,
+            () => ByName.Values.Where(pm => pm.RepresentsValueElement).ToList())!;
+
+    private List<PropertyMapping>? _valueElements;
+
     IEnumerator<PropertyMapping> IEnumerable<PropertyMapping>.GetEnumerator() => _byName.Values.GetEnumerator();
 
     IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)_byName.Values).GetEnumerator();