Optimize RPC target reflection #596
Conversation
| internal static MethodNameMap GetMethodNameMap(TypeInfo type) | ||
| { | ||
| MethodNameMap? map; | ||
| lock (MethodNameMaps) |
There was a problem hiding this comment.
I assume this means gains by reduced GC pressure outweighs the overhead of the lock?
There was a problem hiding this comment.
Oh yes, very much so. 18% faster, remember. 😏
And ya, the reflection that we skip here is expensive in terms of GC pressure. Almost every API we call creates arrays, even if we just wanted one object. Even GetCustomAttribute calls another API that returns an array before returning a single element from it.
There was a problem hiding this comment.
Also note, these are all optimistic locking. We take it just long enough to peek, then we release the lock to do the work. As a result, this lock is not held for very long at all => minimal lock contention.
| /// </param> | ||
| internal JsonRpcMethodAttribute? GetJsonRpcMethodAttribute(string methodName, ReadOnlySpan<ParameterInfo> parameters) | ||
| { | ||
| if (this.targetRequestMethodToClrMethodMap.TryGetValue(methodName, out List<MethodSignatureAndTarget>? existingList)) |
There was a problem hiding this comment.
Do we need locks here or can a local rpc target object be added only before calling these?
There was a problem hiding this comment.
Oooh. good call. Accessing this dictionary without a lock would be dangerous. I'm adding the lock now.
| { | ||
| private const string ImpliedMethodNameAsyncSuffix = "Async"; | ||
| private static readonly Dictionary<TypeInfo, MethodNameMap> MethodNameMaps = new Dictionary<TypeInfo, MethodNameMap>(); | ||
| private static readonly Dictionary<(TypeInfo Type, bool AllowNonPublicInvocation, bool UseSingleObjectParameterDeserialization), Dictionary<string, List<MethodSignature>>> RequestMethodToClrMethodMap = new Dictionary<(TypeInfo Type, bool AllowNonPublicInvocation, bool UseSingleObjectParameterDeserialization), Dictionary<string, List<MethodSignature>>>(); |
There was a problem hiding this comment.
Dictionary<(TypeInfo Type, bool AllowNonPublicInvocation, bool UseSingleObjectParameterDeserialization), Dictionary<string, List>> [](start = 214, length = 147)
new()?
There was a problem hiding this comment.
Love it. Just as soon as C# 9 hits GA. 😉
| /// <summary> | ||
| /// A collection of target objects and their map of clr method to <see cref="JsonRpcMethodAttribute"/> values. | ||
| /// </summary> | ||
| private readonly Dictionary<string, List<MethodSignatureAndTarget>> targetRequestMethodToClrMethodMap = new Dictionary<string, List<MethodSignatureAndTarget>>(StringComparer.Ordinal); |
There was a problem hiding this comment.
Dictionary<string, List> [](start = 116, length = 50)
new(StringComparer.Ordinal)
There was a problem hiding this comment.
When C# 9 is GA, certainly. :)
| { | ||
| if (exceptions is null) | ||
| { | ||
| exceptions = new List<Exception>(); |
There was a problem hiding this comment.
exceptions [](start = 28, length = 10)
Can be simplified to exceptions ??= new List<Exception>();
There was a problem hiding this comment.
And it works in C# 8. Nice. :)
There was a problem hiding this comment.
Cache the results of reflection based on the type and options so subsequent connections can be made faster and with less GC pressure. Also move all the RPC target handling from JsonRpc to its own class.
AArnott
force-pushed
the
targetObjectPerf
branch
from
6a42432 to
8dbf362
Compare
Cache the results of reflection based on the type and options so subsequent connections can be made faster and with less GC pressure.
Also move all the RPC target handling from JsonRpc to its own class.
This makes a short-lived "use once" JsonRpc connection 18% faster and cuts out some large allocations.
Related to #575