1388 xml serializer assembly load context awareness #58932

StephenMolloy · 2021-09-10T11:07:21Z

Makes XmlSerializer aware of AssemblyLoadContexts

When serializing types that are in a different ALC, the serializer
should also operate in that same context. And not keep any
hard references that would prevent the context from being
unloaded.

Fixes #1388, #598, #599

src/libraries/System.Private.Xml/src/System/Xml/Serialization/Compilation.cs

src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs

src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs

mconnew · 2021-09-10T20:06:18Z

Each of the XmlMapping instances could be for a type in a different ALC. I think this is still going to break as it iterates through the types for each of the mappings and generates the serializer for them. If they are in a different ALC then type (which might be null), then it will throw. To fix this looks like it would require quite a bit of refactoring. Instead it would be worth detecting this situation and throwing an exception saying this isn't supported and all XmlMappings need to be for the same ALC.

Refers to: src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs:561 in 8146852. [](commit_id = 8146852e3ae0b9051720359291700d1e6e51a662, deletion_comment = False)

src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs

src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs

…on't keep any hard refs to types that could prevent unloading.

…refinement

…le ALCs.

mconnew · 2021-10-12T20:11:31Z

...ibraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs

-            if (!s_setMemberValueDelegateCache.TryGetValue(typeMemberNameTuple, out ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate? result))
+            Type type = o.GetType();
+            var delegateCacheForType = s_setMemberValueDelegateCache.GetOrCreateValue(type);
+            if (!delegateCacheForType.TryGetValue(memberName, out var result))


GetOrCreateValue uses Activator.CreateInstance to create an instance of TValue (ConcurrentDictionary in this case). There's another method you can use where you provide a callback which would be more efficient.

var delegateCacheForType = s_setMemberValueDelegateCache.GetValue(type, _ => return new ConcurrentDictionary<string, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>());

Also ConcurrentDictionary is very wasteful for space in it's defaults. The default capacity is 31 and default concurrency is the number of CPU's there are. It creates the same number of locks as the concurrency and spreads the data among that number of internal data stores. The likelyhood of there being multiple threads trying to write to this concurrent dictionary is very low, and that will only happen the first time serializing a particular type. I have 2 suggestions.

Use the GetValue overload above and use the ConcurrentDictionary constructor which takes concurrencyLevel and capacity. I would set capacity to 31 (seems reasonable for number of properties on a type) and concurrencyLevel to 2. It does mean if you get 3 threads trying to add to the dictionary at once, one of them would block. But that would require 3 threads trying to use a serializer for the first time each serializing the same type at once, and would only happen once.

Switch to using a Hashtable and use a lock around all writes.

Also this is going to regress performance when using the reflection based serializer. I would suggest having two collections, one for the default ALC, and one for everything else. The extra cost would be on the creation, but lookup for the default case will remain exactly the same.

…ain test assembly.

mconnew · 2021-10-27T23:21:49Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/Compilation.cs

+                // Before generating any IL, check each mapping and supported type to make sure
+                // they are compatible with the current ALC
+                for (int i = 0; i < types.Length; i++)
+                    VerifyLoadContext(types[i], mainAssembly);


Why is the first iterator a traditional for loop and the second a foreach? Inconsistent coding style.

mconnew · 2021-10-27T23:24:37Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/Compilation.cs

+                // they are compatible with the current ALC
+                for (int i = 0; i < types.Length; i++)
+                    VerifyLoadContext(types[i], mainAssembly);
+                foreach (var mapping in xmlMappings)


VerifyLoadContext returns if mainAssembly is null. Could you add that check around the loop and skip it in VerifyLoadContext?

mconnew · 2021-10-27T23:26:37Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/Compilation.cs

+
+            // Collectible types should be in the same collectible context
+            var baseALC = AssemblyLoadContext.GetLoadContext(assembly) ?? AssemblyLoadContext.CurrentContextualReflectionContext;
+            if (typeALC != baseALC)


This is being done for every type. Can you lift this code out of the method so it's done once?

mconnew · 2021-10-27T23:40:27Z

...ibraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs

@@ -627,40 +627,48 @@ private void WriteMembers(ref object? o, Member[] members, UnknownNodeAction ele
            }
        }

-        private static readonly ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate> s_setMemberValueDelegateCache = new ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>();
+        private static readonly ConditionalWeakTable<Type, Hashtable> s_setMemberValueDelegateCache = new ConditionalWeakTable<Type, Hashtable>();



You are still using a ConditionalWeakTable for lookups even when using the default ALC. This will regress a lot as every single property set will hit this method and based on looking at the code for CWT, it's going to be expensive. This is used on a hot code path. You need to do the double table approach for this too.

mconnew · 2021-10-27T23:54:45Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs


        // SxS: This method does not take any resource name and does not expose any resources to the caller.
        // It's OK to suppress the SxS warning.
        internal static bool IsTypeDynamic(Type type)
        {
-            object? oIsTypeDynamic = s_tableIsTypeDynamic[type];
+            s_tableIsTypeDynamic.TryGetValue(type, out object? oIsTypeDynamic);
            if (oIsTypeDynamic == null)


I'm confident this isn't on a hot code path for RefEmit serializer, but what about the reflection based serializer? Does this get called for every object that's being serialized or is it an initialization only usage? If the former, then this needs to do the dual table route.

…ewriting similar code everywhere.

mconnew · 2021-10-28T20:00:47Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/ContextAwareTables.cs

+        {
+            T? ret;
+            AssemblyLoadContext? alc = AssemblyLoadContext.GetLoadContext(t.Assembly);
+


How expensive is ALC.GetLoadContext? If it's non-trivial, it might be worth always checking the default table first and only using GetLoadContext to see which table is should go in if not found in the default. If it's cheap, then this is fine.

mconnew · 2021-10-28T21:27:25Z

src/libraries/System.Private.Xml/src/System/Xml/Serialization/ContextAwareTables.cs

+namespace System.Xml.Serialization
+{
+    using System;
+    using System.Collections;


Usings should be outside of namespace

mconnew

…ness

StephenMolloy · 2021-11-05T21:28:03Z

/backport to release/6.0

github-actions · 2021-11-05T21:28:14Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1427474672

StephenMolloy added the area-Serialization label Sep 10, 2021

StephenMolloy added this to the 6.0.0 milestone Sep 10, 2021

StephenMolloy requested review from mconnew and HongGit September 10, 2021 11:07

stephentoub reviewed Sep 10, 2021

View reviewed changes

src/libraries/System.Private.Xml/src/System/Xml/Serialization/Compilation.cs Outdated Show resolved Hide resolved