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

Attempting to JIT compile method in System.Text.Json tests on iOS devices #58204

Closed
akoeplinger opened this issue Aug 26, 2021 · 12 comments · Fixed by #59182
Closed

Attempting to JIT compile method in System.Text.Json tests on iOS devices #58204

akoeplinger opened this issue Aug 26, 2021 · 12 comments · Fixed by #59182
Assignees
Labels
area-Codegen-AOT-mono disabled-test The test is disabled in source code against the issue
Milestone

Comments

@akoeplinger
Copy link
Member

When running the System.Text.Json tests on iOS arm64 devices with full AOT we're seeing the following methods where it attempts to JIT compile them, causing an ExecutionEngineException:

bool System.Text.Json.Serialization.Converters.IAsyncEnumerableOfTConverter`2<System.Collections.Generic.IAsyncEnumerable`1<int>, int>:OnWriteResume (System.Text.Json.Utf8JsonWriter,System.Collections.Generic.IAsyncEnumerable`1<int>,System.Text.Json.JsonSerializerOptions,System.Text.Json.WriteStack&)
bool System.Text.Json.Serialization.Converters.IAsyncEnumerableOfTConverter`2<System.Text.Json.Serialization.Tests.CollectionTests/MockedAsyncEnumerable`1<int>, int>:OnWriteResume (System.Text.Json.Utf8JsonWriter,System.Text.Json.Serialization.Tests.CollectionTests/MockedAsyncEnumerable`1<int>,System.Text.Json.JsonSerializerOptions,System.Text.Json.WriteStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Collections.Concurrent.ConcurrentDictionary`2<int, int>, int, int>:Add (int,int modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Collections.Concurrent.ConcurrentDictionary`2<string, System.Text.Json.Serialization.Tests.StreamTests/ImmutableStructWithStrings>, string, System.Text.Json.Serialization.Tests.StreamTests/ImmutableStructWithStrings>:Add (string,System.Text.Json.Serialization.Tests.StreamTests/ImmutableStructWithStrings modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Collections.Concurrent.ConcurrentDictionary`2<string, int>, string, int>:Add (string,int modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Collections.Concurrent.ConcurrentDictionary`2<string, ulong>, string, ulong>:Add (string,ulong modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Collections.Generic.IDictionary`2<string, System.Nullable`1<System.DateTime>>, string, System.Nullable`1<System.DateTime>>:Add (string,System.Nullable`1<System.DateTime> modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Collections.Generic.IDictionary`2<string, int>, string, int>:Add (string,int modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Text.Json.Serialization.Tests.CollectionTests/DictionaryThatOnlyImplementsIDictionaryOfStringTValue`1<int>, string, int>:Add (string,int modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Text.Json.Serialization.Tests.GenericStructIDictionaryWrapper`2<string, string>, string, string>:Add (string,string modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)
void System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<System.Text.Json.Serialization.Tests.NullableTests/MyIDictionaryWrapper`1<System.Nullable`1<single>>, string, System.Nullable`1<single>>:Add (string,System.Nullable`1<single> modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)

It's not clear why these fail, needs to be investigated.

/cc @lambdageek

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 26, 2021
@lambdageek lambdageek added this to the 6.0.0 milestone Aug 26, 2021
@SamMonoRT SamMonoRT removed the untriaged New issue has not been triaged by the area owner label Aug 26, 2021
akoeplinger added a commit to akoeplinger/runtime that referenced this issue Aug 27, 2021
A number of tests using `dynamic` can't work on iOS devices due to System.Reflection.Emit not being supported, added PlatformDetection checks for those.

A few other tests run into an "Attempting to JIT compile method" issue that is tracked by dotnet#58204
akoeplinger added a commit that referenced this issue Aug 30, 2021
A number of tests using `dynamic` can't work on iOS devices due to "dynamic invoke" (where Mono does an indirect runtime invoke using a single universal invoke wrapper) not being supported, added PlatformDetection checks for System.Reflection.Emit since that is a close enough approximation for those.

A few other tests run into an "Attempting to JIT compile method" issue that is tracked by #58204
@marek-safar
Copy link
Contributor

@SamMonoRT who is working on this?

@SamMonoRT
Copy link
Member

@marek-safar - at the moment no one, but believe @imhameed has a PR up for fullaot and may have seen this error ?

@SamMonoRT SamMonoRT added the disabled-test The test is disabled in source code against the issue label Aug 31, 2021
@imhameed
Copy link
Contributor

imhameed commented Sep 1, 2021

I've seen this for several runtime tests (see #57350), but I don't know what the cause or causes are yet.

@lambdageek
Copy link
Member

lambdageek commented Sep 1, 2021

@imhameed all these disabled STJ tests use dynamic in C# in some way or another. I didn't dig into how we AOT dynamic function calls, yet, but it's possible we either don't support it, or we're missing some pattern that the STJ serialization is using.

Nevermind. I misunderstood what #58276 was doing

@akoeplinger
Copy link
Member Author

akoeplinger commented Sep 1, 2021

@lambdageek no, the issues here are not in tests using dynamic. They happen in "normal" System.Text.Json tests.

@BrzVlad BrzVlad self-assigned this Sep 7, 2021
@BrzVlad
Copy link
Member

BrzVlad commented Sep 10, 2021

I wasn't able to deploy the test app to device. I'll investigate next week once I'm added to Microsoft iOS Development Team

@akoeplinger
Copy link
Member Author

akoeplinger commented Sep 11, 2021

Yup, the command for that is: ./dotnet.sh build /t:Test /p:TargetOS=iOS /p:TargetArchitecture=arm64 /p:RunAOTCompilation=true /p:DevTeamProvisioning=UBF8T346G9 /p:MonoEnableLLVM=false src/libraries/System.Text.Json/tests/System.Text.Json.Tests

(note that we currently have a bug where we don't recognize that we ILStripped the binaries so if you run this a second time it'll try to AOT the stripped binaries which will fail, probably best to just comment out the <ILStrip /> in tests.mobile.targets)

@akoeplinger akoeplinger changed the title Attempting to JIT compile method in System.Test.Json tests on iOS devices Attempting to JIT compile method in System.Text.Json tests on iOS devices Sep 11, 2021
@BrzVlad
Copy link
Member

BrzVlad commented Sep 15, 2021

These failures are caused by limitations in gsharedvt existing since its implementation. The limitation is reproduced with a constrained call with a signature containing multiple gsharedvt args. (https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/method-to-ir.c#L3684)

This issue can be reproduced from the iOS sample when executing this code

    private static T AssertThrows<T> (Action act) where T : Exception
    {       
            try {
                 act ();
            } catch (T e) {
                 return e;
            }
            throw new Exception ("Was supposed to throw");
    }
    private static void TestMethod ()
    {       
            AssertThrows<JsonException>(() => JsonSerializer.Deserialize<IList<object>>("[1,"));
            AssertThrows<JsonException>(() => JsonSerializer.Deserialize<IList<int>>("[1,"));
            AssertThrows<JsonException>(() => JsonSerializer.Deserialize<IList<int>>("[1")); 
            AssertThrows<JsonException>(() => JsonSerializer.Deserialize<IDictionary<string, int>>("{\"key\":1,"));
            AssertThrows<JsonException>(() => JsonSerializer.Deserialize<IDictionary<string, int>>("{\"key\":1"));
    }

When aot compilling System.Text.Json.dll, an example of a method hitting this limitation is System.Text.Json.Serialization.Converters.IDictionaryOfTKeyTValueConverter`3<TDictionary_GSHAREDVT, TKey_GSHAREDVT, TValue_GSHAREDVT>:Add (TKey_GSHAREDVT,TValue_GSHAREDVT modreq(System.Runtime.InteropServices.InAttribute)&,System.Text.Json.JsonSerializerOptions,System.Text.Json.ReadStack&)

@vargaz will take a look at this but the fix is likely not trivial so I don't see this making 6.0

@SamMonoRT
Copy link
Member

Moving to 7.0 based on above comments.

@SamMonoRT SamMonoRT modified the milestones: 6.0.0, 7.0.0 Sep 15, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 15, 2021
vargaz added a commit to vargaz/runtime that referenced this issue Sep 16, 2021
@lambdageek
Copy link
Member

@vargaz Does #59182 seem risky overall?

@SamMonoRT @akoeplinger @layomia Are the testcases here something contrived or possibly something that real users could run into? (@layomia it's the ones from #58276)

@vargaz
Copy link
Contributor

vargaz commented Sep 17, 2021

It's somewhat risky, if this is needed for 6.0, we can create a version where the new code is ifdef ios|android, and the old code is used on wasm.

@lambdageek
Copy link
Member

It's somewhat risky, if this is needed for 6.0, we can create a version where the new code is ifdef ios|android, and the old code is used on wasm.

I think we should hold off unless we get some customer reports that this is a problem in practice.

vargaz added a commit that referenced this issue Sep 17, 2021
)

* [mono] Remove some of the restrictions on constrained calls from
gsharedvt methods.

Fixes #58204.

* Reenable tests.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-AOT-mono disabled-test The test is disabled in source code against the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants