Fix for "System.NotImplementedException: byref delegate"#1461
Fix for "System.NotImplementedException: byref delegate"#1461AArnott merged 7 commits intoMessagePack-CSharp:masterfrom
Conversation
…nq.Expressions for AOT compilation.
…s commit by some unknown accident).
|
|
||
| #if !DYNAMICCODEDUMPER | ||
| #if DYNAMICCODEDUMPER | ||
| static readonly MessagePackSerializerOptions _standard = new MessagePackSerializerOptions(Resolvers.BuiltinResolver.Instance); |
There was a problem hiding this comment.
FYI: This code fails on CI
There was a problem hiding this comment.
Yes, I saw it. Do you know why?
It builds ok on local PC.
There was a problem hiding this comment.
Try something like
dotnet build --configuration Release /t:build,test,pack /m /v:m
from root folder of the repository. I've made a PR to your repository with couple fixes
There was a problem hiding this comment.
Main goal to add code lines in this place is to have Standard MessagePackSerializerOptions when projects are build with DYNAMICCODEDUMPER symbol.
I am using this Standard static property for example to init MessagePack like this:
public static class MessagePackInitiator
{
static Boolean _resolverWasRegistered = false;
public static void Run(MessagePackHubProtocolOptions options)
{
if (!_resolverWasRegistered)
{
StaticCompositeResolver.Instance.Register(
GeneratedResolver.Instance,
BuiltinResolver.Instance);
_resolverWasRegistered = true;
}
options.SerializerOptions = MessagePackSerializerOptions.Standard
.WithResolver(StaticCompositeResolver.Instance)
.WithSecurity(MessagePackSecurity.UntrustedData);
}
}
But it appeared that it is not so easy to add this property here, because DynamicCodeDumper project has not some files.
Do not want to add missed files. Maybe MessagePack authours will do it.
Instead just deleted my changes.
And instead will init MessagePack like this in my code:
public static class MessagePackInitiator
{
static Boolean _resolverWasRegistered = false;
public static void Run(MessagePackHubProtocolOptions options)
{
if (!_resolverWasRegistered)
{
StaticCompositeResolver.Instance.Register(
GeneratedResolver.Instance,
BuiltinResolver.Instance);
_resolverWasRegistered = true;
}
options.SerializerOptions = new MessagePackSerializerOptions(Resolvers.BuiltinResolver.Instance)
.WithResolver(StaticCompositeResolver.Instance)
.WithSecurity(MessagePackSecurity.UntrustedData);
}
}
| { | ||
| // public static void Serialize<T>(ref MessagePackWriter writer, T obj, MessagePackSerializerOptions options) | ||
| MethodInfo serialize = GetMethod(nameof(Serialize), type, new Type[] { typeof(MessagePackWriter).MakeByRefType(), null, typeof(MessagePackSerializerOptions) }); | ||
| MethodInfo serialize = GetMethod(nameof(SerializeSemiGeneric), type, new Type[] { typeof(MessagePackWriter).MakeByRefType(), typeof(Object), typeof(MessagePackSerializerOptions) }); |
There was a problem hiding this comment.
Is there any performance penalty in SemiGeneric? Should it be only used under ENABLE_IL2CPP then, where is no other option?
There was a problem hiding this comment.
From my point of view it should not be any perfomance penalty.
If you check SerializeSemiGeneric and DeserializeSemiGeneric methods -- they are just wrappers around current implementation with assitional casting. Do not think that casting will add any significatn penalty.
But I cannot garantee it.
Do you have any benchmarks? Can you compare two implementations?
There was a problem hiding this comment.
It seems that the delegate creates takes an object as command line argument anyway, so there is no penalty there. But calling Serialize/Deserialize now introduces unnecessary boxing for value types. Is it executed anywhere in a hot path and critical?
There was a problem hiding this comment.
I changed a bit code.
Now SerializeSemiGeneric and DeserializeSemiGeneric are using only inside MessagePackSerializer.NonGeneric.cs file.
So it decreases cases where additional casting is using.
Do not know how methods from MessagePackSerializer.NonGeneric.cs are used in MessagePack (I am not MessagePack author).
So cannot answer on your question "Is it executed anywhere in a hot path and critical?"
There was a problem hiding this comment.
RE: ENABLE_IL2CPP
I do not know what ENABLE_IL2CPP is for.
When this derective is using? What is advantages/disadvantages to use this option?
And I guess MessagePack nuget package will be build without this option, will not it?
There was a problem hiding this comment.
The ENABLE_IL2CPP is for Unity3D game engine where ahead of time compilation renders Emit code generation unusable.
There was a problem hiding this comment.
I think making the change only on unity wouldn't fix the original issue, which was an app running on iOS.
AArnott
left a comment
There was a problem hiding this comment.
Please merge LineSmarts#3. Then I think we can take this. @neuecc should review as well.
src/MessagePack.UnityClient/Assets/Scripts/MessagePack/MessagePackSerializer.NonGeneric.cs
Show resolved
Hide resolved
src/MessagePack.UnityClient/Assets/Scripts/MessagePack/Resolvers/DynamicObjectResolver.cs
Show resolved
Hide resolved
Revise the fix slightly
Done. |
|
I introduced a new pair of non-generic perf tests to PerfBenchmarkDotNet (#1478) and measured before and after this change. The perf impact was negligible for serialize. The perf got a little slower and more noisy for deserialize, but I suspect it may be all noise. For the tables below, focus on the Before change
After change
|
Fix for "System.NotImplementedException: byref delegate" in System.Linq.Expressions for AOT compilation.
It is second attempt. First commit had accidently corrupted code lines.
See details here: #1344 (comment)