[Local GC] Unify background GC thread and server GC thread creation #14821

swgillespie · 2017-11-02T16:59:35Z

Apologies for the big PR - this ended up being a nontrivial refactoring that resulted in a fair amount of code shuffle.

The big idea behind this PR is to unify the creation of threads in the GC so that they all go through the same code path. The GC creates threads for two reasons:

Background GC threads. One thread is created for every heap but they are left unaffinitized and run at the default scheduling priority. They are unique in that they can be suspended and, as such, have a cooperative/preemptive mode; if a foreground GC happens during a background GC, the GC will transition the running BGC thread to preemptive mode and back (blocking the BGC thread until the FGC completes).
Server GC threads. One thread is created for every heap and they (by default) are affinitized to a single core. They run at a higher priority than the default and can't be suspended.

Previously, these two threads were created with two different APIs: background GC threads were created with GCToEEInterface::CreateBackgroundThread while server GC threads were created with GCToOSInterface::CreateThread. This PR unifies them to instead both use GCToEEInterface::CreateThread, a new API.

In order to accomodate these two thread types, GCToEEInterface::CreateThread has two code paths; one for threads that can be suspended and ones that can't. They are adapted from the existing code in both GCToEEInterface::CreateBackgroundThread and GCToOSInterface::CreateThread, respectively. Some additional thread setup code that previously lived in gc.cpp (e.g. committing the thread stack and setting ThreadType_GC on server GC thrads) was also moved to GCToEEInterface::CreateThread, where it should be.

Since GCToEEInterface::CreateThread doesn't set thread affinity or thread priority, two new GCToOSInterface APIs were added to do so: GCToOSInterface::SetThreadAffinity and GCToOSInterface::BoostThreadPriority. These have been implemented for the non-standalone GC case and in the standalone GC case for Windows platforms:

0:014> ~11   <-- server GC thread
  11  Id: 5394.779c Suspend: 1 Teb: 00000057`68673000 Unfrozen
      Start: CoreCLR!<lambda_5c590d503e0569183472d6ea9c775c63>::<lambda_invoker_cdecl> (00007ff9`c5813900)
      Priority: 2  Priority class: 32  Affinity: 40 <-- boosted priority and single proc affinity
0:014> ~0    <-- user thread
   0  Id: 5394.3ae4 Suspend: 1 Teb: 00000057`6865d000 Unfrozen
      Start: CoreRun!wmainCRTStartup (00007ff6`8a6e3aa0)
      Priority: 0  Priority class: 32  Affinity: ff
0:014> ~10  <-- server GC thread
  10  Id: 5394.5d40 Suspend: 1 Teb: 00000057`68671000 Unfrozen
      Start: CoreCLR!<lambda_5c590d503e0569183472d6ea9c775c63>::<lambda_invoker_cdecl> (00007ff9`c5813900)
      Priority: 2  Priority class: 32  Affinity: 20 <-- boosted priority and single proc affinity

@jkotas @janvorli @sergiy-k PTAL? also cc @Maoni0 who is OOF

jkotas · 2017-11-02T17:58:04Z

src/gc/env/gcenv.ee.h

@@ -82,6 +79,7 @@ class GCToEEInterface
    static void FreeStringConfigValue(const char* key);
    static bool IsGCThread();
    static bool IsGCSpecialThread();
+    static bool CreateThread(void (*threadStart)(void*), void* arg, bool is_special, const wchar_t* name);


I would try to avoid naming things special wherever possible. special is very cryptic.

What about calling this flag suspendable?

(Also, it would be nice to rename IsGCSpecialThread to something more self-descriptive.)

jkotas · 2017-11-02T17:59:58Z

src/gc/sample/gcenv.ee.cpp

+
+bool GCToEEInterface::CreateThread(void (*threadStart)(void*), void* arg, bool is_special, const wchar_t* name)
+{
+    return nullptr;


This method returns bool...

ack, you're right - this is left over from a previous refactor.

jkotas · 2017-11-02T18:00:56Z

src/vm/gcenv.ee.h

@@ -60,6 +59,7 @@ class GCToEEInterface : public IGCToCLR {
    void FreeStringConfigValue(const char* value);
    bool IsGCThread();
    bool IsGCSpecialThread();
+	bool CreateThread(void (*threadStart)(void*), void* arg, bool is_special, const wchar_t* name);


Nit: Indentation

I think my merge tool inserts tabs... 😭 . will fix.

… clang build

swgillespie · 2017-11-02T21:03:04Z

Looks like I have wide-char issues... I'll fix that.

I'm also thinking of naming IsGCSpecial to CurrentThreadCreatedByGC as it more clearly describes what it does, if there aren't any objections (and per Jan's review comment).

…s-platform)

jkotas · 2017-11-03T00:58:50Z

src/vm/gcenv.ee.cpp

+
+        WCHAR threadName[MaxThreadNameSize];
+        LPWSTR wideName = nullptr;
+        if (MultiByteToWideChar(CP_ACP, 0, name, -1 /* null terminated */, threadName, MaxThreadNameSize))


You should use UTF8 here. Or even better - use SString that will do this conversion for you.

will do! was not aware that SString existed.

jkotas · 2017-11-03T01:01:31Z

src/vm/gcenv.ee.cpp

+            {
+#ifdef BACKGROUND_GC
+                // For background GC we revert to doing a blocking GC.
+                return;


It the BACKGROUND_GC defined when compiling VM?

(Also, CommitThreadStack is no-op in CoreCLR. You can just delete the whole block.)

Probably not - I've been looking around and it looks like this is supposed to be defined by gcpriv.h, so it won't have any effect in VM code.

Evidence that it does nothing is this block of code: https://github.com/dotnet/coreclr/blob/master/src/vm/finalizerthread.cpp#L1121-L1126, which definitely would not compile if BACKGROUND_GC were defined...

at any rate, I'll just delete the block.

jkotas · 2017-11-03T01:04:31Z

src/vm/gcenv.ee.cpp

+
+        if (!args.Thread->CreateNewThread(0, threadStub, &args, wideName))
+        {
+            args.Thread->DecExternalCount(FALSE);


Close ThreadStartedEvent ?

Done. Looks like this leak exists today: https://github.com/dotnet/coreclr/blob/master/src/vm/gcenv.ee.cpp#L468-L470

janvorli · 2017-11-03T01:08:32Z

src/gc/env/gcenv.ee.h

@@ -81,7 +78,8 @@ class GCToEEInterface
    static bool GetStringConfigValue(const char* key, const char** value);
    static void FreeStringConfigValue(const char* key);
    static bool IsGCThread();
-    static bool IsGCSpecialThread();
+    static bool CurrentThreadWasCreatedByGC();


A nit - it seems it would be nice to start this function name with the verb (WasCurrentThreadCreatedByGC)

swgillespie · 2017-11-07T00:23:51Z

could be a server GC deadlock on startup... re-running to see if it repros. I'm currently trying to repro on a mac right now.

@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test

…restarting a non-suspended thread is incorrect

jkotas · 2017-11-07T02:05:02Z

src/vm/gcenv.ee.cpp

+        return true;
+    }
+
+    bool CreateUnsuspendableThread(


Nit: CreateNonSuspendableThread may look better

swgillespie · 2017-11-07T02:09:14Z

I'm not sure if this is expected behavior in the PAL's implementation of ResumeThread, but it looks like calling ResumeThread on a thread that is not suspended has the potential to deadlock on non-Windows platforms. The Ubuntu Server GC CI failures are due to a failure to acquire the suspension lock for the suspending thread, which deadlocks. It wasn't clear to me who was holding the suspension lock and I didn't look any further since it was incorrect that my code was attempting to resume a non-suspended thread anyway.

swgillespie · 2017-11-07T21:27:04Z

@dotnet-bot test Ubuntu16.04 armlb Cross Debug Innerloop Build
@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test

swgillespie · 2017-11-08T02:49:31Z

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test

swgillespie · 2017-11-08T17:53:18Z

Not really sure what "Perf Build and Test" is, but I believe I've addressed all outstanding code review comments and passed the CI so I think this should be ready to merge if there aren't any objections.

swgillespie · 2017-11-09T17:58:53Z

thanks for the reviews!

swgillespie added 7 commits October 30, 2017 17:31

Initial cut, ignoring thread affinity

cb8c021

Integrate thread affinity

be3cda4

Affinity for standalone Windows

23a29a1

Add 'specialness' and the thread name as arguments to CreateThread

8f8f8df

First crack at unified implementation

c201782

Set priority for server GC threads

b8bdefd

Remove unused parameter

daaafe3

jkotas reviewed Nov 2, 2017

View reviewed changes

Address code review feedback and remove some dead code that broke the…

7970e85

… clang build

swgillespie added 2 commits November 2, 2017 16:50

Use char* on the interface instead of wchar_t (doesn't play well cros…

bb8855b

…s-platform)

Rename IsGCSpecialThread -> CurrentThreadWasCreatedByGC

a38b167

jkotas reviewed Nov 3, 2017

View reviewed changes

janvorli reviewed Nov 3, 2017

View reviewed changes

swgillespie added 3 commits November 3, 2017 13:27

Code review feedback and fix up the build

9dff01c

rename CurrentThreadWasCreatedByGC -> WasCurrentThreadCreatedByGC

e02b6cc

Fix a contract violation when converting string encodings

e2156e5

sergiy-k approved these changes Nov 4, 2017

View reviewed changes

Thread::CreateUtilityThread returns a thread that is not suspended - …

cba6577

…restarting a non-suspended thread is incorrect

jkotas reviewed Nov 7, 2017

View reviewed changes

CreateUnsuspendableThread -> CreateNonSuspendableThread

d8bec4d

swgillespie merged commit e64cbb5 into dotnet:master Nov 9, 2017

swgillespie deleted the gc-create-thread branch November 9, 2017 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Local GC] Unify background GC thread and server GC thread creation #14821

[Local GC] Unify background GC thread and server GC thread creation #14821

swgillespie commented Nov 2, 2017

jkotas Nov 2, 2017

swgillespie Nov 2, 2017

jkotas Nov 2, 2017

swgillespie Nov 2, 2017

jkotas Nov 2, 2017

swgillespie Nov 2, 2017

swgillespie commented Nov 2, 2017

jkotas Nov 3, 2017

swgillespie Nov 3, 2017

jkotas Nov 3, 2017

jkotas Nov 3, 2017

swgillespie Nov 3, 2017

jkotas Nov 3, 2017

swgillespie Nov 3, 2017

janvorli Nov 3, 2017

swgillespie Nov 3, 2017

swgillespie commented Nov 7, 2017

jkotas Nov 7, 2017

swgillespie Nov 7, 2017

swgillespie commented Nov 7, 2017

swgillespie commented Nov 7, 2017

swgillespie commented Nov 8, 2017

swgillespie commented Nov 8, 2017

swgillespie commented Nov 9, 2017

[Local GC] Unify background GC thread and server GC thread creation #14821

[Local GC] Unify background GC thread and server GC thread creation #14821

Conversation

swgillespie commented Nov 2, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Nov 2, 2017

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

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

Choose a reason for hiding this comment

swgillespie commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Nov 7, 2017

swgillespie commented Nov 7, 2017

swgillespie commented Nov 8, 2017

swgillespie commented Nov 8, 2017

swgillespie commented Nov 9, 2017