-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1388 xml serializer assembly load context awareness #58932
Changes from 12 commits
4647d16
55eb88b
d9e96dc
b6f110a
8616e29
88778c4
15b7517
2c82165
da3c8c1
0d37491
6ab0bcd
f160c1b
454a9fe
d361830
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace System.Xml.Serialization | ||
{ | ||
using System; | ||
using System.Collections; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Runtime.CompilerServices; | ||
using System.Runtime.Loader; | ||
|
||
internal class ContextAwareTables<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]T> where T : class? | ||
{ | ||
private Hashtable _defaultTable; | ||
private ConditionalWeakTable<Type, T> _collectibleTable; | ||
|
||
public ContextAwareTables() | ||
{ | ||
_defaultTable = new Hashtable(); | ||
_collectibleTable = new ConditionalWeakTable<Type, T>(); | ||
} | ||
|
||
internal T GetOrCreateValue(Type t, Func<T> f) | ||
{ | ||
T? ret; | ||
AssemblyLoadContext? alc = AssemblyLoadContext.GetLoadContext(t.Assembly); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// Null and non-collectible load contexts use the default table | ||
if (alc == null || !alc.IsCollectible) | ||
{ | ||
if ((ret = (T?)_defaultTable[t]) == null) | ||
{ | ||
lock (_defaultTable) | ||
{ | ||
if ((ret = (T?)_defaultTable[t]) == null) | ||
{ | ||
ret = f(); | ||
_defaultTable[t] = ret; | ||
} | ||
} | ||
} | ||
} | ||
|
||
// Collectible load contexts should use the ConditionalWeakTable so they can be unloaded | ||
else | ||
{ | ||
if (!_collectibleTable.TryGetValue(t, out ret)) | ||
{ | ||
lock (_collectibleTable) | ||
{ | ||
if (!_collectibleTable.TryGetValue(t, out ret)) | ||
{ | ||
ret = f(); | ||
_collectibleTable.AddOrUpdate(t, ret); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return ret; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -627,40 +627,48 @@ private static void AddObjectsIntoTargetCollection(object targetCollection, List | |
} | ||
} | ||
|
||
private static readonly ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate> s_setMemberValueDelegateCache = new ConcurrentDictionary<(Type, string), ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>(); | ||
private static readonly ContextAwareTables<Hashtable> s_setMemberValueDelegateCache = new ContextAwareTables<Hashtable>(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
[RequiresUnreferencedCode(XmlSerializer.TrimSerializationWarning)] | ||
private static ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate GetSetMemberValueDelegate(object o, string memberName) | ||
{ | ||
Debug.Assert(o != null, "Object o should not be null"); | ||
Debug.Assert(!string.IsNullOrEmpty(memberName), "memberName must have a value"); | ||
(Type, string) typeMemberNameTuple = (o.GetType(), memberName); | ||
if (!s_setMemberValueDelegateCache.TryGetValue(typeMemberNameTuple, out ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate? result)) | ||
Type type = o.GetType(); | ||
var delegateCacheForType = s_setMemberValueDelegateCache.GetOrCreateValue(type, () => new Hashtable()); | ||
var result = delegateCacheForType[memberName]; | ||
if (result == null) | ||
{ | ||
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName); | ||
Debug.Assert(memberInfo != null, "memberInfo could not be retrieved"); | ||
Type memberType; | ||
if (memberInfo is PropertyInfo propInfo) | ||
{ | ||
memberType = propInfo.PropertyType; | ||
} | ||
else if (memberInfo is FieldInfo fieldInfo) | ||
{ | ||
memberType = fieldInfo.FieldType; | ||
} | ||
else | ||
lock (delegateCacheForType) | ||
{ | ||
throw new InvalidOperationException(SR.XmlInternalError); | ||
} | ||
if ((result = delegateCacheForType[memberName]) == null) | ||
{ | ||
MemberInfo memberInfo = ReflectionXmlSerializationHelper.GetEffectiveSetInfo(o.GetType(), memberName); | ||
Debug.Assert(memberInfo != null, "memberInfo could not be retrieved"); | ||
Type memberType; | ||
if (memberInfo is PropertyInfo propInfo) | ||
{ | ||
memberType = propInfo.PropertyType; | ||
} | ||
else if (memberInfo is FieldInfo fieldInfo) | ||
{ | ||
memberType = fieldInfo.FieldType; | ||
} | ||
else | ||
{ | ||
throw new InvalidOperationException(SR.XmlInternalError); | ||
} | ||
|
||
MethodInfo getSetMemberValueDelegateWithTypeGenericMi = typeof(ReflectionXmlSerializationReaderHelper).GetMethod("GetSetMemberValueDelegateWithType", BindingFlags.Static | BindingFlags.Public)!; | ||
MethodInfo getSetMemberValueDelegateWithTypeMi = getSetMemberValueDelegateWithTypeGenericMi.MakeGenericMethod(o.GetType(), memberType); | ||
var getSetMemberValueDelegateWithType = (Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)getSetMemberValueDelegateWithTypeMi.CreateDelegate(typeof(Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)); | ||
result = getSetMemberValueDelegateWithType(memberInfo); | ||
s_setMemberValueDelegateCache.TryAdd(typeMemberNameTuple, result); | ||
MethodInfo getSetMemberValueDelegateWithTypeGenericMi = typeof(ReflectionXmlSerializationReaderHelper).GetMethod("GetSetMemberValueDelegateWithType", BindingFlags.Static | BindingFlags.Public)!; | ||
MethodInfo getSetMemberValueDelegateWithTypeMi = getSetMemberValueDelegateWithTypeGenericMi.MakeGenericMethod(o.GetType(), memberType); | ||
var getSetMemberValueDelegateWithType = (Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)getSetMemberValueDelegateWithTypeMi.CreateDelegate(typeof(Func<MemberInfo, ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate>)); | ||
result = getSetMemberValueDelegateWithType(memberInfo); | ||
delegateCacheForType[memberName] = result; | ||
} | ||
} | ||
} | ||
|
||
return result; | ||
return (ReflectionXmlSerializationReaderHelper.SetMemberValueDelegate)result; | ||
} | ||
|
||
private object? GetMemberValue(object o, MemberInfo memberInfo) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usings should be outside of namespace