[Gc] Turning off GC_parallel should be equivalent to ndef PARALLEL_MARK

Ivan Maidanski ivmai at mail.ru
Tue Oct 28 10:34:14 PST 2008


Hi!

Running GC with GC_markers == 1 is not the same (and a bit slower) than running GC configured without PARALLEL_MARK. Even more, in case of thread local allocations, it sometimes waits (uselessly, though for short periods) in GC_acquire_mark_lock().

This patch does the following:
- adds "if (GC_parallel)" statement in every "ifdef PARALLEL_MARK" block;
- replaces most "ifdef PARALLEL_MARK or THREAD_LOCAL_ALLOC" with "ifdef PARALLEL_MARK" since GC_fl_builder_count, GC_notify_all_builder(), GC_wait_for_reclaim(), GC_acquire/release_mark_lock() are useless unless PARALLEL_MARK (including HANDLE_FORK case);
- also: the length "marker_[b]sp" (in "pthread_support.c" file) is set to max-1 (since last element is unused) and the looping conditions in GC_segment_is_thread_stack/GC_greatest_stack_base_below() are changed accordingly.

Bye.

-------------- next part --------------
diff -ru bdwgc/alloc.c updated/bdwgc/alloc.c
--- bdwgc/alloc.c	2008-10-22 22:59:16.000000000 +0400
+++ updated/bdwgc/alloc.c	2008-10-28 17:26:16.000000000 +0300
@@ -273,7 +273,8 @@
             return;
         } else {
 #   	  ifdef PARALLEL_MARK
-	    GC_wait_for_reclaim();
+	    if (GC_parallel)
+	      GC_wait_for_reclaim();
 #   	  endif
 	  if (GC_need_full_gc || n_partial_gcs >= GC_full_freq) {
 	    if (GC_print_stats) {
@@ -350,6 +351,7 @@
     /* In the find_leak case, we have to finish to guarantee that 	*/
     /* previously unmarked objects are not reported as leaks.		*/
 #       ifdef PARALLEL_MARK
+	  if (GC_parallel)
 	    GC_wait_for_reclaim();
 #       endif
  	if ((GC_find_leak || stop_func != GC_never_stop_func)
@@ -417,7 +419,8 @@
         	    GC_save_callers(GC_last_stack);
 #     		endif
 #		ifdef PARALLEL_MARK
-		    GC_wait_for_reclaim();
+		    if (GC_parallel)
+		      GC_wait_for_reclaim();
 #		endif
 		if (GC_n_attempts < MAX_PRIOR_ATTEMPTS
 		    && GC_time_limit != GC_TIME_UNLIMITED) {
@@ -647,7 +650,7 @@
       	  clear_mark_bit_from_hdr(hhdr, bit_no);
 #	  ifdef PARALLEL_MARK
 	    /* Appr. count, don't decrement to zero! */
-	    if (0 != n_marks) {
+	    if (0 != n_marks || !GC_parallel) {
               hhdr -> hb_n_marks = n_marks;
 	    }
 #	  else
diff -ru bdwgc/darwin_stop_world.c updated/bdwgc/darwin_stop_world.c
--- bdwgc/darwin_stop_world.c	2008-09-25 05:51:23.000000000 +0400
+++ updated/bdwgc/darwin_stop_world.c	2008-10-28 17:22:52.000000000 +0300
@@ -466,9 +466,11 @@
     /* required to acquire and release the GC lock before it starts,	*/
     /* and we have the lock.						*/
 #   ifdef PARALLEL_MARK
-      GC_acquire_mark_lock();
-      GC_ASSERT(GC_fl_builder_count == 0);
-      /* We should have previously waited for it to become zero. */
+      if (GC_parallel) {
+	GC_acquire_mark_lock();
+	GC_ASSERT(GC_fl_builder_count == 0);
+	/* We should have previously waited for it to become zero. */
+      }
 #   endif /* PARALLEL_MARK */
 
     /* Loop stopping threads until you have gone over the whole list
@@ -519,7 +521,8 @@
 #   endif
 
 #   ifdef PARALLEL_MARK
-      GC_release_mark_lock();
+      if (GC_parallel)
+	GC_release_mark_lock();
 #   endif
 #   if DEBUG_THREADS
       GC_printf("World stopped from 0x%lx\n", (unsigned long)my_thread);
diff -ru bdwgc/include/private/gc_priv.h updated/bdwgc/include/private/gc_priv.h
--- bdwgc/include/private/gc_priv.h	2008-10-28 11:38:26.000000000 +0300
+++ updated/bdwgc/include/private/gc_priv.h	2008-10-28 17:07:58.000000000 +0300
@@ -1962,7 +1962,7 @@
 # define GC_STATIC_ASSERT(expr) (void)sizeof(char[(expr)? 1 : -1])
 #endif
 
-# if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
+# if defined(PARALLEL_MARK)
     /* We need additional synchronization facilities from the thread	*/
     /* support.  We believe these are less performance critical		*/
     /* than the main garbage collector lock; standard pthreads-based	*/
@@ -1982,12 +1982,10 @@
      extern void GC_acquire_mark_lock();
      extern void GC_release_mark_lock();
      extern void GC_notify_all_builder();
-     /* extern void GC_wait_builder(); */
      extern void GC_wait_for_reclaim();
 
      extern word GC_fl_builder_count;	/* Protected by mark lock.	*/
-# endif /* PARALLEL_MARK || THREAD_LOCAL_ALLOC */
-# ifdef PARALLEL_MARK
+
      extern void GC_notify_all_marker();
      extern void GC_wait_marker();
      extern word GC_mark_no;		/* Protected by mark lock.	*/
diff -ru bdwgc/mallocx.c updated/bdwgc/mallocx.c
--- bdwgc/mallocx.c	2008-10-28 11:38:10.000000000 +0300
+++ updated/bdwgc/mallocx.c	2008-10-28 17:42:54.000000000 +0300
@@ -308,7 +308,7 @@
 	    GC_ASSERT(hhdr -> hb_sz == lb);
 	    hhdr -> hb_last_reclaimed = (unsigned short) GC_gc_no;
 #	    ifdef PARALLEL_MARK
-		{
+	      if (GC_parallel) {
 		  signed_word my_bytes_allocd_tmp = GC_bytes_allocd_tmp;
 
 		  GC_ASSERT(my_bytes_allocd_tmp >= 0);
@@ -321,11 +321,11 @@
 				(AO_t)(-my_bytes_allocd_tmp));
 		    GC_bytes_allocd += my_bytes_allocd_tmp;
 		  }
-		}
-		GC_acquire_mark_lock();
-		++ GC_fl_builder_count;
-		UNLOCK();
-		GC_release_mark_lock();
+		  GC_acquire_mark_lock();
+		  ++ GC_fl_builder_count;
+		  UNLOCK();
+		  GC_release_mark_lock();
+	      }
 #	    endif
 	    op = GC_reclaim_generic(hbp, hhdr, lb,
 				    ok -> ok_init, 0, &my_bytes_allocd);
@@ -336,30 +336,33 @@
 	      /* inaccurate.						*/
 	      GC_bytes_found += my_bytes_allocd;
 #	      ifdef PARALLEL_MARK
-		*result = op;
-		(void)AO_fetch_and_add(
+		if (GC_parallel) {
+		  *result = op;
+		  (void)AO_fetch_and_add(
 				(volatile AO_t *)(&GC_bytes_allocd_tmp),
 				(AO_t)(my_bytes_allocd));
+		  GC_acquire_mark_lock();
+		  -- GC_fl_builder_count;
+		  if (GC_fl_builder_count == 0) GC_notify_all_builder();
+		  GC_release_mark_lock();
+		  (void) GC_clear_stack(0);
+		  return;
+		}
+#	      endif
+	      GC_bytes_allocd += my_bytes_allocd;
+	      goto out;
+	    }
+#	    ifdef PARALLEL_MARK
+	      if (GC_parallel) {
 		GC_acquire_mark_lock();
 		-- GC_fl_builder_count;
 		if (GC_fl_builder_count == 0) GC_notify_all_builder();
 		GC_release_mark_lock();
-		(void) GC_clear_stack(0);
-		return;
-#	      else
-	        GC_bytes_allocd += my_bytes_allocd;
-	        goto out;
-#	      endif
-	    }
-#	    ifdef PARALLEL_MARK
-	      GC_acquire_mark_lock();
-	      -- GC_fl_builder_count;
-	      if (GC_fl_builder_count == 0) GC_notify_all_builder();
-	      GC_release_mark_lock();
-	      LOCK();
-	      /* GC lock is needed for reclaim list access.	We	*/
-	      /* must decrement fl_builder_count before reaquiring GC	*/
-	      /* lock.  Hopefully this path is rare.			*/
+		LOCK();
+		/* GC lock is needed for reclaim list access.	We	*/
+		/* must decrement fl_builder_count before reaquiring GC	*/
+		/* lock.  Hopefully this path is rare.			*/
+	      }
 #	    endif
     	}
     }
@@ -388,24 +391,26 @@
 	  if (IS_UNCOLLECTABLE(k)) GC_set_hdr_marks(HDR(h));
 	  GC_bytes_allocd += HBLKSIZE - HBLKSIZE % lb;
 #	  ifdef PARALLEL_MARK
-	    GC_acquire_mark_lock();
-	    ++ GC_fl_builder_count;
-	    UNLOCK();
-	    GC_release_mark_lock();
-#	  endif
+	    if (GC_parallel) {
+	      GC_acquire_mark_lock();
+	      ++ GC_fl_builder_count;
+	      UNLOCK();
+	      GC_release_mark_lock();
 
-	  op = GC_build_fl(h, lw, (ok -> ok_init || GC_debugging_started), 0);
-#	  ifdef PARALLEL_MARK
-	    *result = op;
-	    GC_acquire_mark_lock();
-	    -- GC_fl_builder_count;
-	    if (GC_fl_builder_count == 0) GC_notify_all_builder();
-	    GC_release_mark_lock();
-	    (void) GC_clear_stack(0);
-	    return;
-#	  else
-	    goto out;
+	      op = GC_build_fl(h, lw,
+			(ok -> ok_init || GC_debugging_started), 0);
+	    
+	      *result = op;
+	      GC_acquire_mark_lock();
+	      -- GC_fl_builder_count;
+	      if (GC_fl_builder_count == 0) GC_notify_all_builder();
+	      GC_release_mark_lock();
+	      (void) GC_clear_stack(0);
+	      return;
+	    }
 #	  endif
+	  op = GC_build_fl(h, lw, (ok -> ok_init || GC_debugging_started), 0);
+	  goto out;
 	}
     }
     
diff -ru bdwgc/mark.c updated/bdwgc/mark.c
--- bdwgc/mark.c	2008-10-27 15:45:42.000000000 +0300
+++ updated/bdwgc/mark.c	2008-10-28 17:30:36.000000000 +0300
@@ -215,7 +215,7 @@
       clear_mark_bit_from_hdr(hhdr, bit_no);
       n_marks = hhdr -> hb_n_marks - 1;
 #     ifdef PARALLEL_MARK
-        if (n_marks != 0)
+        if (n_marks != 0 || !GC_parallel)
           hhdr -> hb_n_marks = n_marks; 
         /* Don't decrement to zero.  The counts are approximate due to	*/
         /* concurrency issues, but we need to ensure that a count of 	*/
@@ -1308,7 +1308,7 @@
 #ifdef PARALLEL_MARK
     /* Break up root sections into page size chunks to better spread 	*/
     /* out work.							*/
-    GC_bool GC_true_func(struct hblk *h) { return TRUE; }
+    STATIC GC_bool GC_true_func(struct hblk *h) { return TRUE; }
 #   define GC_PUSH_ALL(b,t) GC_push_selected(b,t,GC_true_func,GC_push_all);
 #else
 #   define GC_PUSH_ALL(b,t) GC_push_all(b,t);
diff -ru bdwgc/pthread_stop_world.c updated/bdwgc/pthread_stop_world.c
--- bdwgc/pthread_stop_world.c	2008-10-25 15:42:08.000000000 +0400
+++ updated/bdwgc/pthread_stop_world.c	2008-10-28 17:23:20.000000000 +0300
@@ -383,9 +383,11 @@
     /* required to acquire and release the GC lock before it starts,	*/
     /* and we have the lock.						*/
 #   ifdef PARALLEL_MARK
-      GC_acquire_mark_lock();
-      GC_ASSERT(GC_fl_builder_count == 0);
-      /* We should have previously waited for it to become zero. */
+      if (GC_parallel) {
+	GC_acquire_mark_lock();
+	GC_ASSERT(GC_fl_builder_count == 0);
+	/* We should have previously waited for it to become zero. */
+      }
 #   endif /* PARALLEL_MARK */
     AO_store(&GC_stop_count, GC_stop_count+1);
     	/* Only concurrent reads are possible. */
@@ -432,7 +434,8 @@
 	  }
     }
 #   ifdef PARALLEL_MARK
-      GC_release_mark_lock();
+      if (GC_parallel)
+	GC_release_mark_lock();
 #   endif
 #   if DEBUG_THREADS
       GC_printf("World stopped from 0x%x\n", (unsigned)pthread_self());
diff -ru bdwgc/pthread_support.c updated/bdwgc/pthread_support.c
--- bdwgc/pthread_support.c	2008-10-25 15:46:54.000000000 +0400
+++ updated/bdwgc/pthread_support.c	2008-10-28 18:07:32.000000000 +0300
@@ -272,9 +272,9 @@
 #   define MAX_MARKERS 16
 # endif
 
-static ptr_t marker_sp[MAX_MARKERS] = {0};
+static ptr_t marker_sp[MAX_MARKERS - 1] = {0};
 #ifdef IA64
-  static ptr_t marker_bsp[MAX_MARKERS] = {0};
+  static ptr_t marker_bsp[MAX_MARKERS - 1] = {0};
 #endif
 
 void * GC_mark_thread(void * id)
@@ -506,7 +506,7 @@
     
     GC_ASSERT(I_HOLD_LOCK());
 #   ifdef PARALLEL_MARK
-      for (i = 0; i < GC_markers; ++i) {
+      for (i = 0; i < GC_markers - 1; ++i) {
 	if (marker_sp[i] > lo & marker_sp[i] < hi) return TRUE;
 #       ifdef IA64
 	  if (marker_bsp[i] > lo & marker_bsp[i] < hi) return TRUE;
@@ -540,7 +540,7 @@
     
     GC_ASSERT(I_HOLD_LOCK());
 #   ifdef PARALLEL_MARK
-      for (i = 0; i < GC_markers; ++i) {
+      for (i = 0; i < GC_markers - 1; ++i) {
 	if (marker_sp[i] > result && marker_sp[i] < bound)
 	  result = marker_sp[i];
       }
@@ -640,20 +640,23 @@
     /* Wait for an ongoing GC to finish, since we can't finish it in	*/
     /* the (one remaining thread in) the child.				*/
       LOCK();
-#     if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
-        GC_wait_for_reclaim();
+#     if defined(PARALLEL_MARK)
+	if (GC_parallel)
+          GC_wait_for_reclaim();
 #     endif
       GC_wait_for_gc_completion(TRUE);
-#     if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
-        GC_acquire_mark_lock();
+#     if defined(PARALLEL_MARK)
+	if (GC_parallel)
+          GC_acquire_mark_lock();
 #     endif
 }
 
 /* Called in parent after a fork()	*/
 STATIC void GC_fork_parent_proc(void)
 {
-#   if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
-      GC_release_mark_lock();
+#   if defined(PARALLEL_MARK)
+      if (GC_parallel)
+        GC_release_mark_lock();
 #   endif
     UNLOCK();
 }
@@ -662,8 +665,9 @@
 STATIC void GC_fork_child_proc(void)
 {
     /* Clean up the thread table, so that just our thread is left. */
-#   if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
-      GC_release_mark_lock();
+#   if defined(PARALLEL_MARK)
+      if (GC_parallel)
+        GC_release_mark_lock();
 #   endif
     GC_remove_all_threads_but_me();
 #   ifdef PARALLEL_MARK
@@ -1241,7 +1245,7 @@
                         /* extended period.                             */
 
 #if (!defined(USE_SPIN_LOCK) && !defined(NO_PTHREAD_TRYLOCK)) \
-	|| defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
+	|| defined(PARALLEL_MARK)
 /* If we don't want to use the below spinlock implementation, either	*/
 /* because we don't have a GC_test_and_set implementation, or because 	*/
 /* we don't want to risk sleeping, we can still try spinning on 	*/
@@ -1389,7 +1393,7 @@
 
 #endif /* !USE_SPINLOCK */
 
-#if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
+#ifdef PARALLEL_MARK
 
 #ifdef GC_ASSERTIONS
   unsigned long GC_mark_lock_holder = NO_THREAD;
@@ -1472,10 +1476,6 @@
     }
 }
 
-#endif /* PARALLEL_MARK || THREAD_LOCAL_ALLOC */
-
-#ifdef PARALLEL_MARK
-
 static pthread_cond_t mark_cv = PTHREAD_COND_INITIALIZER;
 
 void GC_wait_marker(void)
diff -ru bdwgc/reclaim.c updated/bdwgc/reclaim.c
--- bdwgc/reclaim.c	2008-10-25 16:15:37.000000000 +0400
+++ updated/bdwgc/reclaim.c	2008-10-28 17:07:08.000000000 +0300
@@ -22,7 +22,7 @@
 			/* minus the number of bytes originally	   */
 			/* on free lists which we had to drop.	   */
 
-#if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
+#if defined(PARALLEL_MARK)
   word GC_fl_builder_count = 0;
 	/* Number of threads currently building free lists without 	*/
 	/* holding GC lock.  It is not safe to collect if this is 	*/
@@ -477,7 +477,7 @@
 {
     unsigned kind;
     
-#   if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
+#   if defined(PARALLEL_MARK)
       GC_ASSERT(0 == GC_fl_builder_count);
 #   endif
     /* Reset in use counters.  GC_reclaim_block recomputes them. */
@@ -522,7 +522,7 @@
     /* so that you can convince yourself that it really is very stupid.	*/
     GC_reclaim_all((GC_stop_func)0, FALSE);
 # endif
-# if defined(PARALLEL_MARK) || defined(THREAD_LOCAL_ALLOC)
+# if defined(PARALLEL_MARK)
     GC_ASSERT(0 == GC_fl_builder_count);
 # endif
     
diff -ru bdwgc/win32_threads.c updated/bdwgc/win32_threads.c
--- bdwgc/win32_threads.c	2008-10-26 13:16:20.000000000 +0300
+++ updated/bdwgc/win32_threads.c	2008-10-28 18:10:51.000000000 +0300
@@ -753,9 +753,11 @@
 
   /* This code is the same as in pthread_stop_world.c */
 # ifdef PARALLEL_MARK
-    GC_acquire_mark_lock();
-    GC_ASSERT(GC_fl_builder_count == 0);
-    /* We should have previously waited for it to become zero. */
+    if (GC_parallel) {
+      GC_acquire_mark_lock();
+      GC_ASSERT(GC_fl_builder_count == 0);
+      /* We should have previously waited for it to become zero. */
+    }
 # endif /* PARALLEL_MARK */
 
   GC_please_stop = TRUE;
@@ -794,7 +796,8 @@
     LeaveCriticalSection(&GC_write_cs);
 # endif    
 # ifdef PARALLEL_MARK
-    GC_release_mark_lock();
+    if (GC_parallel)
+      GC_release_mark_lock();
 # endif
 }
 
@@ -1738,18 +1741,6 @@
     GC_ASSERT(sb_result == GC_SUCCESS);
     
 #   ifdef PARALLEL_MARK
-#     ifndef GC_PTHREADS
-	/* Initialize Win32 event objects for parallel marking		*/
-	/* (used even if parallel marking is set disabled at startup).	*/
-	mark_mutex_event = CreateEventA(NULL /* attrs */,
-				FALSE /* isManualReset */,
-				FALSE /* initialState */, NULL /* name */);
-	builder_cv = CreateEventA(NULL /* attrs */, TRUE /* isManualReset */,
-			FALSE /* initialState */, NULL /* name */);
-	if (mark_mutex_event == (HANDLE)0 || builder_cv == (HANDLE)0)
-	  ABORT("CreateEvent() failed");
-#     endif
-
       /* Set GC_markers. */
       {
 	char * markers_string = GETENV("GC_MARKERS");
@@ -1800,10 +1791,17 @@
 	  GC_markers = 1;
 	} else {
 #	  ifndef GC_PTHREADS
-	    /* Initialize mark_cv if it is really needed.	*/
+	    /* Initialize Win32 event objects for parallel marking.	*/
+	    mark_mutex_event = CreateEventA(NULL /* attrs */,
+				FALSE /* isManualReset */,
+				FALSE /* initialState */, NULL /* name */);
+	    builder_cv = CreateEventA(NULL /* attrs */,
+				TRUE /* isManualReset */,
+				FALSE /* initialState */, NULL /* name */);
 	    mark_cv = CreateEventA(NULL /* attrs */, TRUE /* isManualReset */,
 				FALSE /* initialState */, NULL /* name */);
-	    if (mark_cv == (HANDLE)0)
+	    if (mark_mutex_event == (HANDLE)0 || builder_cv == (HANDLE)0
+		|| mark_cv == (HANDLE)0)
 	      ABORT("CreateEvent() failed");
 #	  endif
 	  GC_parallel = TRUE;


More information about the Gc mailing list