GC update from CoreCLR #2485

swgillespie · 2017-01-11T19:04:03Z

This PR contains code for CoreCLR PRs dotnet/coreclr#8605. dotnet/coreclr#8709, dotnet/coreclr#8568, dotnet/coreclr#8336, and dotnet/coreclr#8199.

Minimal modifications were required on the runtime side to accommodate these GC changes. The largest changes came from moving some GC diagnostics to the runtime (from Maoni's PR) - I am unsure about the changes that I brought to gcrhenv.cpp since I lack some of the context regarding the GC_PROFILING feature define, so I would welcome feedback on that. Other than that change, there should not be any surprises in this PR.

swgillespie · 2017-01-11T19:18:10Z

Looks like I'm still failing a test on OSX locally - will investigate.

jkotas · 2017-01-11T19:33:11Z

src/Native/gc/env/gcenv.base.h

@@ -96,7 +96,7 @@ inline HRESULT HRESULT_FROM_WIN32(unsigned long x)
 #define UNREFERENCED_PARAMETER(P)          (void)(P)

 #ifdef PLATFORM_UNIX
-#define  _vsnprintf vsnprintf
+#define _vsnprintf_s(string, sizeInBytes, count, format, args) vsnprintf(string, sizeInBytes, format, args)


I am surprised that this compiles - args can be variable number of arguments. Does this need to use some kind of open args syntax to work?

Looking at the usage it seems like args is always a va_list (and that seems consistent with the Windows CRT definition of _vsnprintf_s).

swgillespie · 2017-01-11T19:52:22Z

The failure is an assert in gc.cpp:

frame #4: 0x0000000100073ea5 Exceptions`WKS::gc_heap::gc1() + 5285 at gc.cpp:15366
   15363	    if (!settings.concurrent)
   15364	#endif //BACKGROUND_GC
   15365	    {
-> 15366	        assert(!!IsGCThread());
   15367	        adjust_ephemeral_limits();
   15368	    }
   15369

IsGCThread always returns false on CoreRT (https://github.com/dotnet/corert/blob/master/src/Native/Runtime/gcrhenv.cpp#L1350) so this assert isn't useful. I suppose I can either put it under a FEATURE_REDHAWK define or remove it.

cc @Maoni0 @sergiy-k @adityamandaleeka

jkotas · 2017-01-11T20:04:55Z

IsGCThread always returns false on CoreRT

Removing the assert sounds good to me.

Separately, I think it would be useful to get rid of IsGCThread method completely in the GC - it should be possible looking at the few places where it is used.

jkotas · 2017-01-11T20:09:07Z

src/Native/Runtime/gcrhenv.cpp

@@ -1148,6 +1167,180 @@ Thread* GCToEEInterface::CreateBackgroundThread(GCBackgroundThreadFunction threa
    return threadStubArgs.m_pThread;
 }

+void GCToEEInterface::DiagGCStart(int gen, bool isInduced)
+{
+#ifdef GC_PROFILING


I do not think that GC_PROFILING is ever defined in CoreRT/ProjectN. These methods should have empty implementations; or have just the event trace stuff in them.

jkotas · 2017-01-11T20:11:16Z

I am unsure about the changes that I brought to gcrhenv.cpp

You should build this change in the ProjectN tree and make sure that perfview for GC analysis still works. I do not think that what you have has the event tracing done correctly (because of it is under GC_PROFILING define).

Maoni0 · 2017-01-11T20:47:25Z

@jkotas why would you want to remove IsGCThread? it's useful - we want to be sure that certain things are only called on the GC threads or the thread that did the suspension.

jkotas · 2017-01-11T20:53:23Z

If you think IsGCThread is useful, it would be best for the GC to keep track of it itself. I do not see a good reason why the EE should keep track of it for the GC.

Maoni0 · 2017-01-11T21:45:55Z

the name is misleading - it's not just checking if it's a GC thread; it's also checking if it's the thread that did the suspension. so it's not that EE is keeping track of it - it's just a general util in the common header that checks for thread types and both GC and EE use. I do think it makes more sense for the GC side to keep track of it.

swgillespie · 2017-01-11T22:09:32Z

@jkotas I'm seeing that I'm going to need to make some modifications to ProjectN in order for this to compile (apparently I was not building correctly when I was validating this earlier) - should I close this PR and do a code review/check everything into ProjectN instead and let it propagate back to this repo?

jkotas · 2017-01-11T22:09:45Z

CoreCLR kept track of thread types for historic reasons, and we have bolted the GC thread types onto it. There is nothing like that in MRT because of it does not need it - that's why it has a dummy implementation of it.

jkotas · 2017-01-11T22:10:29Z

a code review/check everything into ProjectN instead and let it propagate back to this repo?

Yes, it would make things easier. (You can still use this PR to verify that what you get passes CI on Unix.)

jkotas · 2017-01-13T19:13:27Z

@swgillespie error: extra tokens at end of #endif directive

swgillespie · 2017-01-13T19:22:51Z

@jkotas fixed, thanks!

… number of GC changes done within the last month or two from CoreCLR to CoreRT. The only changes to anything in Native/gc are: 1. Move an assert under #ifndef FEATURE_REDHAWK since it only is a useful assert on CoreCLR (#2485 (comment)) 2. Add UNREFERENCED_PARAMETERs to things in gc.cpp and elsewhere that were producing warnings All other changes in Native/gc are directly from CoreCLR without modification. I have validated that I am able to do PerfView GC analysis with these changes (since the CoreCLR changes touched ETW eventing). [tfs-changeset: 1644465]

jkotas · 2017-01-13T20:29:39Z

Checked in via mirror

GC update from CoreCLR

8076164

dnfclas added the cla-already-signed label Jan 11, 2017

Repair the Unix build

a3d021c

jkotas reviewed Jan 11, 2017

View reviewed changes

swgillespie added 3 commits January 12, 2017 14:51

Fixes to build ProjectN

407a722

Code review feedback

87d2901

More code review feedback

618a61e

Fix trailing tokens after endif

b3cd955

jkotas closed this Jan 13, 2017

swgillespie deleted the gc-update branch January 13, 2017 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GC update from CoreCLR #2485

GC update from CoreCLR #2485

swgillespie commented Jan 11, 2017

swgillespie commented Jan 11, 2017

jkotas Jan 11, 2017

swgillespie Jan 11, 2017

jkotas Jan 11, 2017

swgillespie commented Jan 11, 2017 •

edited

Loading

jkotas commented Jan 11, 2017

jkotas Jan 11, 2017

swgillespie Jan 11, 2017

jkotas commented Jan 11, 2017

Maoni0 commented Jan 11, 2017

jkotas commented Jan 11, 2017

Maoni0 commented Jan 11, 2017

swgillespie commented Jan 11, 2017

jkotas commented Jan 11, 2017

jkotas commented Jan 11, 2017

jkotas commented Jan 13, 2017

swgillespie commented Jan 13, 2017

jkotas commented Jan 13, 2017

GC update from CoreCLR #2485

GC update from CoreCLR #2485

Conversation

swgillespie commented Jan 11, 2017

swgillespie commented Jan 11, 2017

jkotas Jan 11, 2017

Choose a reason for hiding this comment

swgillespie Jan 11, 2017

Choose a reason for hiding this comment

jkotas Jan 11, 2017

Choose a reason for hiding this comment

swgillespie commented Jan 11, 2017 • edited Loading

jkotas commented Jan 11, 2017

jkotas Jan 11, 2017

Choose a reason for hiding this comment

swgillespie Jan 11, 2017

Choose a reason for hiding this comment

jkotas commented Jan 11, 2017

Maoni0 commented Jan 11, 2017

jkotas commented Jan 11, 2017

Maoni0 commented Jan 11, 2017

swgillespie commented Jan 11, 2017

jkotas commented Jan 11, 2017

jkotas commented Jan 11, 2017

jkotas commented Jan 13, 2017

swgillespie commented Jan 13, 2017

jkotas commented Jan 13, 2017

swgillespie commented Jan 11, 2017 •

edited

Loading