move profiling and profiling/ETW shared diagnostics code out of gc.cpp, gcee.cpp and objecthandle.cpp by Maoni0 · Pull Request #8199 · dotnet/coreclr

Maoni0 · 2016-11-18T08:48:49Z

This is the first step in separating the diagnostics code out from gc.cpp (except GC's own diagnostics) for the local GC project. The goal for this commit is to not have code that requires intimate knowledge about diagnostics stuff (eg, profiling, ETW, stress log, perf counters) in gc.cpp. Some of the diagnostics stuff exists in gcee.cpp today so they will need to be refactored out later.

The rule is GC will call the diagnostics functions at various points to communicate
info and these diagnostics functions might call back into the GC if they require
initimate knowledge of the GC in order to get certain info. This way gc.cpp does
not need to know about details of the diagnostics components, eg, the profiling context;
and the diagnostics components do not need to know details about the GC.

Still WIP (I haven't taken care of the gcsample part, for example) but wanted to get some early feedback. Profiling and what Profiling and ETW shared were the most problematic part so that's what I separated out. I stopped at the rest of the ETW part as they are just firing events in a straightforward way. The plan was to keep GC ETW events on the GC side so it's probably not worthwhile to have a wrapper that just calls the FireEtw* functions in another file... I am open to suggestions.

Stress log is similar to ETW - we just have some simple stress log messages sprinkled in gc.cpp. perf counter stuff is all in gcee.cpp right now.

Maoni0 · 2016-11-18T08:56:55Z

@swgillespie @jkotas @noahfalk

jkotas · 2016-11-18T13:20:40Z

@@ -0,0 +1,26 @@
+#ifndef __GCDIAG_H__


License header

(All new files)

jkotas · 2016-11-18T13:23:23Z

@@ -0,0 +1,237 @@
+#include "gcdiag.h"


Should this file live under src/vm so that it is structured properly for the local GC?

jkotas · 2016-11-18T13:31:41Z

@@ -30623,34 +30500,21 @@ BOOL gc_heap::large_object_marked (uint8_t* o, BOOL clearp)
 }

 #if defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)


Remove all GC_PROFILING and FEATURE_EVENT_TRACE ifdefs from the GC?

I still have checks for GC_PROFILING and FEATURE_EVENT_TRACE in gc.cpp because I don't want to include the code that does stuff only with diagnostics if those are not defined. but I am not opposed to unconditionally compile this in 'cause we almost always have FEATURE_EVENT_TRACE defined.

jkotas · 2016-11-18T13:33:21Z

+#ifndef __GCDIAG_H__
+#define __GCDIAG_H__
+
+typedef void (* record_surv_fn)(uint8_t*, uint8_t*, ptrdiff_t, size_t, BOOL, BOOL);


BOOL -> bool. (The GC interfaces should be using the native C++ primitive types, not the Windows-specific ones.)

jkotas · 2016-11-18T13:44:39Z

+    static void UpdateGenerationBounds();
+    static void GCEnd(size_t index, int gen, int reason, bool fConcurrent);
+
+    static void WalkFReachableObjects(gc_heap* hp);


This should not be using internal GC types.

(Maybe I do not understand what kind of separation is this change is trying to achieve.)

adityamandaleeka · 2016-11-18T21:24:33Z

+void GCDiagnostics::WalkLOHSurvivors(gc_heap* hp)
+{
+#if defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)
+    if (ShouldTrackMovementForProfilerOrEtw())


Minor comment: can we make this (and the other similar ones) be

if (!ShouldTrackMovementForProfilerOrEtw()) return;

to avoid a level of nesting?

Maoni0 · 2016-11-18T22:30:04Z

so as I mentioned this is an early version - it's not polished (things like license header/not exposing gc internal types will come later). I mainly wanted to get the diag specific code out of gc.cpp and a feel of what things should be on this interface. Jan and I talked, so the ETW/stress log messages will stay the way they are in gc.cpp.

noahfalk · 2016-11-19T01:56:37Z

-void CFinalize::WalkFReachableObjects (gc_heap* hp)
+void CFinalize::WalkFReachableObjects (fq_walk_fn fn)
 {
    BEGIN_PIN_PROFILER(CORProfilerPresent());


BEGIN_PIN_PROFILER(CORProfilerPresent()); [](start = 4, length = 41)

I was just poking around and I didn't see any path to get here other than a call from GCDiagnostics::WalkFReachableObjects(gc_heap* hp). Assuming I didn't miss one the profiler was already pinned and doesn't need to be pinned a 2nd time.

ahh, yes, I missed deleting the one in CFinalize::WalkFReachableObjects. thanks!

noahfalk · 2016-11-19T02:37:54Z

I chatted with Maoni offline as well, I think the overall direction is good and beyond that I didn't scrutinize too closely because the changes are still evolving.

Maoni0 · 2016-11-19T02:59:49Z

so I've moved the gcdiag.* onto the vm side and exposed the necessary things from GC on the IGCHeap interface.

for the GCProfileWalkHeap code in gcee.cpp, which is shared by ETW and profiling, I am going to move the bulk of it into gcdiag.cpp so it will live in clr.dll/coreclr.dll, not gc.dll (and again expose what it needs from the GC on the IGCHeap interface). please let me know if you disagree!

I'll send another commit out soon.

swgillespie · 2016-11-19T21:10:56Z


 typedef BOOL (* walk_fn)(Object*, void*);
 typedef void (* gen_walk_fn)(void* context, int generation, uint8_t* range_start, uint8_t* range_end, uint8_t* range_reserved);
+typedef void (* record_surv_fn)(uint8_t* begin, uint8_t* end, ptrdiff_t reloc, size_t context, BOOL compacting_p, BOOL bgc_p);


use c++ bool here and below instead of BOOL?

Also - after moving things to the VM side, do you think we still need ScanContext and ProfilingScanContext to be on the GC interface anymore?

Maoni0 · 2016-11-20T22:55:41Z

regarding BOOL vs bool, we are already using BOOL's all over the place in gcinterface.h so I would prefer that we do that in a separate commit.

Maoni0 · 2016-11-22T07:13:16Z

@swgillespie @jkotas @noahfalk @adityamandaleeka @sergiy-k PTAL. This does have functional changes to make things cleaner and more efficient (mostly to make it cleaner so I don't need to expose unnecessary things on the GC interface. improving the perf of profiling was not a goal here but it ended up having that effect).

Note I changed ProfScanRootsHelper to not return in this case:

    if ((o < hp->gc_low) || (o >= hp->gc_high))

this really doesn't make sense - it's saying if it's an interior pointer we would only report it if it's within the condemned range but for all the other stack roots and all other kinds of roots we will report all of them without checking for this range. my change will make it report all interior pointers, which I think was the intended effect.

I ended up not adding new files as I didn't want to get yet another interface so I just added stuff to gcenv.ee.cpp and GCToEEInterface.

I got rid of places that checked for GC_PROFILING || FEATURE_EVENT_TRACE in gc.cpp. it's cleaner this way (yes we do compile in more code if you don't have either defined but we always have the latter defined anyway).

I left ETW only stuff, perf counter and stress log stuff the way it is. In order to build a standalone gc.dll obviously that needs some work. We can do those in a separate PR.

mjsabby · 2016-11-22T07:34:33Z

Maoni this accomplishes #7704 I imagine? I haven't looked at the code, but essentially generation info, start/stop, reason and if it concurrent or not you could supply to the profiling apis as well now without doing movement tracking?

Maoni0 · 2016-11-22T08:42:25Z

no...what kind of info is provided is no different from before. this is just to separate code out for the local GC project (where we can build GC as a standalone dll). for #7704 you need to add a new mask to the profiling api where it reads in what info you want; essentially you want to split COR_PRF_MONITOR_GC
into a lightweight mask (that only does GC start/stop and a bit of other info) and a heavyweight one (that has the object survival info/relocate info/root info/etc). if this is high priority for you we should just get it implemented.

jkotas

I just added stuff to gcenv.ee.cpp and GCToEEInterface

I like it.

yes we do compile in more code

Compiling (unrechable) code is fine. Compilers are pretty good at getting rid of it from the final binary these days.

jkotas · 2016-11-22T11:04:12Z

+    virtual void ScanFinalizeQueue(fq_scan_fn fn, void* context) = 0;
+
+    // Scan handles for profiling or ETW.
+    virtual void ScanHandlesForProfilerAndETW(handle_scan_fn fn, int gen_number, ScanContext* context) = 0;


It would be nice to not have ProfilerAndETW in the names. Call it just ScanHandles or DiagScanHandles?

Maybe have Diag prefix on all the diagnostic methods - to match the other direction?

jkotas · 2016-11-22T11:06:23Z

+{
+#if defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)
+    Object *pObj = *ppObject;
+#ifdef INTERIOR_POINTERS


This is internal GC define. It should not be used here - remove it here.

jkotas · 2016-11-22T11:12:15Z

+        {
+            GcScanRootsForProfilerAndETW(&ProfScanRootsHelper, max_generation, max_generation, &SC);
+            SC.dwEtwRootKind = kEtwGCRootKindFinalizer;
+            GCHeapUtilities::GetGCHeap()->ScanFinalizeQueue(&ProfScanRootsHelper, (void*)&SC);


Nit - (void*) should not be necessary here. (It is not in the other places in this method.)

jkotas · 2016-11-22T11:16:44Z

+    virtual void WalkHeap(walk_fn fn, void* context, int gen_number, BOOL walk_large_object_heap_p) = 0;
+
+    // Walks the survivors and get the relocation information if objects have moved.
+    virtual void WalkSurvivors(void* gc_context, record_surv_fn fn, size_t diag_context, walk_surv_type type) = 0;


diag_context should be void* - to match what is used for context everywhere else.

this is because it's that way in the profiling/ETW code....they use void* sometimes and size_t other times (different kinds of contexts) and I didn't change that.

In reply to: 89092893 [](ancestors = 89092893)

We should be creating issues for getting these things cleaned up - it would be unfortunate if we declared the GC interface to be stable with them.

agreed..I will create issues for things like this.

In reply to: 89250978 [](ancestors = 89250978)

swgillespie

Looks good to me!

swgillespie · 2016-11-22T18:16:28Z

+    static void DiagGCEnd(size_t index, int gen, int reason, bool fConcurrent);
+    static void DiagWalkFReachableObjects(void* gcContext);
+    static void DiagWalkSurvivors(void* gcContext);
+	static void DiagWalkLOHSurvivors(void* gcContext);


nit - spaces instead of a tab

swgillespie · 2016-11-22T18:23:47Z

+    void DiagGCEnd(size_t index, int gen, int reason, bool fConcurrent);
+    void DiagWalkFReachableObjects(void* gcContext);
+    void DiagWalkSurvivors(void* gcContext);
+	void DiagWalkLOHSurvivors(void* gcContext);


swgillespie · 2016-11-22T18:24:01Z

+    if (CORProfilerTrackGC())
+    {
+        BEGIN_PIN_PROFILER(CORProfilerPresent());
+		GCHeapUtilities::GetGCHeap()->WalkFinalizeQueue(gcContext, g_FQWalkFn);


swgillespie · 2016-11-22T18:34:24Z

+inline void GCToEEInterface::DiagGCStart(int gen, bool isInduced)
+{
+    assert(g_theGCToCLR != nullptr);
+    return g_theGCToCLR->DiagGCStart(gen, isInduced);


nit - these all return void so you don't need the return

swgillespie · 2016-11-22T18:45:14Z

+typedef void (* record_surv_fn)(uint8_t* begin, uint8_t* end, ptrdiff_t reloc, size_t context, BOOL compacting_p, BOOL bgc_p);
+typedef void (* fq_walk_fn)(BOOL, void*);
+typedef void (* fq_scan_fn)(Object** ppObject, ScanContext *pSC, uint32_t dwFlags);
+typedef void (* handle_scan_fn)(Object** pRef, Object* pSec, uint32_t flags, ScanContext* context, BOOL isDependent);


Are the contexts here derived from the void pointers passed to the interface below? If so, I'm wondering should we be enforcing that the user passes a ScanContext by making the context argument be one instead of a void pointer, or by making these function pointers accept void* as a context?

they are different contexts. I am passing ScanContext* where it should be a ScanContext.

In reply to: 89182964 [](ancestors = 89182964)

swgillespie · 2016-11-22T18:46:28Z

@dotnet-bot test Windows_NT x64 Checked Standalone GC please

adityamandaleeka · 2016-11-22T18:52:03Z

        saved_post_plug_reloc = temp;
    }

-#if defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)


👍 for removing ifdefs

adityamandaleeka · 2016-11-22T18:59:40Z

+    else if (type == walk_for_loh)
+        walk_survivors_for_loh (context, fn);
+    else
+        assert (!"unknow type!");


typo: unknown

adityamandaleeka · 2016-11-22T19:01:47Z

+{
+    if (type == walk_for_gc)
+        walk_survivors_relocation (context, fn);
 #if defined(BACKGROUND_GC) && defined(FEATURE_EVENT_TRACE)


Is this necessary to have? type won't be walk_for_bgc unless BGC is enabled anyway right?

I see walk_survivors_for_bgc is also under the BGC ifdef. If that's non-trivial to remove then feel free to ignore this comment.

adityamandaleeka · 2016-11-22T19:09:32Z

+void gc_heap::walk_heap (walk_fn fn, void* context, int gen_number, BOOL walk_large_object_heap_p)
+{
+#ifdef MULTIPLE_HEAPS
+    int hn = 0;


Minor: just use for (int hn = 0;... for clarity since hn is only used in the loop?

adityamandaleeka · 2016-11-22T19:13:47Z

+    virtual
+    void DiagGCEnd(size_t index, int gen, int reason, bool fConcurrent) = 0;
+
+    // During a GC after we discover what objects's finalizers should run, gives the diagnostics code a chance to run.


GC's own logging), gcee.cpp and objecthandle.cpp. The rule is GC will call the diagnostics functions at various points to communicate info and these diagnostics functions might call back into the GC if they require initimate knowledge of the GC in order to get certain info. This way gc.cpp does not need to know about details of the diagnostics components, eg, the profiling context; and the diagnostics components do not need to know details about the GC. So got rid of ProfilingScanContext in gcinterface.h and passed scanning functions to GC and handle table. Got rid of where it goes through things per heap as this is not something that diagnostics should care about, including going through stack roots per heap (which is only done in GC for scalability purposes but profiling is doing this on a single thread). This also makes it faster. Got rid of the checks for gc_low/high in ProfScanRootsHelper as this is knowledge profiling shouldn't have. And it was also incorrectly not including all interior pointer stack roots.

Maoni0 · 2016-11-23T04:11:21Z

@dotnet-bot test Windows_NT x64 Checked Standalone GC please

swgillespie · 2016-11-23T04:57:50Z

@dotnet-bot test Windows_NT Release standalone_gc

Maoni0 · 2016-11-23T22:00:25Z

BTW, I've addressed all your feedback in the latest commit. and am doing the final sanity check of the code and then will check in.

AlgorithmsAreCool · 2016-11-23T22:24:53Z

@Maoni0 Is there any public information about the Local GC effort that I could read? It sounds interesting.

Maoni0 · 2016-11-24T01:23:16Z

#8268 #8267

Maoni0 · 2016-11-24T01:29:15Z

@AlgorithmsAreCool the Local GC effort is for building the GC as its own dll so all the interaction it has with the rest of the runtime needs to communicate through interfaces. diagnostics code being a big part as we have so many different ways to do diagnostics and the diagnostics code tends to be very intertwined with the GC code. we can certainly open issues that explain what work is left that will serve as public info. I opened 2 yesterday related to diagnostics and referred to them above. we'll open more for other things that need to be done for this. please stay tuned.

Maoni0 · 2016-11-28T03:39:23Z

@dotnet-bot test Windows_NT x86 compatjit Checked Build and Test please

Maoni0 · 2016-11-29T21:18:14Z

@AlgorithmsAreCool we have started a project on this https://github.com/dotnet/coreclr/projects/3 to keep track of all the issues.

AlgorithmsAreCool · 2016-11-29T21:26:11Z

Is the greater vision to allow alternative GC implementations? Or just to further modularize the coreclr?

move profiling and profiling/ETW shared diagnostics code out of gc.cpp, gcee.cpp and objecthandle.cpp Commit migrated from dotnet/coreclr@38a0b15

dnfclas added the cla-already-signed label Nov 18, 2016

Maoni0 changed the title ~~[WIP - do not merge] move diagnostics code out of gc.cpp~~ [WIP - do not merge] move diagnostics code out of gc code Nov 18, 2016

Maoni0 changed the title ~~[WIP - do not merge] move diagnostics code out of gc code~~ [WIP - do not merge] move diagnostics code out of gc.cpp Nov 18, 2016

jkotas reviewed Nov 18, 2016

View reviewed changes

adityamandaleeka reviewed Nov 18, 2016

View reviewed changes

noahfalk reviewed Nov 19, 2016

View reviewed changes

Maoni0 force-pushed the local_gc branch from ea89333 to e16291d Compare November 19, 2016 05:27

swgillespie reviewed Nov 19, 2016

View reviewed changes

Maoni0 force-pushed the local_gc branch 5 times, most recently from f3429f5 to 73c7910 Compare November 22, 2016 07:07

Maoni0 changed the title ~~[WIP - do not merge] move diagnostics code out of gc.cpp~~ move profiling and profiling/ETW shared diagnostics code out of gc.cpp, gcee.cpp and objecthandle.cpp Nov 22, 2016

jkotas reviewed Nov 22, 2016

View reviewed changes

swgillespie reviewed Nov 22, 2016

View reviewed changes

adityamandaleeka reviewed Nov 22, 2016

View reviewed changes

Maoni0 force-pushed the local_gc branch from 73c7910 to 1af5714 Compare November 23, 2016 04:10

Maoni0 merged commit 38a0b15 into dotnet:master Nov 28, 2016

swgillespie mentioned this pull request Jan 11, 2017

GC update from CoreCLR dotnet/corert#2485

Closed

karelz modified the milestone: 2.0.0 Aug 28, 2017

swgillespie mentioned this pull request Jan 31, 2020

Advanced GC Internals Documentation dotnet/runtime#7743

Closed

		@@ -30623,34 +30500,21 @@ BOOL gc_heap::large_object_marked (uint8_t* o, BOOL clearp)
		}

		#if defined(GC_PROFILING) \|\| defined(FEATURE_EVENT_TRACE)

Conversation

Maoni0 commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maoni0 commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Maoni0 commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahfalk commented Nov 19, 2016

Uh oh!

Maoni0 commented Nov 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swgillespie Nov 19, 2016 • 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!

Maoni0 commented Nov 20, 2016

Uh oh!

Maoni0 commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjsabby commented Nov 22, 2016

Uh oh!

Maoni0 commented Nov 22, 2016

Uh oh!

jkotas left a comment

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!

swgillespie left a comment

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

Maoni0 commented Nov 18, 2016 •

edited

Loading

Maoni0 commented Nov 18, 2016 •

edited

Loading

Maoni0 commented Nov 19, 2016 •

edited

Loading

swgillespie Nov 19, 2016 •

edited

Loading

Maoni0 commented Nov 22, 2016 •

edited

Loading

adityamandaleeka Nov 22, 2016 •

edited

Loading