PR feedback: Add a runtime switch for the aggressive SCC merging

Previously every time we add a new runtime setting for the bridge
processor we have added a new callback for it. Because this is getting
unweildy, and also because some of these settings might have been
broken by recent init order changes, I created a new set_config
callback and merged the existing set_dump_prefix and enable_processing
with it.

diff --git a/mono/metadata/sgen-bridge-internals.h b/mono/metadata/sgen-bridge-internals.h
index 0597bdd7882fdd994391eadbe65c84ca58c0c181..a3d64be9bc1bc1b11dcd7c487495a115d7ae5e67 100644 (file)
--- a/mono/metadata/sgen-bridge-internals.h
+++ b/mono/metadata/sgen-bridge-internals.h
@@ -36,6 +36,12 @@ void sgen_mark_bridge_object (GCObject *obj);
 gboolean sgen_bridge_handle_gc_debug (const char *opt);
 void sgen_bridge_print_gc_debug_usage (void);
 
+typedef struct {
+       char *dump_prefix;
+       gboolean accounting;
+       gboolean scc_precise_merge; // Used by Tarjan
+} SgenBridgeProcessorConfig;
+
 typedef struct {
        void (*reset_data) (void);
        void (*processing_stw_step) (void);
@@ -44,10 +50,9 @@ typedef struct {
        MonoGCBridgeObjectKind (*class_kind) (MonoClass *klass);
        void (*register_finalized_object) (GCObject *object);
        void (*describe_pointer) (GCObject *object);
-       void (*enable_accounting) (void);
 
-       // Optional-- used for debugging
-       void (*set_dump_prefix) (const char *prefix);
+       /* Should be called once, immediately after init */
+       void (*set_config) (const SgenBridgeProcessorConfig *);
 
        /*
         * These are set by processing_build_callback_data().

diff --git a/mono/metadata/sgen-bridge.c b/mono/metadata/sgen-bridge.c
index 006e68c50121f32e9de1a776a9f164ead82907dd..8cff504267f0498fdb3d304838094c9722ed2dc7 100644 (file)
--- a/mono/metadata/sgen-bridge.c
+++ b/mono/metadata/sgen-bridge.c
@@ -33,6 +33,8 @@ typedef enum {
 static BridgeProcessorSelection bridge_processor_selection = BRIDGE_PROCESSOR_DEFAULT;
 // Most recently requested callbacks
 static MonoGCBridgeCallbacks pending_bridge_callbacks;
+// Configuration to be passed to bridge processor after init
+static SgenBridgeProcessorConfig bridge_processor_config;
 // Currently-in-use callbacks
 MonoGCBridgeCallbacks bridge_callbacks;
 
@@ -84,6 +86,12 @@ bridge_processor_name (const char *name)
        }
 }
 
+static gboolean
+bridge_processor_started ()
+{
+       return bridge_processor.reset_data != NULL;
+}
+
 // Initialize a single bridge processor
 static void
 init_bridge_processor (SgenBridgeProcessor *processor, BridgeProcessorSelection selection)
@@ -133,9 +141,17 @@ sgen_init_bridge ()
                bridge_callbacks = pending_bridge_callbacks;
 
                // If a bridge was registered but there is no bridge processor yet
-               if (bridge_callbacks.cross_references && !bridge_processor.reset_data)
+               if (bridge_callbacks.cross_references && !bridge_processor_started ()) {
                        init_bridge_processor (&bridge_processor, bridge_processor_selection);
 
+                       if (bridge_processor.set_config)
+                               bridge_processor.set_config (&bridge_processor_config);
+
+                       // Config is no longer needed so free its memory
+                       free (bridge_processor_config.dump_prefix);
+                       bridge_processor_config.dump_prefix = NULL;
+               }
+
                sgen_gc_unlock ();
        }
 }
@@ -147,7 +163,7 @@ sgen_set_bridge_implementation (const char *name)
 
        if (selection == BRIDGE_PROCESSOR_INVALID)
                g_warning ("Invalid value for bridge processor implementation, valid values are: 'new', 'old' and 'tarjan'.");
-       else if (bridge_processor.reset_data)
+       else if (bridge_processor_started ())
                g_warning ("Cannot set bridge processor implementation once bridge has already started");
        else
                bridge_processor_selection = selection;
@@ -480,12 +496,9 @@ sgen_bridge_describe_pointer (GCObject *obj)
 static void
 set_dump_prefix (const char *prefix)
 {
-       if (!bridge_processor.set_dump_prefix) {
-               fprintf (stderr, "Warning: Bridge implementation does not support dumping - ignoring.\n");
-               return;
-       }
-
-       bridge_processor.set_dump_prefix (prefix);
+       if (bridge_processor_config.dump_prefix)
+               free (bridge_processor_config.dump_prefix);
+       bridge_processor_config.dump_prefix = strdup (prefix);
 }
 
 /* Test support code */
@@ -655,19 +668,24 @@ register_test_bridge_callbacks (const char *bridge_class_name)
 gboolean
 sgen_bridge_handle_gc_debug (const char *opt)
 {
+       assert (!bridge_processor_started ());
+
        if (g_str_has_prefix (opt, "bridge=")) {
                opt = strchr (opt, '=') + 1;
                register_test_bridge_callbacks (g_strdup (opt));
        } else if (!strcmp (opt, "enable-bridge-accounting")) {
-               bridge_processor.enable_accounting ();
+               bridge_processor_config.accounting = TRUE;
+       } else if (!strcmp (opt, "enable-tarjan-precise-merge")) {
+               bridge_processor_config.scc_precise_merge = TRUE;
        } else if (g_str_has_prefix (opt, "bridge-dump=")) {
                char *prefix = strchr (opt, '=') + 1;
-               set_dump_prefix (prefix);
+               set_dump_prefix(prefix);
        } else if (g_str_has_prefix (opt, "bridge-compare-to=")) {
                const char *name = strchr (opt, '=') + 1;
                BridgeProcessorSelection selection = bridge_processor_name (name);
 
                if (selection != BRIDGE_PROCESSOR_INVALID) {
+                       // Compare processor doesn't get config
                        init_bridge_processor (&compare_to_bridge_processor, selection);
                } else {
                        g_warning ("Invalid bridge implementation to compare against - ignoring.");

diff --git a/mono/metadata/sgen-new-bridge.c b/mono/metadata/sgen-new-bridge.c
index 4631c0c5430f2ec946931fee01fee5ae079a6ad7..501ceb76d5b8c5a987d26b28891e01ca701cdb53 100644 (file)
--- a/mono/metadata/sgen-new-bridge.c
+++ b/mono/metadata/sgen-new-bridge.c
@@ -101,6 +101,8 @@ typedef struct _SCC {
 #endif
 } SCC;
 
+static char *dump_prefix = NULL;
+
 // Maps managed objects to corresponding HashEntry stricts
 static SgenHashTable hash_table = SGEN_HASH_TABLE_INIT (INTERNAL_MEM_BRIDGE_HASH_TABLE, INTERNAL_MEM_BRIDGE_HASH_TABLE_ENTRY, sizeof (HashEntry), mono_aligned_addr_hash, NULL);
 
@@ -161,11 +163,16 @@ dyn_array_int_contains (DynIntArray *da, int x)
 #endif
 
 static void
-enable_accounting (void)
+set_config (const SgenBridgeProcessorConfig *config)
 {
-       SgenHashTable table = SGEN_HASH_TABLE_INIT (INTERNAL_MEM_BRIDGE_HASH_TABLE, INTERNAL_MEM_BRIDGE_HASH_TABLE_ENTRY, sizeof (HashEntryWithAccounting), mono_aligned_addr_hash, NULL);
-       bridge_accounting_enabled = TRUE;
-       hash_table = table;
+       if (config->accounting) {
+               SgenHashTable table = SGEN_HASH_TABLE_INIT (INTERNAL_MEM_BRIDGE_HASH_TABLE, INTERNAL_MEM_BRIDGE_HASH_TABLE_ENTRY, sizeof (HashEntryWithAccounting), mono_aligned_addr_hash, NULL);
+               bridge_accounting_enabled = TRUE;
+               hash_table = table;
+       }
+       if (config->dump_prefix) {
+               dump_prefix = strdup (config->dump_prefix);
+       }
 }
 
 static MonoGCBridgeObjectKind
@@ -604,8 +611,6 @@ reset_flags (SCC *scc)
 }
 #endif
 
-static char *dump_prefix = NULL;
-
 static void
 dump_graph (void)
 {
@@ -657,12 +662,6 @@ dump_graph (void)
        fclose (file);
 }
 
-static void
-set_dump_prefix (const char *prefix)
-{
-       dump_prefix = strdup (prefix);
-}
-
 static int
 compare_hash_entries (const HashEntry *e1, const HashEntry *e2)
 {
@@ -1087,8 +1086,7 @@ sgen_new_bridge_init (SgenBridgeProcessor *collector)
        collector->class_kind = class_kind;
        collector->register_finalized_object = register_finalized_object;
        collector->describe_pointer = describe_pointer;
-       collector->enable_accounting = enable_accounting;
-       collector->set_dump_prefix = set_dump_prefix;
+       collector->set_config = set_config;
 
        bridge_processor = collector;
 }

diff --git a/mono/metadata/sgen-old-bridge.c b/mono/metadata/sgen-old-bridge.c
index d33d5d86815d795db6168cf97b9e0f24b9c3f1ff..346e4b0aeb50a1aff3d8ada64bf8b2042898dd17 100644 (file)
--- a/mono/metadata/sgen-old-bridge.c
+++ b/mono/metadata/sgen-old-bridge.c
@@ -372,11 +372,13 @@ dyn_array_int_merge_one (DynIntArray *array, int value)
 
 
 static void
-enable_accounting (void)
+set_config (const SgenBridgeProcessorConfig *config)
 {
-       SgenHashTable table = SGEN_HASH_TABLE_INIT (INTERNAL_MEM_BRIDGE_HASH_TABLE, INTERNAL_MEM_BRIDGE_HASH_TABLE_ENTRY, sizeof (HashEntryWithAccounting), mono_aligned_addr_hash, NULL);
-       bridge_accounting_enabled = TRUE;
-       hash_table = table;
+       if (config->accounting) {
+               SgenHashTable table = SGEN_HASH_TABLE_INIT (INTERNAL_MEM_BRIDGE_HASH_TABLE, INTERNAL_MEM_BRIDGE_HASH_TABLE_ENTRY, sizeof (HashEntryWithAccounting), mono_aligned_addr_hash, NULL);
+               bridge_accounting_enabled = TRUE;
+               hash_table = table;
+       }
 }
 
 static MonoGCBridgeObjectKind
@@ -922,7 +924,7 @@ sgen_old_bridge_init (SgenBridgeProcessor *collector)
        collector->class_kind = class_kind;
        collector->register_finalized_object = register_finalized_object;
        collector->describe_pointer = describe_pointer;
-       collector->enable_accounting = enable_accounting;
+       collector->set_config = set_config;
 
        bridge_processor = collector;
 }

diff --git a/mono/metadata/sgen-tarjan-bridge.c b/mono/metadata/sgen-tarjan-bridge.c
index c333fb420f87f398eb6cc21e0fc2a30fbec86dcd..daa983e64ac61ce1a29d534c4f2785d872e5e3ce 100644 (file)
--- a/mono/metadata/sgen-tarjan-bridge.c
+++ b/mono/metadata/sgen-tarjan-bridge.c
@@ -39,13 +39,6 @@
  *     which colors. The color graph then becomes the reduced SCC graph.
  */
 
-static void
-enable_accounting (void)
-{
-       // bridge_accounting_enabled = TRUE;
-       // hash_table = (SgenHashTable)SGEN_HASH_TABLE_INIT (INTERNAL_MEM_BRIDGE_HASH_TABLE, INTERNAL_MEM_BRIDGE_HASH_TABLE_ENTRY, sizeof (HashEntryWithAccounting), mono_aligned_addr_hash, NULL);
-}
-
 // Is this class bridged or not, and should its dependencies be scanned or not?
 // The result of this callback will be cached for use by is_opaque_object later.
 static MonoGCBridgeObjectKind
@@ -432,9 +425,11 @@ Memory wise, 4/32 takes 512 and 8/128 takes 8k, so it's quite reasonable.
 #define ELEMENTS_PER_BUCKET 8
 #define COLOR_CACHE_SIZE 128
 static HashEntry merge_cache [COLOR_CACHE_SIZE][ELEMENTS_PER_BUCKET];
-
 static unsigned int hash_perturb;
 
+// If true, disable an optimization where sometimes SCC nodes are merged without a perfect check
+static gboolean scc_precise_merge;
+
 static unsigned int
 mix_hash (uintptr_t source)
 {
@@ -459,7 +454,10 @@ static void
 reset_cache (void)
 {
        memset (merge_cache, 0, sizeof (merge_cache));
-       ++hash_perturb;
+
+       // When using the precise merge debug option, we do not want the inconsistency caused by hash_perturb.
+       if (!scc_precise_merge)
+               ++hash_perturb;
 }
 
 
@@ -494,6 +492,8 @@ match_colors (DynPtrArray *a, DynPtrArray *b)
        return TRUE;
 }
 
+// If scc_precise_merge, "semihits" refers to find_in_cache calls aborted because the merge array was too large.
+// Otherwise "semihits" refers to cache hits where the match was only estimated.
 static int cache_hits, cache_semihits, cache_misses;
 
 // The cache contains only non-bridged colors.
@@ -505,6 +505,21 @@ find_in_cache (int *insert_index)
 
        size = dyn_array_ptr_size (&color_merge_array);
 
+       /* Color equality checking is very expensive with a lot of elements, so when there are many
+        * elements we switch to a cheap comparison method which allows false positives. When false
+        * positives occur the worst that can happen is two items will be inappropriately merged
+        * and memory will be retained longer than it should be. We assume that will correct itself
+        * on the next GC (the hash_perturb mechanism increases the probability of this).
+        *
+        * Because this has *some* potential to create problems, if the user set the debug option
+        * 'enable-tarjan-precise-merge' we bail out early (and never merge SCCs with >3 colors).
+        */
+       gboolean color_merge_array_large = size > 3;
+       if (scc_precise_merge && color_merge_array_large) {
+               ++cache_semihits;
+               return NULL;
+       }
+
        unsigned int hash = color_merge_array_hash;
        if (!hash) // 0 is used to indicate an empty bucket entry
                hash = 1;
@@ -515,13 +530,7 @@ find_in_cache (int *insert_index)
                if (bucket [i].hash != hash)
                        continue;
 
-               /* Color equality checking is very expensive with a lot of elements, so when there are many
-                * elements we switch to a cheap comparison method which allows false positives. When false
-                * positives occur the worst that can happen is two items will be inappropriately merged
-                * and memory will be retained longer than it should be. We assume that will correct itself
-                * on the next GC (the hash_perturb mechanism increases the probability of this).
-                */
-               if (size > 3) {
+               if (color_merge_array_large) {
                        if (match_colors_estimate (&bucket [i].color->other_colors, &color_merge_array)) {
                                ++cache_semihits;
                                return bucket [i].color;
@@ -1159,10 +1168,10 @@ processing_after_callback (int generation)
 
        cleanup_time = step_timer (&curtime);
 
-       mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_GC, "GC_TAR_BRIDGE bridges %d objects %d opaque %d colors %d colors-bridged %d colors-visible %d xref %d cache %d/%d/%d setup %.2fms tarjan %.2fms scc-setup %.2fms gather-xref %.2fms xref-setup %.2fms cleanup %.2fms",
+       mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_GC, "GC_TAR_BRIDGE bridges %d objects %d opaque %d colors %d colors-bridged %d colors-visible %d xref %d cache-hit %d cache-%s %d cache-miss %d setup %.2fms tarjan %.2fms scc-setup %.2fms gather-xref %.2fms xref-setup %.2fms cleanup %.2fms",
                bridge_count, object_count, ignored_objects,
                color_count, colors_with_bridges_count, num_sccs, xref_count,
-               cache_hits, cache_semihits, cache_misses,
+               cache_hits, (scc_precise_merge ? "abstain" : "semihit"), cache_semihits, cache_misses,
                setup_time / 10000.0f,
                tarjan_time / 10000.0f,
                scc_setup_time / 10000.0f,
@@ -1196,6 +1205,15 @@ describe_pointer (GCObject *obj)
        // printf ("  is visited: %d\n", (int)entry->is_visited);
 }
 
+static void
+set_config (const SgenBridgeProcessorConfig *config)
+{
+       if (config->scc_precise_merge) {
+               hash_perturb = 0;
+               scc_precise_merge = TRUE;
+       }
+}
+
 void
 sgen_tarjan_bridge_init (SgenBridgeProcessor *collector)
 {
@@ -1206,8 +1224,7 @@ sgen_tarjan_bridge_init (SgenBridgeProcessor *collector)
        collector->class_kind = class_kind;
        collector->register_finalized_object = register_finalized_object;
        collector->describe_pointer = describe_pointer;
-       collector->enable_accounting = enable_accounting;
-       // collector->set_dump_prefix = set_dump_prefix;
+       collector->set_config = set_config;
 
        sgen_register_fixed_internal_mem_type (INTERNAL_MEM_TARJAN_OBJ_BUCKET, BUCKET_SIZE);
        g_assert (sizeof (ObjectBucket) <= BUCKET_SIZE);