[m-rev.] For review: Improve instrumentation of Boehm GC.

Paul Bone pbone at csse.unimelb.edu.au
Sun Nov 7 20:08:30 AEDT 2010


For review by Julien.

Julien, My modifications to Boehm GC head apply (mostly) cleanly to the current
version of the collector that we're using.  Should I commit this now or wait
for you to upgrade the collector?

Thanks.

---

Improve instrumentation of Boehm GC.

We've made a modification to the Boehm GC to allow us to profile programs more
easily.  This instrumented the GC code with a number of callbacks that are
called for interesting events.  This change was never thorough, and many places
that should have been instrumented where not.  This change completes this work
to the best of my knowledge, when I send these changes to the Boehm GC
maintainers I'll ask for their input.

This has been bootchecked in asm_fast.gc and asm_fast.gc.par.threadscope, this
coveres the two alternative sets of build options for the collector plus use of
the callbacks.

boehm_gc/alloc.c:
boehm_gc/gc_dlopen.c:
boehm_gc/include/gc.h:
boehm_gc/malloc.c:
boehm_gc/mallocx.c:
boehm_gc/misc.c:
boehm_gc/pthread_stop_world.c:
    Rename and modify the placement of callbacks our modification of the Boehm
    GC.  This patch will be sent upstream to make this easier to maintain.

boehm_gc/tests/test.c:
    Modify the main test case so that it checks the mutating -> collecting and
    collecting -> mutating transitions make sense.

runtime/mercury_threadscope.c:
    Conform to changes in boehm_gc/include/gc.h

Index: boehm_gc/alloc.c
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/alloc.c,v
retrieving revision 1.21
diff -u -p -b -r1.21 alloc.c
--- boehm_gc/alloc.c	1 Mar 2010 02:27:30 -0000	1.21
+++ boehm_gc/alloc.c	7 Nov 2010 05:17:02 -0000
@@ -64,12 +64,12 @@ GC_bool         GC_mercury_calc_gc_time 
 unsigned long 	GC_total_gc_time = 0;
 			   /* Measured in milliseconds.         */
 
-void (*GC_mercury_callback_start_collect)(void) = NULL;
-void (*GC_mercury_callback_stop_collect)(void) = NULL;
-void (*GC_mercury_callback_pause_thread)(void) = NULL;
-void (*GC_mercury_callback_resume_thread)(void) = NULL;
-                           /* Callbacks for mercury to notify   */
-                           /* the runtime of certain events     */
+void (*GC_callback_start_collect)(void) = NULL;
+void (*GC_callback_stop_collect)(void) = NULL;
+void (*GC_callback_pause_thread)(void) = NULL;
+void (*GC_callback_resume_thread)(void) = NULL;
+                           /* Callbacks for notifing the        */
+                           /* application of certain events     */
 
 #ifndef GC_DISABLE_INCREMENTAL
   GC_INNER int GC_incremental = 0;      /* By default, stop the world.  */
@@ -447,9 +447,6 @@ GC_INNER GC_bool GC_try_to_collect_inner
         GC_save_callers(GC_last_stack);
 #   endif
     GC_is_full_gc = TRUE;
-    if (GC_mercury_callback_start_collect) {
-      GC_mercury_callback_start_collect();
-    }
     if (!GC_stopped_mark(stop_func)) {
       if (!GC_incremental) {
         /* We're partially done and have no way to complete or use      */
@@ -475,9 +472,6 @@ GC_INNER GC_bool GC_try_to_collect_inner
             GC_total_gc_time += cur_gc_time;
 	}
     }
-    if (GC_mercury_callback_stop_collect) {
-      GC_mercury_callback_stop_collect();
-    }
 #   endif
     return(TRUE);
 }
@@ -552,10 +546,14 @@ GC_API int GC_CALL GC_collect_a_little(v
     int result;
     DCL_LOCK_STATE;
 
+    if (GC_callback_start_collect)
+      GC_callback_start_collect();
     LOCK();
     GC_collect_a_little_inner(1);
     result = (int)GC_collection_in_progress();
     UNLOCK();
+    if (GC_callback_stop_collect)
+      GC_callback_stop_collect();
     if (!result && GC_debugging_started) GC_print_all_smashed();
     return(result);
 }
@@ -989,21 +987,33 @@ STATIC GC_bool GC_try_to_collect_general
 /* Externally callable routines to invoke full, stop-the-world collection. */
 GC_API int GC_CALL GC_try_to_collect(GC_stop_func stop_func)
 {
+    if (GC_callback_start_collect)
+        GC_callback_start_collect();
     GC_ASSERT(stop_func != 0);
     return (int)GC_try_to_collect_general(stop_func, FALSE);
+    if (GC_callback_stop_collect)
+        GC_callback_stop_collect();
 }
 
 GC_API void GC_CALL GC_gcollect(void)
 {
     /* 0 is passed as stop_func to get GC_default_stop_func value       */
     /* while holding the allocation lock (to prevent data races).       */
+    if (GC_callback_start_collect)
+        GC_callback_start_collect();
     (void)GC_try_to_collect_general(0, FALSE);
     if (GC_have_errors) GC_print_all_errors();
+    if (GC_callback_stop_collect)
+        GC_callback_stop_collect();
 }
 
 GC_API void GC_CALL GC_gcollect_and_unmap(void)
 {
+    if (GC_callback_start_collect)
+        GC_callback_start_collect();
     (void)GC_try_to_collect_general(GC_never_stop_func, TRUE);
+    if (GC_callback_stop_collect)
+        GC_callback_stop_collect();
 }
 
 GC_INNER word GC_n_heap_sects = 0;
@@ -1229,10 +1239,16 @@ GC_INNER GC_bool GC_collect_or_expand(wo
     GC_bool gc_not_stopped = TRUE;
     word blocks_to_get;
     IF_CANCEL(int cancel_state;)
+    GC_bool called_start_collect_callback = FALSE;
 
     DISABLE_CANCEL(cancel_state);
     if (!GC_incremental && !GC_dont_gc &&
         ((GC_dont_expand && GC_bytes_allocd > 0) || GC_should_collect())) {
+      if (GC_callback_start_collect)
+      {
+        GC_callback_start_collect();
+        called_start_collect_callback = TRUE;
+      }
       /* Try to do a full collection using 'default' stop_func (unless  */
       /* nothing has been allocated since the latest collection or heap */
       /* expansion is disabled).                                        */
@@ -1243,6 +1259,8 @@ GC_INNER GC_bool GC_collect_or_expand(wo
         /* Either the collection hasn't been aborted or this is the     */
         /* first attempt (in a loop).                                   */
         RESTORE_CANCEL(cancel_state);
+        if (GC_callback_stop_collect)
+          GC_callback_stop_collect();
         return(TRUE);
       }
     }
@@ -1272,10 +1290,18 @@ GC_INNER GC_bool GC_collect_or_expand(wo
         && !GC_expand_hp_inner(needed_blocks)) {
       if (gc_not_stopped == FALSE) {
         /* Don't increment GC_fail_count here (and no warning).     */
+        if (!called_start_collect_callback && GC_callback_start_collect) {
+          GC_callback_start_collect();
+          called_start_collect_callback = TRUE;
+        }
         GC_gcollect_inner();
         GC_ASSERT(GC_bytes_allocd == 0);
       } else if (GC_fail_count++ < GC_max_retries) {
         WARN("Out of Memory!  Trying to continue ...\n", 0);
+        if (!called_start_collect_callback && GC_callback_start_collect) {
+          GC_callback_start_collect();
+          called_start_collect_callback = TRUE;
+        }
         GC_gcollect_inner();
       } else {
 #       if !defined(AMIGA) || !defined(GC_AMIGA_FASTALLOC)
@@ -1283,12 +1309,16 @@ GC_INNER GC_bool GC_collect_or_expand(wo
                " Returning NIL!\n", (GC_heapsize - GC_unmapped_bytes) >> 20);
 #       endif
         RESTORE_CANCEL(cancel_state);
+        if (called_start_collect_callback && GC_callback_stop_collect)
+          GC_callback_stop_collect();
         return(FALSE);
       }
     } else if (GC_fail_count && GC_print_stats) {
       GC_printf("Memory available again ...\n");
     }
     RESTORE_CANCEL(cancel_state);
+    if (called_start_collect_callback && GC_callback_stop_collect)
+      GC_callback_stop_collect();
     return(TRUE);
 }
 
@@ -1303,10 +1333,15 @@ GC_INNER ptr_t GC_allocobj(size_t gran, 
     void ** flh = &(GC_obj_kinds[kind].ok_freelist[gran]);
     GC_bool tried_minor = FALSE;
     GC_bool retry = FALSE;
+    GC_bool started_collect = FALSE;
 
     if (gran == 0) return(0);
 
     while (*flh == 0) {
+      if (GC_callback_start_collect) {
+        GC_callback_start_collect();
+        started_collect = TRUE;
+      }
       ENTER_GC();
       /* Do our share of marking work */
         if(TRUE_INCREMENTAL) GC_collect_a_little_inner(1);
@@ -1323,6 +1358,15 @@ GC_INNER ptr_t GC_allocobj(size_t gran, 
           GC_collect_a_little_inner(1);
           tried_minor = TRUE;
         } else {
+          /*
+           * Say that we've stopped collecting before we call
+           * GC_collect_or_expand since it might call GC_callback_start_collect
+           * itself.
+           */
+          if (started_collect && GC_callback_stop_collect) {
+            GC_callback_stop_collect();
+            started_collect = FALSE;
+          }
           if (!GC_collect_or_expand(1, FALSE, retry)) {
             EXIT_GC();
             return(0);
@@ -1335,5 +1379,8 @@ GC_INNER ptr_t GC_allocobj(size_t gran, 
     /* Successful allocation; reset failure count.      */
     GC_fail_count = 0;
 
+    if (started_collect && GC_callback_stop_collect)
+      GC_callback_stop_collect();
+
     return(*flh);
 }
Index: boehm_gc/gc_dlopen.c
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/gc_dlopen.c,v
retrieving revision 1.6
diff -u -p -b -r1.6 gc_dlopen.c
--- boehm_gc/gc_dlopen.c	24 Feb 2010 07:04:33 -0000	1.6
+++ boehm_gc/gc_dlopen.c	5 Nov 2010 04:37:28 -0000
@@ -50,11 +50,19 @@
   /* But I don't know of a better solution.                             */
   static void disable_gc_for_dlopen(void)
   {
+    GC_bool started_collection = FALSE;
+
     LOCK();
     while (GC_incremental && GC_collection_in_progress()) {
+        if (!started_collection && GC_callback_start_collect) {
+            GC_callback_start_collect();
+            started_collection = TRUE;
+        }
         GC_collect_a_little_inner(1000);
     }
     ++GC_dont_gc;
+    if (started_collection && GC_callback_stop_collect)
+      GC_callback_stop_collect();
     UNLOCK();
   }
 
Index: boehm_gc/malloc.c
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/malloc.c,v
retrieving revision 1.19
diff -u -p -b -r1.19 malloc.c
--- boehm_gc/malloc.c	1 Mar 2010 02:27:30 -0000	1.19
+++ boehm_gc/malloc.c	5 Nov 2010 04:34:57 -0000
@@ -56,8 +56,13 @@ GC_INNER ptr_t GC_alloc_large(size_t lb,
     n_blocks = OBJ_SZ_TO_BLOCKS(lb);
     if (!GC_is_initialized) GC_init();
     /* Do our share of marking work */
-        if (GC_incremental && !GC_dont_gc)
+        if (GC_incremental && !GC_dont_gc) {
+            if (GC_callback_start_collect)
+                GC_callback_start_collect();
             GC_collect_a_little_inner((int)n_blocks);
+            if (GC_callback_stop_collect)
+                GC_callback_stop_collect();
+        }
     h = GC_allochblk(lb, k, flags);
 #   ifdef USE_MUNMAP
         if (0 == h) {
Index: boehm_gc/mallocx.c
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/mallocx.c,v
retrieving revision 1.6
diff -u -p -b -r1.6 mallocx.c
--- boehm_gc/mallocx.c	24 Feb 2010 07:04:34 -0000	1.6
+++ boehm_gc/mallocx.c	5 Nov 2010 04:34:57 -0000
@@ -287,9 +287,13 @@ GC_API void GC_CALL GC_generic_malloc_ma
     if (!GC_is_initialized) GC_init();
     /* Do our share of marking work */
       if (GC_incremental && !GC_dont_gc) {
+        if (GC_callback_start_collect)
+            GC_callback_start_collect();
         ENTER_GC();
         GC_collect_a_little_inner(1);
         EXIT_GC();
+        if (GC_callback_stop_collect)
+            GC_callback_stop_collect();
       }
     /* First see if we can reclaim a page of objects waiting to be */
     /* reclaimed.                                                  */
Index: boehm_gc/misc.c
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/misc.c,v
retrieving revision 1.19
diff -u -p -b -r1.19 misc.c
--- boehm_gc/misc.c	24 Feb 2010 07:04:34 -0000	1.19
+++ boehm_gc/misc.c	5 Nov 2010 04:34:57 -0000
@@ -856,7 +856,13 @@ GC_API void GC_CALL GC_init(void)
 #   endif
     COND_DUMP;
     /* Get black list set up and/or incremental GC started */
-      if (!GC_dont_precollect || GC_incremental) GC_gcollect_inner();
+      if (!GC_dont_precollect || GC_incremental) {
+        if (GC_callback_start_collect)
+          GC_callback_start_collect();
+        GC_gcollect_inner();
+        if (GC_callback_stop_collect)
+          GC_callback_stop_collect();
+      }
 #   ifdef STUBBORN_ALLOC
         GC_stubborn_init();
 #   endif
@@ -913,7 +919,11 @@ GC_API void GC_CALL GC_enable_incrementa
                                 /* Can't easily do it if GC_dont_gc.    */
           if (GC_bytes_allocd > 0) {
             /* There may be unmarked reachable objects. */
+            if (GC_callback_start_collect)
+              GC_callback_start_collect();
             GC_gcollect_inner();
+            if (GC_callback_stop_collect)
+              GC_callback_stop_collect();
           }
             /* else we're OK in assuming everything's   */
             /* clean since nothing can point to an      */
Index: boehm_gc/pthread_stop_world.c
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/pthread_stop_world.c,v
retrieving revision 1.6
diff -u -p -b -r1.6 pthread_stop_world.c
--- boehm_gc/pthread_stop_world.c	24 Feb 2010 07:04:34 -0000	1.6
+++ boehm_gc/pthread_stop_world.c	6 Nov 2010 04:30:38 -0000
@@ -146,12 +146,12 @@ STATIC void GC_suspend_handler_inner(ptr
 #endif
 {
   int old_errno = errno;
-  if (GC_mercury_callback_pause_thread) {
-    GC_mercury_callback_pause_thread();
+  if (GC_callback_pause_thread) {
+    GC_callback_pause_thread();
   }
   GC_with_callee_saves_pushed(GC_suspend_handler_inner, (ptr_t)(word)sig);
-  if (GC_mercury_callback_resume_thread) {
-    GC_mercury_callback_resume_thread();
+  if (GC_callback_resume_thread) {
+    GC_callback_resume_thread();
   }
   errno = old_errno;
 }
@@ -165,15 +165,15 @@ STATIC void GC_suspend_handler_inner(ptr
 #endif
 {
   int old_errno = errno;
-  if (GC_mercury_callback_pause_thread) {
-    GC_mercury_callback_pause_thread();
+  if (GC_callback_pause_thread) {
+    GC_callback_pause_thread();
   }
 # ifndef SA_SIGINFO
     void *context = 0;
 # endif
   GC_suspend_handler_inner((ptr_t)(word)sig, context);
-  if (GC_mercury_callback_resume_thread) {
-    GC_mercury_callback_resume_thread();
+  if (GC_callback_resume_thread) {
+    GC_callback_resume_thread();
   }
   errno = old_errno;
 }
Index: boehm_gc/include/gc.h
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/include/gc.h,v
retrieving revision 1.22
diff -u -p -b -r1.22 gc.h
--- boehm_gc/include/gc.h	1 Mar 2010 02:27:29 -0000	1.22
+++ boehm_gc/include/gc.h	5 Nov 2010 04:38:30 -0000
@@ -337,25 +337,27 @@ GC_API unsigned long GC_total_gc_time;
 				/* so far by garbage collections. It is  */
 				/* measured in milliseconds.		 */
 
-/*
- * Callbacks for mercury to notify the runtime of certain events.
- */
-GC_API void (*GC_mercury_callback_start_collect)(void);
+/* Callbacks to notify the application of certain events.  These might  */
+/* be used for profiling.  The stop collect function will be called by  */
+/* the same thread that called start collect.  This will be the thread  */
+/* that decided to begin collection.  The pause_thread and              */
+/* resume_thread functions are called from each of the other mutator    */
+/* threads.                                                             */
+GC_API void (*GC_callback_start_collect)(void);
                 /* Starting a collection */
-GC_API void (*GC_mercury_callback_stop_collect)(void);
+GC_API void (*GC_callback_stop_collect)(void);
                 /* Stopping a collection */
-GC_API void (*GC_mercury_callback_pause_thread)(void);
-                /*
-                 * This thread is about to be paused.
-                 *
-                 * Use these with care!  They're called from a signal handler,
-                 * they must NOT allocate memory and if they do locking they
-                 * must use reentrant mutexes.  Also note that these do not
-                 * work on OS X/Darwin.  On Darwin the world is stopped in a
-                 * different way, we can't easily add support for these
-                 * callbacks on Darwin. 
-                 */
-GC_API void (*GC_mercury_callback_resume_thread)(void);
+GC_API void (*GC_callback_pause_thread)(void);
+                /* This thread is about to be paused.                   */
+
+                /* Use these with care!  They're called from a signal   */
+                /* handler, they must NOT allocate memory and if they   */
+                /* do locking they must use reentrant mutexes.  Also    */
+                /* note that these do not work on OS X/Darwin.  On      */
+                /* Darwin the world is stopped in a different way, we   */
+                /* can't easily add support for these callbacks on      */
+                /* Darwin.                                              */
+GC_API void (*GC_callback_resume_thread)(void);
                 /* This thread is about to be resumed */
 
 /* Public procedures */
Index: boehm_gc/tests/test.c
===================================================================
RCS file: /home/mercury1/repository/mercury/boehm_gc/tests/test.c,v
retrieving revision 1.7
diff -u -p -b -r1.7 test.c
--- boehm_gc/tests/test.c	24 Feb 2010 07:04:41 -0000	1.7
+++ boehm_gc/tests/test.c	7 Nov 2010 05:19:12 -0000
@@ -1360,6 +1360,108 @@ void GC_CALLBACK warn_proc(char *msg, GC
 # define WINMAIN_LPTSTR LPSTR
 #endif
 
+#define GC_TEST_STOP 0
+#define GC_TEST_START 1
+
+/*
+ * This code tests the GC callback functions.
+ */
+#if defined(GC_PTHREADS)
+static pthread_key_t last_operation_key; 
+
+static void start_collect(void) {
+    int* last_operation = pthread_getspecific(last_operation_key);
+    if (!last_operation) {
+        last_operation = malloc(sizeof(int));
+        *last_operation = GC_TEST_STOP;
+        pthread_setspecific(last_operation_key, last_operation);
+    }
+
+    if (*last_operation != GC_TEST_STOP) {
+        fprintf(stderr,
+            "Start collection event wasn't preceded by a stop collection event\n");
+        exit(1);
+    }
+    *last_operation = GC_TEST_START;
+}
+
+static void stop_collect(void) {
+    int* last_operation = pthread_getspecific(last_operation_key);
+    if (!last_operation) {
+        fprintf(stderr,
+            "Last collection is uninitialised in stop_collect\n");
+        exit(1);
+    } else if (*last_operation != GC_TEST_START) {
+        fprintf(stderr,
+            "Stop collection event wasn't preceded by a start collection event\n");
+        exit(1);
+    }
+    *last_operation = GC_TEST_STOP;
+}
+
+static void setup_callbacks(void) {
+    pthread_key_create(&last_operation_key, free);
+
+    GC_callback_start_collect = start_collect;
+    GC_callback_stop_collect = stop_collect;
+}
+
+static void finalize_callbacks(void) {
+    int* last_operation = pthread_getspecific(last_operation_key);
+
+    if (!last_operation) {
+        fprintf(stderr, "last operation for this thread is unilitialised in finalize_callbacks\n");
+        exit(1);
+    } else if (*last_operation == GC_TEST_START) {
+        fprintf(stderr, "Invalid last operation in finalize_callbacks\n");
+        exit(1);
+    }
+}
+
+#elif defined(GC_WIN32_THREADS)
+/* Testing callbacks is not supported with win32 threads. */
+static void setup_callbacks(void) {
+}
+
+static void finalize_callbacks(void) {
+}
+#else
+static int last_operation = GC_TEST_STOP;
+
+static void start_collect(void) {
+
+    if (last_operation != GC_TEST_STOP) {
+        fprintf(stderr,
+            "Start collection event wasn't preceded by a stop collection event\n");
+        exit(1);
+    }
+    last_operation = GC_TEST_START;
+}
+
+static void stop_collect(void) {
+    if (last_operation != GC_TEST_START) {
+        fprintf(stderr,
+            "Stop collection event wasn't preceded by a start collection event\n");
+        exit(1);
+    }
+    last_operation = GC_TEST_STOP;
+}
+
+static void setup_callbacks(void) {
+    GC_callback_start_collect = start_collect;
+    GC_callback_stop_collect = stop_collect;
+}
+
+static void finalize_callbacks(void) {
+    if (last_operation != GC_TEST_STOP)
+    {
+        fprintf(stderr,
+            "Final state should have been GC_TEST_STOP\n");
+        exit(1);
+    }
+}
+#endif
+
 #if !defined(PCR) \
     && !defined(GC_WIN32_THREADS) && !defined(GC_PTHREADS) \
     || defined(LINT)
@@ -1370,6 +1472,8 @@ void GC_CALLBACK warn_proc(char *msg, GC
   int main(void)
 #endif
 {
+    setup_callbacks();
+
     n_tests = 0;
 #   if defined(MACOS)
         /* Make sure we have lots and lots of stack space.      */
@@ -1416,6 +1520,9 @@ void GC_CALLBACK warn_proc(char *msg, GC
 #   ifdef MSWIN32
       GC_win32_free_heap();
 #   endif
+
+    finalize_callbacks();
+
     return(0);
 }
 # endif
@@ -1612,6 +1719,9 @@ int main(void)
     pthread_attr_t attr;
     int code;
     int i;
+
+    setup_callbacks();
+
 #   ifdef GC_IRIX_THREADS
         /* Force a larger stack to be preallocated      */
         /* Since the initial can't always grow later.   */
@@ -1679,6 +1789,9 @@ int main(void)
         pthread_win32_thread_detach_np ();
         pthread_win32_process_detach_np ();
 #   endif
+
+    finalize_callbacks();
+
     return(0);
 }
 #endif /* GC_PTHREADS */
Index: runtime/mercury_threadscope.c
===================================================================
RCS file: /home/mercury1/repository/mercury/runtime/mercury_threadscope.c,v
retrieving revision 1.7
diff -u -p -b -r1.7 mercury_threadscope.c
--- runtime/mercury_threadscope.c	24 May 2010 06:40:11 -0000	1.7
+++ runtime/mercury_threadscope.c	6 Nov 2010 04:37:42 -0000
@@ -586,10 +586,10 @@ MR_setup_threadscope(void) 
     */
     
     /* Configure Boehm */
-    GC_mercury_callback_start_collect = start_gc_callback;
-    GC_mercury_callback_stop_collect = stop_gc_callback;
-    GC_mercury_callback_pause_thread = pause_thread_gc_callback;
-    GC_mercury_callback_resume_thread = resume_thread_gc_callback;
+    GC_callback_start_collect = start_gc_callback;
+    GC_callback_stop_collect = stop_gc_callback;
+    GC_callback_pause_thread = pause_thread_gc_callback;
+    GC_callback_resume_thread = resume_thread_gc_callback;
 
     /* Clear the global buffer and setup the file */
     global_buffer.MR_tsbuffer_pos = 0;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20101107/c2bd8bfa/attachment.sig>


More information about the reviews mailing list