[NativeAOT] Refactored GCToEEInterface and RedhawkGCInterface implementations into separate files. #95991

VSadov · 2023-12-14T00:21:04Z

This is mostly just separating implementation of GCToEEInterface (GCToEEInterface.cpp) and RedhawkGCInterface (RedhawkGCInterface.cpp).

These interfaces serve unrelated purposes. One is for GC calling into runtime and another for the runtime calling into GC things.

We had both interfaces implemented in the same file and a part of GCToEEInterface in a separate file (rhscan.cpp), which lead to some confusing patterns. For example we had RedhawkGCInterface::EnumGcRef implemented via GcEnumObject which was also a helper for GCToEEInterface::GcScanRoots, thus an "extern" from a different .cpp file.

ghost · 2023-12-14T00:21:17Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

This is mostly just separating implementation of GCToEEInterface (GCToEEInterface.cpp) and RedhawkGCInterface (RedhawkGCInterface.cpp).

These interfaces serve unrelated purposes. One is for GC calling into runtime and another for the runtime calling into GC things.

We had both interfaces implemented in the same file and a part of GCToEEInterface in a separate file (rhscan.cpp), which lead to some confusing patterns. For example we had RedhawkGCInterface::EnumGcRef implemented via GcEnumObject which was also a helper for GCToEEInterface::GcScanRoots, thus an "extern" from a different .cpp file.

Author:	VSadov
Assignees:	-
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

src/coreclr/nativeaot/Runtime/MiscHelpers.cpp

jkotas · 2023-12-14T02:49:50Z

src/coreclr/nativeaot/Runtime/SpinLock.h

@@ -3,8 +3,15 @@
 #ifndef __SPINLOCK_H__
 #define __SPINLOCK_H__

-// defined in gcrhenv.cpp
-bool __SwitchToThread(uint32_t dwSleepMSec, uint32_t dwSwitchCount);
+bool __SwitchToThread(uint32_t dwSleepMSec, uint32_t /*dwSwitchCount*/)


This is header file. Does it need to be marked inline?

It looks like this lock is not used anywhere. It is also a dangerously simplistic implementation that could only be ok in trivial never contending cases.
Seems like better to just delete the file.

jkotas · 2023-12-14T02:54:28Z

src/coreclr/nativeaot/Runtime/thread.cpp

@@ -385,6 +385,11 @@ void Thread::Destroy()
    ASSERT(m_pGCFrameRegistrations == NULL);
 }

+gc_alloc_context* Thread::GetAllocContext()


Should it be in thread.inl ? It seems important that this method gets inlined.

jkotas · 2023-12-14T02:57:35Z

src/coreclr/nativeaot/Runtime/CMakeLists.txt

@@ -24,6 +23,7 @@ set(COMMON_RUNTIME_SOURCES
    ObjectLayout.cpp
    portable.cpp
    RestrictedCallouts.cpp
+    RedhawkGCInterface.cpp


Should we start moving away from the Redhawk name? One needs decoder ring to understand what it means.

What would this file be called if we have done further refactoring to share it with regular CoreCLR?

What would this file be called if we have done further refactoring to share it with regular CoreCLR?

I think EEToGC interface is a specifically a NativeAOT thing. It looks like there was a reason/effort in the past to not spread knowledge about GC all over the runtime.

In CoreCLR it is more adhoc - we just do GCHeapUtilities::GetGCHeap() and call IGCHeap stuff. I do not think there is an interface. Helpers that are more complex live closer to their use. I.E. the EnumGcRefs lives in EECodeManager

That is also what we do in many cases in NativeAOT now. RedhawkGCInterface is still convenient to park complicated helpers, but some RedhawkGCInterface methods just delegate to GCHeapUtilities::GetGCHeap()->..., so it could be more historical that we have this.

Maybe we do not need this interface. We could just call GCHeapUtilities::GetGCHeap() in simple cases and see what remains here.
Possibly it will be just initialization and ref scanning, which may also go to places that use them - startup.cpp, thread.cpp and the like.

I think EEToGC interface is a specifically a NativeAOT thing. It looks like there was a reason/effort in the past to not spread knowledge about GC all over the runtime.

Yes, build of the native redhawk runtime was setup in very complicated way with a lot of artificial layers. I do not think we need to maintain that.

Re: GCToEEInterface.cpp

I am not very attached to the actual name of this file. I had to choose some kind of name, preferrably self-documenting the purpose of the file, so I just matched the name of the class that is implemented there.

The coreclr does have an analog for this file - gcenv.ee.cpp. That is not a very self-documenting name, but we could still use it here for consistency.

I would use it here for consistency.

jkotas · 2023-12-14T05:11:50Z

src/coreclr/nativeaot/Runtime/GCToEEInterface.cpp

+            while (pRoot != NULL)
+            {
+                STRESS_LOG2(LF_GC | LF_GCROOTS, LL_INFO100, "{ Scanning Thread's %p inline thread statics root %p. \n", pThread, pRoot);
+                RedhawkGCInterface::EnumGcRef((PTR_RtuObjectRef)&pRoot->m_threadStaticsBase, GCRK_Object, (void*)fn, sc);


Can we use the actual types in signatures to avoid these casts?

It is possible to avoid casts of scan funcs. I was just afraid it would cause too much of cascading changes, but it ended up not too bad.

I think I will change RtuObjectRef --> OBJECT_REF as well, while I am at this.

kasperk81 · 2023-12-14T19:03:31Z

src/coreclr/nativeaot/Runtime/RedhawkGCInterface.cpp

NativeAOTGCInterface.cpp? some of them were replaced in #79217

Co-authored-by: Robin Sue <robinsue@live.de>

jkotas · 2023-12-15T18:01:30Z

src/coreclr/nativeaot/Runtime/thread.h

+
+    // The MethodTable is remembered in some slow-path allocation paths. This value is used in event tracing.
+    // It may statistically correlate with the most allocated type on the given stack/thread.
+    MethodTable*            m_LastAllocationEEType;


This can be a regular thread static in GCHelpers.cpp. I think we should avoid placing every thread static into thread.h. thread.h should only have the special thread statics that are accessed from external threads.

Yes, I think this went a bit too far.
LastAllocationEEType is a fancy of allocation slow path and allocation tick event. Thread does not need to know or report about this.

VSadov · 2023-12-15T19:08:19Z

Now that RedhawkGCInterface only contains GC enum functionality, I'd like to rename it accordingly.

What it does is - when given an enumeration callback and an object/range/frame it enumerates object references appropriately.
Should it be "GcEnum" ? As in ( GcEnum::EnumGcRefsInRegionConservatively )

jkotas · 2023-12-15T21:20:59Z

It is 4 static methods (~100 lines):

EnumGcRef
EnumGcRefs
EnumGcRefConservatively
EnumGcRefsInRegionConservatively

I think they can be global static methods.

VSadov · 2023-12-15T22:22:53Z

The test failure seems to be a variety of #94728 , just with timer queue this time.

         at System.Threading.Lock.TryInitializeStatics() + 0xff
         at System.Threading.Lock.LazyInitializeOrEnter() + 0x150
         at System.Threading.Lock.TryEnterSlow(Int32, Lock.ThreadId) + 0x22d
         at System.Threading.Lock.TryEnter_Inlined(Int32) + 0x107
         at System.Threading.Lock.EnterAndGetCurrentThreadId() + 0x2b
         at System.Threading.Lock.EnterScope() + 0x32
         at System.Runtime.CompilerServices.ClassConstructorRunner.Cctor.GetCctor(StaticClassConstructionContext*) + 0x50
         at System.Runtime.CompilerServices.ClassConstructorRunner.EnsureClassConstructorRun(StaticClassConstructionContext*) + 0x6e
         at System.Runtime.CompilerServices.ClassConstructorRunner.CheckStaticClassConstructionReturnGCStaticBase(StaticClassConstructionContext*, Object) + 0x22
         at System.Diagnostics.Tracing.NativeRuntimeEventSource..cctor() + 0x3c
         at System.Runtime.CompilerServices.ClassConstructorRunner.EnsureClassConstructorRun(StaticClassConstructionContext*) + 0x1a4
         --- End of inner exception stack trace ---
         at System.Runtime.CompilerServices.ClassConstructorRunner.EnsureClassConstructorRun(StaticClassConstructionContext*) + 0x26d
         at System.Runtime.CompilerServices.ClassConstructorRunner.CheckStaticClassConstructionReturnGCStaticBase(StaticClassConstructionContext*, Object) + 0x22
         at System.Threading.WaitHandle.WaitOneNoCheck(Int32, Object, NativeRuntimeEventSource.WaitHandleWaitSourceMap) + 0x1b7
         at System.Threading.WaitHandle.WaitOne(Int32) + 0x37
         at System.Threading.TimerQueue.TimerThread() + 0x11c

VSadov · 2023-12-15T22:30:42Z

I think they can be global static methods.

We can do this too. I generally prefer that methods of common functionality are scoped together, unless it is really hard to place a helper somewhere or it needs to be externally exported. Perhaps a habit from writing C#.
NativeAOT runtime is relatively small though, so a few more global methods would be ok.

src/coreclr/nativeaot/Runtime/GCHelpers.cpp

src/coreclr/nativeaot/Runtime/startup.cpp

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

VSadov · 2023-12-16T01:59:22Z

I do not have any more changes planned, unless I missed some actionable comment.

jkotas · 2023-12-16T02:07:27Z

src/coreclr/nativeaot/Runtime/AsmOffsetsVerify.cpp

@@ -18,6 +17,9 @@
 #include "shash.h"
 #include <minipal/cpufeatures.h>

+#include "CommonMacros.inl"


(Future cleanup: Including the .inl files manually like this is fragile. It would be best to make the .inl files included at the end of the .h files. It is the common pattern in C/C++.)

The situation with includes is not the best. Everything seems to include a lot of random stuff - "slist.h"? Is everything fiddling with lists?
I am especially unhappy that the order of includes often matters. I'd have points deducted in "C++ level 1" for that.

Rationalizing includes could be a tough knot to untangle.

logged #96081 about this.

jkotas

LGTM. Thanks!

VSadov · 2023-12-16T05:10:08Z

Thanks!!

VSadov requested a review from MichalStrehovsky as a code owner December 14, 2023 00:21

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label Dec 14, 2023

ghost assigned VSadov Dec 14, 2023

Suchiman reviewed Dec 14, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/MiscHelpers.cpp Outdated Show resolved Hide resolved

jkotas reviewed Dec 14, 2023

View reviewed changes

kasperk81 reviewed Dec 14, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/RedhawkGCInterface.cpp Outdated

Copy link

Contributor

kasperk81 Dec 14, 2023

Choose a reason for hiding this comment

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

NativeAOTGCInterface.cpp? some of them were replaced in #79217

VSadov and others added 10 commits December 14, 2023 14:51

renamed gcrhinterface.h --> RedhawkGCInterface

3056eda

Separated gcrhenv on GCToEEInterface.cpp and RedhawkGCInterface.cpp

633cb90

cleanup RedhawkGCInterface a bit

2544cca

unneded includes

3cbd29e

unify EnumGcRef

b147bb9

centry rename

828a9f6

Update src/coreclr/nativeaot/Runtime/MiscHelpers.cpp

9147059

Co-authored-by: Robin Sue <robinsue@live.de>

unix build

96a3316

Do not cast scan funcs

7b5b9f0

RtuObjectRef --> OBJECTREF

ad346e5

VSadov force-pushed the refRhGc branch from c9da47f to ad346e5 Compare December 14, 2023 22:59

VSadov added 3 commits December 14, 2023 16:42

deleted spinlock.h

c0ed59b

moved GetAllocContext to .inl

36d2c6d

renamed GCToEEInterface.cpp --> gcenv.ee.cpp

7499700

VSadov force-pushed the refRhGc branch 2 times, most recently from 3d9310c to 57b0973 Compare December 15, 2023 04:27

only init and scan left in RedhawkGCInterface

7457162

VSadov force-pushed the refRhGc branch from 57b0973 to 7457162 Compare December 15, 2023 17:43

jkotas reviewed Dec 15, 2023

View reviewed changes

only scanning functionality in RedhawkGCInterface

340d814

removed unused RedhawkGCInterface.h includes

58b6491

build fix

9c88b3f

This was referenced Dec 15, 2023

Test failure - System.NullReferenceException in System.Threading.Lock.TryInitializeStatics #94728

Closed

[8.0] Failing to build native components #94823

Closed

VSadov added 5 commits December 15, 2023 14:42

deleted RedhawkGCInterface class

47a9be1

fix arm build

f803a7a

move FORCEINLINE methods from Thread.cpp to .inl

d2c93ca

a few cleanups

3945520

removed LastAllocationEEType from Thread.

fa0973b

VSadov force-pushed the refRhGc branch from a60f07b to d2c93ca Compare December 16, 2023 01:11

jkotas reviewed Dec 16, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/GCHelpers.cpp Outdated Show resolved Hide resolved

jkotas reviewed Dec 16, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/GCHelpers.cpp Outdated Show resolved Hide resolved

VSadov commented Dec 16, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/startup.cpp Outdated Show resolved Hide resolved

VSadov commented Dec 16, 2023

View reviewed changes

src/coreclr/nativeaot/Runtime/startup.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

e5f4254

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

jkotas reviewed Dec 16, 2023

View reviewed changes

jkotas approved these changes Dec 16, 2023

View reviewed changes

build-analysis bot mentioned this pull request Dec 16, 2023

Tracking issue for CI build timeouts #76454

Closed

VSadov merged commit 09536ba into dotnet:main Dec 16, 2023
110 checks passed

VSadov deleted the refRhGc branch December 16, 2023 05:09

SingleAccretion mentioned this pull request Dec 26, 2023

[NativeAOT-LLVM] Post-merge tasks dotnet/runtimelab#2472

Open

github-actions bot locked and limited conversation to collaborators Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NativeAOT] Refactored GCToEEInterface and RedhawkGCInterface implementations into separate files. #95991

[NativeAOT] Refactored GCToEEInterface and RedhawkGCInterface implementations into separate files. #95991

VSadov commented Dec 14, 2023

ghost commented Dec 14, 2023

jkotas Dec 14, 2023

VSadov Dec 15, 2023

jkotas Dec 14, 2023

jkotas Dec 14, 2023

VSadov Dec 14, 2023 •

edited

Loading

jkotas Dec 14, 2023

VSadov Dec 14, 2023 •

edited

Loading

jkotas Dec 15, 2023

jkotas Dec 14, 2023

VSadov Dec 14, 2023 •

edited

Loading

kasperk81 Dec 14, 2023

jkotas Dec 15, 2023

VSadov Dec 16, 2023 •

edited

Loading

VSadov commented Dec 15, 2023 •

edited

Loading

jkotas commented Dec 15, 2023

VSadov commented Dec 15, 2023

VSadov commented Dec 15, 2023

VSadov commented Dec 16, 2023

jkotas Dec 16, 2023

VSadov Dec 16, 2023 •

edited

Loading

VSadov Dec 16, 2023

jkotas left a comment

VSadov commented Dec 16, 2023

[NativeAOT] Refactored GCToEEInterface and RedhawkGCInterface implementations into separate files. #95991

[NativeAOT] Refactored GCToEEInterface and RedhawkGCInterface implementations into separate files. #95991

Conversation

VSadov commented Dec 14, 2023

ghost commented Dec 14, 2023

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

VSadov Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

VSadov commented Dec 15, 2023 • edited Loading

jkotas commented Dec 15, 2023

VSadov commented Dec 15, 2023

VSadov commented Dec 15, 2023

VSadov commented Dec 16, 2023

Choose a reason for hiding this comment

VSadov Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

VSadov commented Dec 16, 2023

VSadov Dec 14, 2023 •

edited

Loading

VSadov Dec 14, 2023 •

edited

Loading

VSadov Dec 14, 2023 •

edited

Loading

VSadov Dec 16, 2023 •

edited

Loading

VSadov commented Dec 15, 2023 •

edited

Loading

VSadov Dec 16, 2023 •

edited

Loading