Remove non INPLACE_RUNTIME NativeAOT paths, cleanup exceptions #100379

MichalPetryka · 2024-03-28T00:56:03Z

Continuation of #99778

MichalStrehovsky · 2024-03-28T09:19:05Z

If we're removing INPLACE_RUNTIME, we should be sure that this is the layering we actually want, in the theoretical case that we'd need to extract a shared piece (for e.g. sharing between multiple dlls loaded in the same process, etc.). There's a lot more that can/should be ripped out here if that's what we want.

Casting and interface dispatch would probably go to a CoreLib and they would not be part of the runtime anymore.

But exception handling - I don't have much clarity about that.

I've been avoiding touching this so that I don't have to think about this. Some of this code could in theory be compiled into an obj file that is linked into coreclr.dll at some point and then we do need a form of INPLACE_RUNTIME. Similarly for the NAOT scenario I mentioned in the first paragraph.

I'd leave these questions for Jan, I'm sure he already has some thoughts. This is all the thoughts I have around it.

src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/RuntimeExports.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/MethodTable.Runtime.cs

src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/RuntimeExports.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Array.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Exception.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/RuntimeExports.cs

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs

MichalStrehovsky · 2024-04-03T04:46:38Z

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs

@@ -432,7 +431,7 @@ private static unsafe object CheckCastClassSpecial(MethodTable* pTargetType, obj
                    goto done;

                // They don't cast to any other class
-                goto fail;
+                ThrowHelper.ThrowInvalidCastException();


Is this introducing a frame to this method? This structuring was obviously deliberate and matches ChkCastClassSpecial in the other corelib. What motivates this change?

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs

MichalStrehovsky · 2024-04-03T04:47:49Z

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs

-        // Would not be inlined, but still need to mark NoInlining so that it doesn't throw off tail calls
-        [MethodImpl(MethodImplOptions.NoInlining)]
-        private static unsafe object ThrowInvalidCastException(MethodTable* pMT)


ThrowHelper.ThrowInvalidCastException looks different (doesn't return object, is not marked noinlining). Are we deoptimizing codegen or are the underlying RyuJIT issues fixed up?

RyuJIT understands and special cases throw helpers that aren't marked with NoInlining so this should improve the codegen or keep it the same.

I've checked and the codegen is different, the JIT now puts the throw in a cold block at the end of the method instead of a tailcall in the middle. I'd argue it's better since the need for a jump for the exception shouldn't matter much.

jkotas · 2024-06-18T01:32:17Z

@MichalPetryka Are you interested in finishing this PR?

MichalPetryka · 2024-06-18T01:48:53Z

@MichalPetryka Are you interested in finishing this PR?

I'll get back to it tomorrow.

MichalStrehovsky · 2024-06-24T06:20:58Z

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

@@ -1081,38 +989,9 @@ private static uint CalculateCodeOffset(byte* pbControlPC, in MethodRegionInfo m
            return false;
        }

-#if DEBUG && !INPLACE_RUNTIME && NATIVEAOT
-        private static MethodTable* s_pLowLevelObjectType;
-        private static void AssertNotRuntimeObject(MethodTable* pClauseType)


This still looks valuable if we expect exception handling would still use some C# code and might end up being shared between all managed runtime instances within a process (for loading-multiple-nativeAOT-dlls-in-the-same-process-and sharing-low-level-runtime case).

I don't know how such layering would look like, so I defer to Jan. The existing INPLACE_LAYERING was known to actually work because it was used like that in .NET Native. I don't know about this new layering.

I think that the shared component should be GC and GC support like thread suspension, not much else. Ideally, we would have an option to run in a configuration where a CoreCLR instance and native AOT instances would be able to share one process wide GC instance. Exception handling, casting, interface dispatch should be all private per-runtime. I think that the .NET Native design where these services tried to be in the shared component was problematic.

With this plan, the shared component may still need some managed code that can be stackwalked for good perf, but it should not need to do any allocations or handle exceptions on its own. We would use compiler/linker to enforce that. It means that it should be fine to delete this code.

MichalStrehovsky · 2024-06-24T06:22:56Z

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs

-
-        fail:
-            return ThrowInvalidCastException(pTargetType);


My old comment still stands - this is the existing structuring in the other copy of TypeCast.cs used with JIT.

They should be in sync, so either do the same change there (better to be 100% sure this will not deoptimize codegen) or undo it here.

MichalStrehovsky · 2024-06-24T06:25:27Z

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/TypeCast.cs

            }

            return result;
        }

        private static unsafe object IsInstanceOfVariantType(MethodTable* pTargetType, object obj)
        {
+            Debug.Assert(obj is not null);


These new asserts are not useful. We'll nullref on the subsequent line which is as good as an assert.

Debug.Assert is not low level enough to be callable from casting code. We could end up with reentrancy and then instead of troubleshooting an easy nullref, we need to troubleshoot a stack overflow or something worse.

dotnet-policy-service · 2024-09-10T01:58:57Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

dotnet-policy-service · 2024-09-24T06:35:53Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

Remove non INPLACE_RUNTIME NativeAOT paths, cleanup exceptions

8da67f1

MichalPetryka requested a review from MichalStrehovsky as a code owner March 28, 2024 00:56

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Mar 28, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 28, 2024

build-analysis bot mentioned this pull request Mar 28, 2024

Error reported in diagnostic logs. Please examine the log for more details dotnet/dnceng#1928

Open

3 tasks

jkotas reviewed Mar 28, 2024

View reviewed changes

Review changes

da61990

jkotas reviewed Mar 29, 2024

View reviewed changes

build-analysis bot mentioned this pull request Mar 29, 2024

System.Runtime.InteropServices.JavaScript.Tests.JSImportTest.MissingImport test fails on CI leg: browser-wasm linux Release LibraryTests_Threading #100421

Closed

Review

10ab978

build-analysis bot mentioned this pull request Mar 30, 2024

[browser][MT] TIMED_OUT but 0 was expected #99888

Closed

jkotas reviewed Mar 31, 2024

View reviewed changes

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs Outdated Show resolved Hide resolved

jkotas reviewed Mar 31, 2024

View reviewed changes

src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/InternalCalls.cs Outdated Show resolved Hide resolved

jkotas reviewed Mar 31, 2024

View reviewed changes

src/coreclr/nativeaot/Runtime.Base/src/System/Array.cs Outdated Show resolved Hide resolved

jkotas reviewed Mar 31, 2024

View reviewed changes

src/coreclr/nativeaot/Runtime.Base/src/System/Exception.cs Outdated Show resolved Hide resolved

Cleanup

9c9ce90

This was referenced Apr 1, 2024

System.IO.Tests.File_GetSetTimes_SafeFileHandle.WritingShouldUpdateWriteTime_After_SetLastAccessTime failing #97020

Open

mono_os_mutex_destroy: pthread_mutex_destroy failed with "Device or resource busy" #99609

Closed

jkotas reviewed Apr 1, 2024

View reviewed changes

MichalStrehovsky reviewed Apr 3, 2024

View reviewed changes

agocke added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 20, 2024

dotnet-policy-service bot added the no-recent-activity label Jun 3, 2024

dotnet-policy-service bot closed this Jun 17, 2024

jkotas reopened this Jun 18, 2024

dotnet-policy-service bot removed the no-recent-activity label Jun 18, 2024

MichalPetryka added 2 commits June 21, 2024 03:50

Merge remote-tracking branch 'upstream/main' into inplace-aot

1e36fc8

Address comments

a765dd8

dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 21, 2024

MichalStrehovsky reviewed Jun 24, 2024

View reviewed changes

agocke added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 26, 2024

dotnet-policy-service bot added the no-recent-activity label Sep 10, 2024

dotnet-policy-service bot closed this Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove non INPLACE_RUNTIME NativeAOT paths, cleanup exceptions #100379

Remove non INPLACE_RUNTIME NativeAOT paths, cleanup exceptions #100379

MichalPetryka commented Mar 28, 2024

MichalStrehovsky commented Mar 28, 2024

MichalStrehovsky Apr 3, 2024

MichalStrehovsky Apr 3, 2024

MichalPetryka Apr 5, 2024

MichalPetryka Jun 21, 2024

jkotas commented Jun 18, 2024

MichalPetryka commented Jun 18, 2024

MichalStrehovsky Jun 24, 2024

jkotas Jul 4, 2024

MichalStrehovsky Jun 24, 2024

MichalStrehovsky Jun 24, 2024

dotnet-policy-service bot commented Sep 10, 2024

dotnet-policy-service bot commented Sep 24, 2024

Remove non INPLACE_RUNTIME NativeAOT paths, cleanup exceptions #100379

Remove non INPLACE_RUNTIME NativeAOT paths, cleanup exceptions #100379

Conversation

MichalPetryka commented Mar 28, 2024

MichalStrehovsky commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jun 18, 2024

MichalPetryka commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnet-policy-service bot commented Sep 10, 2024

dotnet-policy-service bot commented Sep 24, 2024