[m-rev.] for post-commit review: par_builtin tweaks

[Zoltan Somogyi]

For review by Peter Wang.

Zoltan.

library/par_builtin.m:
	Don't initialize the value field unnecessarily.

	Give MR_ prefixes to structure field names.

	Fix some comments.

cvs diff: Diffing .
Index: par_builtin.m
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/library/par_builtin.m,v
retrieving revision 1.13
diff -u -b -r1.13 par_builtin.m
--- par_builtin.m	12 Sep 2007 06:21:13 -0000	1.13
+++ par_builtin.m	1 Oct 2008 06:09:15 -0000
@@ -72,13 +72,14 @@
 
 #ifdef MR_THREAD_SAFE
     struct MR_Future {
-        MercuryLock lock;
             /* lock preventing concurrent accesses */
-        int signalled;
+        MercuryLock     MR_fut_lock;
             /* whether this future has been signalled yet */
-        MR_Context *suspended;
+        int             MR_fut_signalled;
+
             /* linked list of all the contexts blocked on this future */
-        MR_Word value;
+        MR_Context      *MR_fut_suspended;
+        MR_Word         MR_fut_value;
     };
 #else /* !MR_THREAD_SAFE */
     struct MR_Future {
@@ -106,20 +107,24 @@
     MR_incr_hp(fut_addr, MR_round_up(sizeof(MR_Future), sizeof(MR_Word)));
     Future = (MR_Future *) fut_addr;
 
-    pthread_mutex_init(&(Future->lock), MR_MUTEX_ATTR);
+    pthread_mutex_init(&(Future->MR_fut_lock), MR_MUTEX_ATTR);
 
     /*
     ** The mutex needs to be destroyed when the future is garbage collected.
     ** For efficiency we might want to ignore this altogether, e.g. on Linux
     ** pthread_mutex_destroy() only checks that the mutex is unlocked.
+    **
+    ** We initialize the value field only to prevent its previous value,
+    ** which may point to an allocated block, keeping that block alive.
+    ** Semantically, the value field is undefined at this point in time.
     */
   #ifdef MR_CONSERVATIVE_GC
     GC_REGISTER_FINALIZER(Future, MR_finalize_future, NULL, NULL, NULL);
+    Future->MR_fut_value = 0;
   #endif
 
-    Future->signalled = MR_FALSE;
-    Future->suspended = NULL;
-    Future->value = 0;
+    Future->MR_fut_signalled = MR_FALSE;
+    Future->MR_fut_suspended = NULL;
 
 #else
 
@@ -143,7 +148,7 @@
         MR_Future *fut = (MR_Future *) obj;
 
       #ifdef MR_THREAD_SAFE
-        pthread_mutex_destroy(&(fut->lock));
+        pthread_mutex_destroy(&(fut->MR_fut_lock));
       #endif
     }
 #endif
@@ -155,17 +160,27 @@
 "
 #if (!defined MR_HIGHLEVEL_CODE) && (defined MR_THREAD_SAFE)
 
-    MR_LOCK(&(Future->lock), ""future.wait"");
+    /*
+    ** It would be nice if we could rely on an invariant such as
+    ** `if MR_fut_signalled is true, then reading MR_fut_value is ok'
+    ** even *without* wrapping up those two field accesses in the mutex,
+    ** taking the mutex only when MR_fut_signalled is false. (We would
+    ** then have to repeat the test of MR_fut_signalled, of course.)
+    ** Unfortunately, memory systems today cannot be relied on to provide
+    ** the required level of consistency.
+    */
+
+    MR_LOCK(&(Future->MR_fut_lock), ""future.wait"");
 
-    if (Future->signalled) {
-        Value = Future->value;
-        MR_UNLOCK(&(Future->lock), ""future.wait"");
+    if (Future->MR_fut_signalled) {
+        Value = Future->MR_fut_value;
+        MR_UNLOCK(&(Future->MR_fut_lock), ""future.wait"");
     } else {
         MR_Context *ctxt;
 
         /*
-        ** The address of the future can be lost when we resume so save it on
-        ** top of the stack.
+        ** Put the address of the future at a fixed place known to
+        ** mercury__par_builtin__wait_resume, to wit, the top of the stack.
         */
         MR_incr_sp(1);
         MR_sv(1) = (MR_Word) Future;
@@ -178,10 +193,10 @@
         MR_save_context(ctxt);
 
         ctxt->MR_ctxt_resume = MR_ENTRY(mercury__par_builtin__wait_resume);
-        ctxt->MR_ctxt_next = Future->suspended;
-        Future->suspended = ctxt;
+        ctxt->MR_ctxt_next = Future->MR_fut_suspended;
+        Future->MR_fut_suspended = ctxt;
 
-        MR_UNLOCK(&(Future->lock), ""future.wait"");
+        MR_UNLOCK(&(Future->MR_fut_lock), ""future.wait"");
 
         MR_ENGINE(MR_eng_this_context) = NULL;
         MR_runnext();
@@ -225,10 +240,10 @@
         Future = (MR_Future *) MR_sv(1);
         MR_decr_sp(1);
 
-        assert(Future->signalled);
+        assert(Future->MR_fut_signalled);
 
         /* Return to the caller of par_builtin.wait. */
-        MR_r1 = Future->value;
+        MR_r1 = Future->MR_fut_value;
         MR_proceed();
     }
     MR_END_MODULE
@@ -270,8 +285,8 @@
 "
 #if (!defined MR_HIGHLEVEL_CODE) && (defined MR_THREAD_SAFE)
 
-    assert(Future->signalled);
-    Value = Future->value;
+    assert(Future->MR_fut_signalled);
+    Value = Future->MR_fut_value;
 
 #else
 
@@ -290,29 +305,29 @@
     MR_Context *ctxt;
     MR_Context *next;
 
-    MR_LOCK(&(Future->lock), ""future.signal"");
+    MR_LOCK(&(Future->MR_fut_lock), ""future.signal"");
 
     /*
     ** If the same future is passed twice to a procedure then it
     ** could be signalled twice, but the value must be the same.
     */
-    if (Future->signalled) {
-        assert(Future->value == Value);
+    if (Future->MR_fut_signalled) {
+        assert(Future->MR_fut_value == Value);
     } else {
-        Future->signalled = MR_TRUE;
-        Future->value = Value;
+        Future->MR_fut_signalled = MR_TRUE;
+        Future->MR_fut_value = Value;
     }
 
     /* Schedule all the contexts which are blocking on this future. */
-    ctxt = Future->suspended;
+    ctxt = Future->MR_fut_suspended;
     while (ctxt != NULL) {
         next = ctxt->MR_ctxt_next;
         MR_schedule_context(ctxt);  /* clobbers MR_ctxt_next */
         ctxt = next;
     }
-    Future->suspended = NULL;
+    Future->MR_fut_suspended = NULL;
 
-    MR_UNLOCK(&(Future->lock), ""future.signal"");
+    MR_UNLOCK(&(Future->MR_fut_lock), ""future.signal"");
 
 #else
 
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------