GNU bug report logs - #21887
'monitor' form broken

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 21887 in the body.
You can then email your comments to 21887 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: taylanbayirli <at> gmail.com (Taylan Ulrich Bayırlı/Kammer)
To: bug-guile <at> gnu.org
Subject: 'monitor' form broken
Date: Thu, 12 Nov 2015 16:29:30 +0100

It seems that the 'monitor' form is currently a no-op.  The form

    (par-for-each (lambda (x)
                    (monitor
                      (foo)))
                  xs)

should be functionally equivalent to

    (let ((mutex (make-mutex)))
      (par-for-each (lambda (x)
                      (with-mutex mutex
                        (foo)))
                    xs))

but currently becomes

    (par-for-each (lambda (x)
                    (let ((mutex (make-mutex)))
                      (with-mutex mutex
                        (foo))))
                  xs)

which is ineffective.

I don't know what's the best way to fix this.  The simplest thing that
comes to my mind is something along the lines of:

    (define-syntax monitor
      (lambda (stx)
        (syntax-case stx ()
          ((_ body body* ...)
           (let ((uuid (generate-uuid)))
             #`(with-mutex (mutex-with-uuid #,uuid)
                 body body* ...))))))

where mutex-with-uuid looks it up from a hash table at run-time and
instantiates it when it doesn't exist, this operation also being
synchronized across threads, like:

    (define mutex-table (make-hash-table))

    (define mutex-table-mutex (make-mutex))

    (define (mutex-with-uuid uuid)
      (with-mutex mutex-table-mutex
        (or (hash-ref mutex-table uuid)
            (let ((mutex (make-mutex)))
              (hash-set! mutex-table uuid mutex)
              mutex))))

If that looks OK, I can try to make a proper patch from it.  I'm not
sure what I'd use in place of `generate-uuid' though.  Would `gensym' be
good enough?


Shameless advertisement: with SRFI-126, the (or (hash-ref ...) ...) bit
would have been just:

    (hashtable-intern! mutex-table uuid make-mutex)

It's borrowed from MIT/GNU Scheme.  Seems pretty useful.

Taylan

Message #8 received at 21887 <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: taylanbayirli <at> gmail.com (Taylan Ulrich "Bayırlı/Kammer")
Cc: 21887 <at> debbugs.gnu.org
Subject: Re: bug#21887: 'monitor' form broken
Date: Fri, 24 Jun 2016 18:04:06 +0200

Hi Taylan,

On Thu 12 Nov 2015 16:29, taylanbayirli <at> gmail.com (Taylan Ulrich "Bayırlı/Kammer") writes:

> It seems that the 'monitor' form is currently a no-op.  The form
>
>     (par-for-each (lambda (x)
>                     (monitor
>                       (foo)))
>                   xs)
>
> should be functionally equivalent to
>
>     (let ((mutex (make-mutex)))
>       (par-for-each (lambda (x)
>                       (with-mutex mutex
>                         (foo)))
>                     xs))
>
> but currently becomes
>
>     (par-for-each (lambda (x)
>                     (let ((mutex (make-mutex)))
>                       (with-mutex mutex
>                         (foo))))
>                   xs)
>
> which is ineffective.
>
> I don't know what's the best way to fix this.  The simplest thing that
> comes to my mind is something along the lines of:
>
>     (define-syntax monitor
>       (lambda (stx)
>         (syntax-case stx ()
>           ((_ body body* ...)
>            (let ((uuid (generate-uuid)))
>              #`(with-mutex (mutex-with-uuid #,uuid)
>                  body body* ...))))))
>
> where mutex-with-uuid looks it up from a hash table at run-time and
> instantiates it when it doesn't exist, this operation also being
> synchronized across threads, like:
>
>     (define mutex-table (make-hash-table))
>
>     (define mutex-table-mutex (make-mutex))
>
>     (define (mutex-with-uuid uuid)
>       (with-mutex mutex-table-mutex
>         (or (hash-ref mutex-table uuid)
>             (let ((mutex (make-mutex)))
>               (hash-set! mutex-table uuid mutex)
>               mutex))))
>
> If that looks OK, I can try to make a proper patch from it.  I'm not
> sure what I'd use in place of `generate-uuid' though.  Would `gensym' be
> good enough?

You're totally right on all points.  Please do prepare a patch :)  I
wish we could do something faster for the "embedded" mutex but
correctness should come first.

Cheers,

Andy

Message #11 received at 21887 <at> debbugs.gnu.org (full text, mbox):

From: taylanbayirli <at> gmail.com (Taylan Ulrich Bayırlı/Kammer)
To: Andy Wingo <wingo <at> pobox.com>
Cc: 21887 <at> debbugs.gnu.org
Subject: Re: bug#21887: 'monitor' form broken
Date: Sat, 25 Jun 2016 16:51:26 +0200

[Message part 1 (text/plain, inline)]

Here's a patch, tested minimally by running

    (par-for-each (lambda (x)
                    (monitor
                      (sleep 1)
                      (display "foo\n")))
                  (iota 10))

on a quad-core.  Previously it would print the "foo"s in groups of four
with a second between each group; now it prints them one by one with a
second between each, as should be.

[0001-Fix-monitor-macro.patch (text/x-diff, inline)]

From 08c7f4cd98c86fbb6551c7c0b6f17262c67e7b23 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Taylan=20Ulrich=20Bay=C4=B1rl=C4=B1/Kammer?=
 <taylanbayirli <at> gmail.com>
Date: Sat, 25 Jun 2016 16:43:36 +0200
Subject: [PATCH] Fix 'monitor' macro.

* module/ice-9/threads.scm (monitor-mutex-table)
(monitor-mutex-table-mutex, monitor-mutex-with-id): New variables.
(monitor): Fix it.
---
 module/ice-9/threads.scm | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/module/ice-9/threads.scm b/module/ice-9/threads.scm
index 9f9e1bf..14da113 100644
--- a/module/ice-9/threads.scm
+++ b/module/ice-9/threads.scm
@@ -85,9 +85,24 @@
       (lambda () (begin e0 e1 ...))
       (lambda () (unlock-mutex x)))))
 
-(define-syntax-rule (monitor first rest ...)
-  (with-mutex (make-mutex)
-    first rest ...))
+(define monitor-mutex-table (make-hash-table))
+
+(define monitor-mutex-table-mutex (make-mutex))
+
+(define (monitor-mutex-with-id id)
+  (with-mutex monitor-mutex-table-mutex
+    (or (hashq-ref monitor-mutex-table id)
+        (let ((mutex (make-mutex)))
+          (hashq-set! monitor-mutex-table id mutex)
+          mutex))))
+
+(define-syntax monitor
+  (lambda (stx)
+    (syntax-case stx ()
+      ((_ body body* ...)
+       (let ((id (datum->syntax #'body (gensym))))
+         #`(with-mutex (monitor-mutex-with-id '#,id)
+             body body* ...))))))
 
 (define (par-mapper mapper cons)
   (lambda (proc . lists)
-- 
2.8.4

Message #14 received at 21887 <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: taylanbayirli <at> gmail.com (Taylan Ulrich "Bayırlı/Kammer")
Cc: 21887 <at> debbugs.gnu.org
Subject: Re: bug#21887: 'monitor' form broken
Date: Mon, 27 Jun 2016 09:33:16 +0200

On Sat 25 Jun 2016 16:51, taylanbayirli <at> gmail.com (Taylan Ulrich "Bayırlı/Kammer") writes:

> Here's a patch, tested minimally by running
>
>     (par-for-each (lambda (x)
>                     (monitor
>                       (sleep 1)
>                       (display "foo\n")))
>                   (iota 10))
>
> on a quad-core.  Previously it would print the "foo"s in groups of four
> with a second between each group; now it prints them one by one with a
> second between each, as should be.

Applied, thanks!

Andy

Message #19 received at 21887-done <at> debbugs.gnu.org (full text, mbox):

From: Andy Wingo <wingo <at> pobox.com>
To: 21887-done <at> debbugs.gnu.org
Subject: Re: bug#21887: 'monitor' form broken
Date: Mon, 27 Jun 2016 09:33:36 +0200

thanks

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.