Skip to content
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

System.Text.Json Serializer does not appear to work on Xamarin iOS #31326

Closed
jefffhaynes opened this issue Oct 28, 2019 · 34 comments
Closed

System.Text.Json Serializer does not appear to work on Xamarin iOS #31326

jefffhaynes opened this issue Oct 28, 2019 · 34 comments
Assignees
Milestone

Comments

@jefffhaynes
Copy link

Perhaps not surprisingly, System.Text.Json.Serializer does not appear to function on iOS likely owing to the lack of Emit support.

public class ChatMessage
{
    [JsonPropertyName("id")]
    public string Id { get; set; }

    [JsonPropertyName("sent")]
    public DateTime? SentOn { get; set; }
}

JsonSerializer.Deserialize<ChatMessage>(json);

This yields:

System.ExecutionEngineException: Attempting to JIT compile method '(wrapper delegate-invoke) void :invoke_callvirt_void_ChatMessage_Nullable1<DateTime> (Chat.Messaging.ChatMessage,System.Nullable1<System.DateTime>)' while running in aot-only mode. See https://docs.microsoft.com/xamarin/ios/internals/limitations for more information.

@ericstj
Copy link
Member

ericstj commented Oct 28, 2019

/cc @steveharter

@steveharter
Copy link
Member

FWIW there is a fallback today to just use standard reflection which is currently enabled in source-only builds (not the "inbox" builds).

@jefffhaynes
Copy link
Author

No worries I switched back to newtonsoft for now. I'll switch back once it's ready. Thanks!

@sfedotovs
Copy link

have the same issue with Json Serializer on Xamarin.iOS.

@steveharter can you elaborate how your note helps in this situation?
I found the place in JsonSerializerOptions.cs

#if BUILDING_INBOX_LIBRARY
                    _memberAccessorStrategy = new ReflectionEmitMemberAccessor();
#else
                    // todo: should we attempt to detect here, or at least have a #define like #SUPPORTS_IL_EMIT
                    _memberAccessorStrategy = new ReflectionMemberAccessor();
#endif

I use nuget package, it should resolve to ReflectionMemberAccessor but the issue still exists. I believe it is not related to Emit, but some other place where JIT is used (Delegates?).

Is there a hope to get fix for the issue in nearest future (at least in preview)? Or should we consider to choose another JsonSerializer to use on Xamarin iOS?

@jkotas
Copy link
Member

jkotas commented Oct 31, 2019

some other place where JIT is used (Delegates?).

Yes, there are number of other constructs that System.Text.Json uses that are not compatible with full AOT compilation. For example, MakeGenericType is not compatible with full AOT compilation. Some of the delegate constructs that you have mentioned are not friendly to full AOT compilation either.

Is there a hope to get fix for the issue in nearest future (at least in preview)?

Unlikely. Making System.Text.Json compatible with full AOT compilation will require fundamental changes. I believe that the best fix is going to be to pre-generate the serializers at build time:
https://github.com/dotnet/corefx/issues/41398

cc @marek-safar

@klogeaage
Copy link

So shouldn't the dependencies of the NuGet package reflect that it simply should not be attempted on Xamarin iOS? I actually found this surprising and have now wasted several hours moving from Json.Net to System.Text.Json because I liked the idea of having the same serializer in both front-end and the .Net Core 3.0 back-end. And because of the performance improvements that it should give.

Another surprise was that it works on the Xamarin iOS simulator, but not on the real device.

So this was a dead-end and that would have been nice to know beforehand.

@huoyaoyuan
Copy link
Member

Interesting that I can make System.Text.Json working on UWP .Net Native after adding some linker instructions.

@sasivishnu
Copy link
Contributor

sasivishnu commented Nov 27, 2019

So shouldn't the dependencies of the NuGet package reflect that it simply should not be attempted on Xamarin iOS? I actually found this surprising and have now wasted several hours moving from Json.Net to System.Text.Json because I liked the idea of having the same serializer in both front-end and the .Net Core 3.0 back-end. And because of the performance improvements that it should give.

Another surprise was that it works on the Xamarin iOS simulator, but not on the real device.

So this was a dead-end and that would have been nice to know beforehand.

Yes, It will be nice to mention not to use this package where AOT is enabled? We too switched using NewtonSoft Json for our company xamarin mobile app project for now.

@PaulVrugt
Copy link

what the hell? So this package isn't compatible with iOS at all?

@klogeaage
Copy link

klogeaage commented Dec 23, 2019

Nope - very surprising, to say the least. And what is even worse: you don't discover this until you spent time on implementing it, then get the weird error message when running on a real iOS device, then search on Google and then find this issue. Not a good developer experience at all...

@PaulVrugt
Copy link

We even released a version of our app, because we didn't find anything wrong, because "most" cases simply work. We now have to create a new version and revert to newtonsoft.json because of all of the functionality in our app about 2 functions are not working.

@klogeaage
Copy link

Interesting. I was lucky then that my app didn't even start correctly.

@danmoseley
Copy link
Member

Yes, It will be nice to mention not to use this package where AOT is enabled? We too switched using NewtonSoft Json for our company xamarin mobile app project for now.

Note the package includes functionality that does not care about AOT eg the readers/writers.

@eman1986
Copy link

eman1986 commented Feb 18, 2020

Are there plans to get this working for iOS? My team just upgraded our api website to NETCore 3.1 and we switched out to the System.Text.Json

@steveharter
Copy link
Member

steveharter commented Apr 28, 2020

Are there plans to get this working for iOS?

We are working on enabling "reach" scenarios for 5.0, but nothing is committed yet.

We've made some serializer changes in 5.0 that appear to help with its usage of Type.MakeGenericType -- I verified that a simple iOS device test that used to fail on 3.1 is now working on 5.0 master. When we get our test infrastructure to run all serializer tests on an iOS device then we will know the actual status and what work is remaining.

@mc0re
Copy link

mc0re commented Jun 1, 2020

Interesting that I can make System.Text.Json working on UWP .Net Native after adding some linker instructions.

@huoyaoyuan What instructions?

@huoyaoyuan
Copy link
Member

@mc0re

<Assembly Name="System.Collections.Immutable" Dynamic="Required Public" />

It only solves part of the problem. Not fully working.

@mc0re
Copy link

mc0re commented Jun 2, 2020

I did this:

<Namespace Name="System.Text.Json.Serialization.Converters" Browse="Required All"/>

Seems to work. I know that "All" is probably an overkill, but I didn't want to waste the time trying to figure out the minimal requirement :-)

@steveharter
Copy link
Member

Let's try to keep this issue just for Xamarin and not .NET Native since there are separate issues.

The .NET Native issues are caused by its linker removing "unreferenced" assemblies.

@Jacko1394
Copy link

Adding the --interpreter flag to the additional Mtouch args seems to fix the problem for me.
<MtouchExtraArgs>--interpreter</MtouchExtraArgs>
Tho I agree annoying it just doesn't work...

@steveharter
Copy link
Member

steveharter commented Aug 4, 2020

Adding the --interpreter flag to the additional Mtouch args seems to fix the problem for me.

@Jacko1394 is this still on 3.1? Have you been able to try this on any 5.0 previews?

@steveharter
Copy link
Member

Closing this as fixed for 5.0.

If this repros still on 5.0, please re-open.

@klogeaage
Copy link

klogeaage commented Aug 15, 2020

@steveharter I tried the latest preview package, 5.0.0-preview.7.20364.11, but it completely failed to deserialize the JSON received from my .Net Core 3.1 backend. This is the more surprising, as the backend uses System.Text.Json. It gave no errors, just returned objects with no sensible data in them.

This is the code with the working Newtonsoft statements commented out:

  HttpResponseMessage response = await _client.GetAsync($"{ControllerName}{additionalParameters}");
  response.EnsureSuccessStatusCode();
  using var stream = await response.Content.ReadAsStreamAsync();
  //using (var reader = new StreamReader(stream))
  //using (var json = new JsonTextReader(reader))
  //    rv = _serializer.Deserialize<T1>(json);
  rv = await System.Text.Json.JsonSerializer.DeserializeAsync<T1>(stream);
  return rv;

Would you be interested in digging into what the problem is? Then I would be happy to continue testing.

@mc0re
Copy link

mc0re commented Aug 15, 2020

@klogeaage I might be off-track here, but check the serialization options you use in the 3.1 serializer.
For what I recall, Newtonsoft (de)serializes into camelCase by default, while System.Text.Json - into PascalCase. There is also different treatment of numbers - Newtonsoft happily accepts "10" as a numeric field, while System.Text.Json does not (it complains about the quotes and expects a bare 10).

In short, check the incoming text and if you're using any options for serialization.

@steveharter
Copy link
Member

Due to Ios policy, Reflection.Emit is not supported. The netstandard version of System.Text.Json needs to be used which uses standard reflection instead of Emit.

@klogeaage
Copy link

@mc0re thanks for your comments, which led me to the solution: setting PropertyNamingPolicy = System.Text.Json.JsonNamingPolicy.CamelCase on the front-end. I don't set this on the back-end, but apparently Asp.NET Core does, which I guess makes sense as it results in more idiomatic JSON.

@steveharter so with the above change, I was able to get it working also on a physical iOS device, which is what didn't work previously. So this issue can remain closed as far as I'm concerned.

However, I found that there is no performance improvement to be gained in moving from Newtonsoft (using the JsonTextReader pattern and avoiding strings as shown above) - on the contrary, I found a degradation in deserialization performance of between 32% (Android) and 20% (iOS), measured on physical devices as an average of 10 operations deserializing a 523 KB JSON payload.

While this is not a disastrous performance hit, it is contrary to the claim that System.Text.Json will have better performance than Newtonsoft.Json and it once again shows that Newtonsoft.Json is a tough act to follow.

@ericstj
Copy link
Member

ericstj commented Aug 19, 2020

That seems a significant per difference and disagrees with most of our benchmarks. Share a full benchmark in a new issue and we’ll investigate. We stand by our perf claims. It’s possible there is an opportunity for improvement here in STJ or mono runtime.

@klogeaage
Copy link

@ericstj I'm glad you take this seriously and have created a new issue as requested: #41089.

@steveharter
Copy link
Member

steveharter commented Aug 24, 2020

However, I found that there is no performance improvement to be gained in moving from Newtonsoft (using the JsonTextReader pattern and avoiding strings as shown above) - on the contrary, I found a degradation in deserialization performance of between 32% (Android) and 20% (iOS), measured on physical devices as an average of 10 operations deserializing a 523 KB JSON payload.

The likely reason perf is slower in STJ is in these cases is because STJ uses standard reflection Invoke, not Emit, for the netstandard (non-inbox) configurations. Newtonsoft instead uses compiled expressions trees which are netstandard-compatible but also brings in the very large System.Linq.Expressions.dll so there a disk_size+memory_usage+startup_time vs throughput tradeoff here.

It is possible to add support for S.Linq.Expressions if we decide it's worth the tradeoff in these non-inbox scenarios. However, for 6.0 we are also looking at alternate ways to get\set properties and call constructors that do not use Emit but are just as fast, plus we plan on having a code-gen solution that will generate AOT code per POCO type to get\set properties and call ctors that avoids the need for Emit\Reflection entirely.

@klogeaage
Copy link

@steveharter may I suggest you post your comments in #41089 which I created for the performance issue?

@steveharter
Copy link
Member

@klogeaage yes thanks.

@jefffhaynes
Copy link
Author

You could try "magic methods" (maybe that is what you're trying), originally from Skeet:

https://github.com/jefffhaynes/BinarySerializer/blob/master/BinarySerializer/MagicMethods.cs

@steveharter
Copy link
Member

steveharter commented Aug 24, 2020

You could try "magic methods" (maybe that is what you're trying), originally from Skeet:

That exact example will box, however in 3.0\3.1 we did use a similar approach for performance (instead of reflection invoke). However, it required tricky usage of generics to avoid boxing, which is exactly the issue that caused Xamarin to not work (multiple generic parameter types on "PropertyInfo" classes with generic constraints + inheritance of "PropertyInfo" types).

@jefffhaynes
Copy link
Author

Ah, fair point!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests