dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.37k stars 4.75k forks source link

System.Text.Json fails to build on .NET Native due to unconstrained raw pointer uses #84895

Closed Sergio0694 closed 1 year ago

Sergio0694 commented 1 year ago

Description

I'm dogfooding .NET 8 builds of System.Text.Json in the Microsoft Store, now that we've fully migrated to it, and noticed that since the .NET 8 Preview 1 release, .NET Native has starting failing to build. We've investigated this and @AntonLapounov managed to narrow this down to some uses of unconstrained raw pointers, which is a known issue with .NET Native.

Specifically, the build failure is caused by some changes in 4deccc02588990626180eaa812e63603d6c2790e:

https://github.com/dotnet/runtime/blob/8cb3bf89e4b28b66bf3b4e2957fd015bf925a787/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs#L135-L182

https://github.com/dotnet/runtime/blob/8cb3bf89e4b28b66bf3b4e2957fd015bf925a787/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs#L236-L262

This is a complete blocker for us, as we cannot update to System.Text.Json 8.0 at all.

Could we revert these changes in EnumConverter<T> and just keep using Unsafe.As<TFrom, TTo>? Since it's an intrinsic it wouldn't really have any performance impacts on .NET 8, and it'd at least fix the compile issues on .NET Native we're hitting.

cc. @eiriktsarpalis, also could we mark this as partner impact? We're completely stuck on 7.0 as of right now 🥲

Reproduction Steps

[JsonConverter(typeof(JsonStringEnumConverter))]
public enum MyEnum
{
    A,
    B,
    C
}

public sealed class MyClass
{
    public MyEnum Value { get; set; }
}

[JsonSerializable(typeof(MyClass))]
public sealed partial class MyContext : JsonSerializerContext
{
}

Compile in Release.

Expected behavior

The app should build correctly.

Actual behavior

The build fails with an error like this:

"Error ILT0005: 'C:\Users\sergiopedri.nuget\packages\runtime.win10-x64.microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\x64\ilc\Tools\nutc_driver.exe @"C:\Users\sergiopedri\source\repos\SystemTextJsonILCRepro\obj\x64\Release\ilc\intermediate\MDIL\SystemTextJsonILCRepro.rsp"' returned exit code -1073741819"

Specifically, the real issue is here:

"System.Text.Json.Serialization.Converters.EnumConverter`1[[System.UniversalCanon,System.Private.CoreLib]],System.Text.Json" "Read" /singleMethodParam "System.UniversalCanon,System.Private.CoreLib" /singleMethodParam "System.Text.Json.Utf8JsonReader&,System.Text.Json" /singleMethodParam "System.Type,System.Private.CoreLib" /singleMethodParam "System.Text.Json.JsonSerializerOptions,System.Text.Json""

Regression?

Yes, it works just fine with System.Text.Json 7.0.2.

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description I'm dogfooding .NET 8 builds of System.Text.Json in the Microsoft Store, now that we've fully migrated to it, and noticed that since the .NET 8 Preview 1 release, .NET Native has starting failing to build. We've investigated this and @AntonLapounov managed to narrow this down to some uses of unconstrained raw pointers, which is a known issue with .NET Native. Specifically, the build failure is caused by some changes in [4deccc02588990626180eaa812e63603d6c2790e](https://github.com/dotnet/runtime/commit/4deccc02588990626180eaa812e63603d6c2790e#diff-2fdb28f63c50c262d9ccdd1bea7e7ba9affaa934491b07c59f3ddc40a235ee4bL140): https://github.com/dotnet/runtime/blob/8cb3bf89e4b28b66bf3b4e2957fd015bf925a787/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs#L135-L182 https://github.com/dotnet/runtime/blob/8cb3bf89e4b28b66bf3b4e2957fd015bf925a787/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs#L236-L262 This is a **complete blocker** for us, as we cannot update to System.Text.Json 8.0 at all. Could we revert these changes in `EnumConverter` and just keep using `Unsafe.As`? Since it's an intrinsic it wouldn't really have any performance impacts on .NET 8, and it'd at least fix the compile issues on .NET Native we're hitting. cc. @eiriktsarpalis, also could we mark this as partner impact? We're completely stuck on 7.0 as of right now 🥲 ### Reproduction Steps - Create a blank UWP app - Reference System.Text.Json 8.0.0 Preview 1 or greater - Add this code ```csharp [JsonConverter(typeof(JsonStringEnumConverter))] public enum MyEnum { A, B, C } public sealed class MyClass { public MyEnum Value { get; set; } } [JsonSerializable(typeof(MyClass))] public sealed partial class MyContext : JsonSerializerContext { } ``` Compile in Release. ### Expected behavior The app should build correctly. ### Actual behavior The build fails with an error like this: > "Error ILT0005: 'C:\Users\sergiopedri\.nuget\packages\runtime.win10-x64.microsoft.net.native.compiler\2.2.12-rel-31116-00\tools\x64\ilc\Tools\nutc_driver.exe @"C:\Users\sergiopedri\source\repos\SystemTextJsonILCRepro\obj\x64\Release\ilc\intermediate\MDIL\SystemTextJsonILCRepro.rsp"' returned exit code -1073741819" Specifically, the real issue is here: > "System.Text.Json.Serialization.Converters.EnumConverter`1[[System.__UniversalCanon,System.Private.CoreLib]],System.Text.Json" "Read" /singleMethodParam "System.__UniversalCanon,System.Private.CoreLib" /singleMethodParam "System.Text.Json.Utf8JsonReader&,System.Text.Json" /singleMethodParam "System.Type,System.Private.CoreLib" /singleMethodParam "System.Text.Json.JsonSerializerOptions,System.Text.Json"" ### Regression? Yes, it works just fine with System.Text.Json 7.0.2. ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: Sergio0694
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
Sergio0694 commented 1 year ago

Thinking about this more, and since @tannergooding mentioned that the previous use of Unsafe.As<TFrom, TTo> had some concerns around additional generic instantiations, could we not just stop using pointers altogether and just box/unbox? The JIT/AOT compilers should recognize things anyway and elide the boxing anyway. That is, in both directions:

// Read
if (reader.TryGetInt32(out int int32))
{
    return (T)(object)int32;
}
// Write
writer.WriteNumberValue((int)(object)value);

This would both solve the issue on .NET Native and also potentially gives even better codegen due to no address taken?

EgorBo commented 1 year ago

(TEnum)(object)integer pattern won't work for .NET Framework and likely won't for Mono too, e.g.

https://sharplab.io/#v2:EYLgHgbALANALiAhgZwLYB8ACAmAjAWAChMAGAAk1wDoAlAVwDs4BLVAUyoGEB7VAB2YAbNgCcAyqIBuzAMZtkAbiJFMAZgrYynMgG8iZA2X2G1ZNgzqoyAWQCeAUQupjBvYUMeyAQRgAhGJwuZAC+QUGUEDYOTmQAKvJwABQAlEFuniYA7HHcjpYAPHZ5qAB8iVCqyUruhqGEQQDa1mxwABbcACYAkvyCic1tnT18ggDyfCzcDMhUXgDmcyLyyMySbF0MgswM23PJALrhuJGxOcX5sWXbcGSIyWQA7q2ibHFkIGTIcCJ0MnAwZGKaSCHkw2USsWSiW4wAAVmw/slENUPHUiMEgA

CoreCLR learned to fold it fairly recently (in net7)

Sergio0694 commented 1 year ago

"won't work for .NET Framework"

Just to clarify, do you mean it'll work slowly, and not that it won't work at all? Because ai tried this in the NETFX C# interactive window and it seems to work fine on NETFX too.

If performance on NETFX and Mono is an issue, it seems we could:

EgorBo commented 1 year ago

it'll work slowly

Yes, it boxes (allocates)

jkotas commented 1 year ago

it's an intrinsic it wouldn't really have any performance impacts on .NET 8

The generic intrinsic references contribute to static footprint that is also a performance metric. Reducing the static footprint was my motivation for the change.

Revert the Unsafe.As use assuming that's fine with respect to additional generics

I think it is fine to revert the change for System.Text.Json. System.Text.Json EnumConverter has large static footprint (e.g. it instantiates ConcurrentDictionary<string, T> - that is 10's kB of static footprint right there for each serialized enum type). A few extra generic instantiations from Unsafe.As are not a big deal.

Sergio0694 commented 1 year ago

"The generic intrinsic references contribute to static footprint that is also a performance metric. Reducing the static footprint was my motivation for the change."

Yup, no that makes perfect sense. I should've clarified, I only meant to revert those changes in EnumConverter<T> specifically, I do understand the motivation for the change in all other places in general to reduce generic instantiations 🙂


Was about to say feel free to assign me to this but I see you already have a PR up ahah awesome! 😄