-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[NativeAOT] Refactored GCToEEInterface and RedhawkGCInterface implementations into separate files. #95991
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThis is mostly just separating implementation of 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
|
@@ -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*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is header file. Does it need to be marked inline
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -385,6 +385,11 @@ void Thread::Destroy() | |||
ASSERT(m_pGCFrameRegistrations == NULL); | |||
} | |||
|
|||
gc_alloc_context* Thread::GetAllocContext() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be in thread.inl
? It seems important that this method gets inlined.
@@ -24,6 +23,7 @@ set(COMMON_RUNTIME_SOURCES | |||
ObjectLayout.cpp | |||
portable.cpp | |||
RestrictedCallouts.cpp | |||
RedhawkGCInterface.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use it here for consistency.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the actual types in signatures to avoid these casts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
Co-authored-by: Robin Sue <[email protected]>
3d9310c
to
57b0973
Compare
|
||
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Now that What it does is - when given an enumeration callback and an object/range/frame it enumerates object references appropriately. |
It is 4 static methods (~100 lines):
I think they can be global static methods. |
The test failure seems to be a variety of #94728 , just with timer queue this time.
|
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#. |
Co-authored-by: Jan Kotas <[email protected]>
I do not have any more changes planned, unless I missed some actionable comment. |
@@ -18,6 +17,9 @@ | |||
#include "shash.h" | |||
#include <minipal/cpufeatures.h> | |||
|
|||
#include "CommonMacros.inl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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++.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logged #96081 about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Thanks!! |
This is mostly just separating implementation of
GCToEEInterface
(GCToEEInterface.cpp) andRedhawkGCInterface
(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 hadRedhawkGCInterface::EnumGcRef
implemented viaGcEnumObject
which was also a helper forGCToEEInterface::GcScanRoots
, thus an "extern" from a different .cpp file.