Skip to content

Fix for "System.NotImplementedException: byref delegate"#1461

Merged
AArnott merged 7 commits intoMessagePack-CSharp:masterfrom
LineSmarts:master
Aug 2, 2022
Merged

Fix for "System.NotImplementedException: byref delegate"#1461
AArnott merged 7 commits intoMessagePack-CSharp:masterfrom
LineSmarts:master

Conversation

@MaximMikhisor
Copy link
Contributor

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)

@MaximMikhisor MaximMikhisor changed the title Fix for "System.NotImplementedException: byref delegate" (asecond attempt) Fix for "System.NotImplementedException: byref delegate" (second attempt) Jun 28, 2022

#if !DYNAMICCODEDUMPER
#if DYNAMICCODEDUMPER
static readonly MessagePackSerializerOptions _standard = new MessagePackSerializerOptions(Resolvers.BuiltinResolver.Instance);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This code fails on CI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw it. Do you know why?
It builds ok on local PC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any performance penalty in SemiGeneric? Should it be only used under ENABLE_IL2CPP then, where is no other option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ENABLE_IL2CPP is for Unity3D game engine where ahead of time compilation renders Emit code generation unusable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think making the change only on unity wouldn't fix the original issue, which was an app running on iOS.

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge LineSmarts#3. Then I think we can take this. @neuecc should review as well.

@AArnott AArnott requested a review from neuecc July 23, 2022 14:06
Revise the fix slightly
@MaximMikhisor
Copy link
Contributor Author

Please merge LineSmarts#3. Then I think we can take this. @neuecc should review as well.

Done.

@AArnott
Copy link
Collaborator

AArnott commented Aug 2, 2022

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 IntKey_NonGeneric benchmark.

Before change

Method Job Toolchain IterationCount LaunchCount WarmupCount Mean Error StdDev Ratio RatioSD Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
IntKey InProcess InProcessToolchain Default Default Default 239.7 ns 2.575 ns 2.409 ns 1.00 0.00 0.0086 - - 56 B
IntKey_NonGeneric InProcess InProcessToolchain Default Default Default 256.3 ns 1.293 ns 1.080 ns 1.07 0.01 0.0086 - - 56 B

After change

Method Job Toolchain IterationCount LaunchCount WarmupCount Mean Error StdDev Ratio RatioSD Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
IntKey InProcess InProcessToolchain Default Default Default 242.9 ns 1.789 ns 1.585 ns 1.00 0.00 0.0086 - - 56 B
IntKey_NonGeneric InProcess InProcessToolchain Default Default Default 281.9 ns 5.782 ns 11.141 ns 1.19 0.05 0.0086 - - 56 B

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Xamarin forms - iOS Deserialize throws System.NotImplementedException: byref delegate

3 participants