Refactor GC background thread creation #6384
Conversation
|
@jkotas can you take a look please? |
src/gc/gc.cpp
Outdated
| @@ -26879,26 +26818,10 @@ uint32_t gc_heap::bgc_thread_function() | |||
| BOOL do_exit = FALSE; | |||
| Thread* thread_to_destroy = 0; | |||
There was a problem hiding this comment.
It would be nice to move the DestroyThread call to the wrapper function as well, and remove DestroyThread from the gcenv headers (the other call to DestroyThread in gc_heap::kill_gc_thread is completely broken - it is not causing problems because this function is never called).
|
LGTM otherwise |
|
@jkotas I've reflected your feedback, can you take another look please? |
src/gc/env/gcenv.base.h
Outdated
|
|
||
| #ifdef FEATURE_REDHAWK | ||
| typedef uint32_t (__stdcall *BackgroundCallback)(void* pCallbackContext); | ||
| REDHAWK_PALIMPORT bool REDHAWK_PALAPI PalStartBackgroundGCThread(BackgroundCallback callback, void* pCallbackContext); |
There was a problem hiding this comment.
Can PalStartBackgroundGCThread can be removed as well?
|
👍 |
This change modifies the GCToEEInterface::CreateBackgroundThread so that it returns a fully initialized and running thread.
janvorli
force-pushed
the
refactor-gc-thread-creation-2
branch
from
27f89b7 to
0537929
Compare
|
I've removed the PalStartBackgroundGCThread and squashed the commits |
|
We were not creating the equivalent of threadStartedEvent everytime we created a BGC thread - we just created one time and kept it. With the new code you are, could you restore the old behavior? |
|
Why would you want to keep the event alive forever? I think it is better to create it just for the small window that it is needed. |
|
@Maoni0 The code in GCToEEInterface where the event is used doesn't have any idea about GC heaps and that event was originally per heap. While I could keep that event in the hc_heap and pass it to the GCToEEInterface::CreateBackgroundThread as an additional parameter, that would be quite ugly, since the event has no meaning for the GC code anymore. |
|
you don't need to have the event in the gc_heap and pass it, you can just have the event stored in gcenv.ee.cpp. GC doesn't need to know about it at all. the event is likely needed forever because we'll likely keep doing BGCs but not frequent enough to keep the thread alive. |
|
Thinking about this a bit more I think it's fine to keep it the way it is. We had different policies for server and workstation GC. for server we don't time out the threads so we'd only get into the situation of recreating the bgc thread in workstation case and there's no per heap there. But perhaps one day we may want to also timeout BGC threads for server then it would get kind of ugly to have to have code in gcenv.ee.cpp to recognize the difference between svr and wks. |
|
Creating a new thread is several orders of magnitude more expensive operation than creating the event. So it is fine to create/close the event each time - there is no value in caching it. |
This change modifies the GCToEEInterface::CreateBackgroundThread so that it returns a fully initialized and running thread. Commit migrated from dotnet/coreclr@3b5550f
4 participants
This change modifies the GCToEEInterface::CreateBackgroundThread so that it returns
a fully initialized and running thread.