[local gc] refactor apis for threading and suspension to avoid redundant calls in to the runtime by davmason · Pull Request #16765 · dotnet/coreclr

davmason · 2018-03-05T22:18:13Z

This starts with the work from #14907 and continues with the following:

GCToEEInterface::EnablePreemptiveGC now returns a bool indicating whether the thread mode was changed
Callback apis for the GC to be notified when the runtime is trapping threads
Changing allow_fgc to check if the runtime is trapping threads rather than if the current thread is at a safe point
Removing CatchAtSafepoint from the EE interface
Refactoring WaitLongerNoInstru and WaitLonger to only call in to the runtime to set preemptive/cooperative mode

This became a fairly large PR, let me know if I should separate out the allow_fgc/WaitLongerNoInstru work in to a separate PR. It should be fairly easy.

So far I've run the tests locally and have a GC stress run in progress. It's been about 12 hours with no issues found, I intend to run GC stress for 48 hours.

…oc contexts to always operate on current thread

…nused call to GetThread

…eturn a bool indicating whether the mode was changed

…ing threads

… for allow_fgc

jkotas · 2018-03-05T23:15:19Z

+    // Queries whether or not the current thread has preemptive GC disabled.
    virtual
-    bool IsPreemptiveGCDisabled(Thread * pThread) = 0;
+    bool IsPreemptiveGCDisabled() = 0;


Nit: I always found the double negations like this very hard to reason about. Would it make sense to call this IsPreemptiveGCModeEnable ?

If we were introducing from scratch I would agree with you, but I don't think it makes sense in this case. It is named after Thread::PreemptiveGCDisabled and makes sense to keep that way.

Maybe we can replace all of the PreemptiveGCDisabled in the whole runtime and GC in a separate PR. I've also found the naming to make thinking about the state difficult. There are 312 occurences total.

jkotas · 2018-03-05T23:15:33Z

+    bool IsPreemptiveGCDisabled() = 0;

-    // Enables preemptive GC on the given thread.
+    // Enables preemptive GC on the current thread.


Could you please add comment on what this returns?

jkotas · 2018-03-05T23:17:18Z

@@ -245,13 +245,9 @@ class IGCToCLR {
    virtual
    bool TrapReturningThreads() = 0;


We should not need TrapReturningThreads now that the GC is keeping a mirror.

jkotas · 2018-03-05T23:20:41Z

-    pThread->EnablePreemptiveGC();
+
+    Thread* pThread = ::GetThread();
+    bool bToggleGC = GCToEEInterface::IsPreemptiveGCDisabled();


This should just call the method on pThread. Not go through GCToEEInterface

jkotas · 2018-03-05T23:23:00Z

 {
-    return pThread->EnablePreemptiveGC();
+    Thread* pThread = ::GetThread();
+    bool bToggleGC = GCToEEInterface::IsPreemptiveGCDisabled();


jkotas · 2018-03-05T23:26:30Z

 BOOL reset_mm_p = TRUE;

 bool g_fFinalizerRunOnShutDown = false;
+VOLATILE(int32_t) g_fSuspensionPending = 0;


Nit: This can be in gccommon.cpp. If it is here, it is going to be compiled twice (once for server and once for workstation).

davmason · 2018-03-08T02:55:23Z

@jkotas I just added a commit that addresses all your feedback except the IsPreemptiveGCDisabled for the reasons I stated above. PTAL

jkotas · 2018-03-08T03:41:12Z


-    // Enables preemptive GC on the given thread.
+    // Enables preemptive GC on the current thread. Returns true if the thread mode 
+    // was changed and false if the therad mode wasn't changed or the thread is not


Typo: therad

jkotas · 2018-03-08T03:44:40Z

@@ -461,18 +461,17 @@ uint32_t gc_heap::user_thread_wait (GCEvent *event, BOOL no_mode_change, int tim
    if (!no_mode_change)
    {
        pCurThread = GCToEEInterface::GetThread();


This call to GetThread() and check for null is unnecessary. GCToEEInterface::EnablePreemptiveGC does the same check already.

jkotas · 2018-03-08T03:45:23Z


    dwWaitResult = event->Wait(time_out_ms, FALSE);

    if (!no_mode_change && mode)


Nit: This condition can be simplified to if (mode)

Maoni0 · 2018-03-12T17:35:20Z

+    enable_preemptive ();
    GCToOSInterface::Sleep (1);
-    disable_preemptive (current_thread, TRUE);
+    disable_preemptive (TRUE);


TRUE [](start = 24, length = 4)

nit - true instead of TRUE, same with other places where disable is called

Maoni0 · 2018-03-12T18:14:42Z

 #ifdef BACKGROUND_GC
-        Thread* current_thread = GCToEEInterface::GetThread();
-        BOOL cooperative_mode = TRUE;
+        bool cooperative_mode = TRUE;


TRUE [](start = 32, length = 4)

nit - true

Maoni0

swgillespie and others added 8 commits November 3, 2017 15:53

[Local GC] Refactor calls involving thread modes, suspension, and all…

d035e1e

…oc contexts to always operate on current thread

BOOL -> bool for enable/disable preemptive routines, also remove an u…

71ac272

…nused call to GetThread

fixing merge conflicts

38b0cb3

avoid one indirection by having GCToEEInterface::EnablePreemptiveGC r…

96d14cb

…eturn a bool indicating whether the mode was changed

Callback on IGCHeap for the runtime to notify the GC when it is trapp…

77c31f5

…ing threads

use g_fSuspensionPending instead of GCToEEInterface::CatchAtSafePoint…

3a741b5

… for allow_fgc

Remove CatchAtSafePoint

b916eba

Remove GetThread from WaitLongerNoInstru

5c98cc4

davmason added the area-GC label Mar 5, 2018

davmason added this to the 2.1.0 milestone Mar 5, 2018

davmason self-assigned this Mar 5, 2018

davmason requested review from Maoni0 and jkotas March 5, 2018 22:18

jkotas suggested changes Mar 5, 2018

View reviewed changes

code review feedback

d828ae0

jkotas reviewed Mar 8, 2018

View reviewed changes

jkotas approved these changes Mar 8, 2018

View reviewed changes

code review feedback

aacaa7a

Maoni0 reviewed Mar 12, 2018

View reviewed changes

Maoni0 approved these changes Mar 12, 2018

View reviewed changes

change BOOL to bool

6efec1a

davmason merged commit 8edb3ef into dotnet:master Mar 13, 2018

davmason deleted the gc_threading branch March 13, 2018 08:24

jkotas mentioned this pull request Mar 23, 2018

[Local GC] Refactor calls involving thread modes, suspension, and all… #14907

Closed

MCRcortex mentioned this pull request Aug 10, 2026

NativeAOT(GarbageCollector): BGC allow_fgc does not yield to GCToEEInterface::SuspendEE during FGC in NativeAoT dotnet/runtime#132064

Closed

		@@ -245,13 +245,9 @@ class IGCToCLR {
		virtual
		bool TrapReturningThreads() = 0;


		dwWaitResult = event->Wait(time_out_ms, FALSE);

		if (!no_mode_change && mode)

Conversation

davmason commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davmason commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maoni0 Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maoni0 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Maoni0 Mar 12, 2018 •

edited

Loading