* [PATCH] Futures: Avoid creating the worker pool more than once
@ 2012-11-06 23:07 Mark H Weaver
2012-11-07 13:46 ` Mark H Weaver
2012-11-08 19:11 ` Ludovic Courtès
0 siblings, 2 replies; 9+ messages in thread
From: Mark H Weaver @ 2012-11-06 23:07 UTC (permalink / raw
To: guile-devel
[-- Attachment #1: Type: text/plain, Size: 49 bytes --]
Any objections to applying this fix?
Mark
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Futures: Avoid creating the worker pool more than once --]
[-- Type: text/x-diff, Size: 1661 bytes --]
From ff2c84b118b68020805eae93f71c3a8fd13ca3d9 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Tue, 6 Nov 2012 18:03:39 -0500
Subject: [PATCH] Futures: Avoid creating the worker pool more than once.
* module/ice-9/futures.scm (%create-workers!): Check to make sure the
worker pool hasn't already been created within the critical section.
---
module/ice-9/futures.scm | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
index 0f64b5c..afdf48f 100644
--- a/module/ice-9/futures.scm
+++ b/module/ice-9/futures.scm
@@ -158,13 +158,19 @@ touched."
(define (%create-workers!)
(lock-mutex %futures-mutex)
- (set! %workers
- (unfold (lambda (i) (>= i %worker-count))
- (lambda (i)
- (call-with-new-thread process-futures))
- 1+
- 0))
- (set! create-workers! (lambda () #t))
+ ;; setting 'create-workers!' to a no-op is an optimization, but it is
+ ;; still possible for '%create-workers!' to be called more than once
+ ;; from different threads. Therefore, to avoid creating %workers more
+ ;; than once (and thus creating too many threads), we check to make
+ ;; sure %workers is empty within the critical section.
+ (when (null? %workers)
+ (set! create-workers! (lambda () #t))
+ (set! %workers
+ (unfold (lambda (i) (>= i %worker-count))
+ (lambda (i)
+ (call-with-new-thread process-futures))
+ 1+
+ 0)))
(unlock-mutex %futures-mutex))
(define create-workers!
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-06 23:07 [PATCH] Futures: Avoid creating the worker pool more than once Mark H Weaver
@ 2012-11-07 13:46 ` Mark H Weaver
2012-11-07 14:14 ` Daniel Hartwig
2012-11-16 23:15 ` Ludovic Courtès
2012-11-08 19:11 ` Ludovic Courtès
1 sibling, 2 replies; 9+ messages in thread
From: Mark H Weaver @ 2012-11-07 13:46 UTC (permalink / raw
To: guile-devel
[-- Attachment #1: Type: text/plain, Size: 192 bytes --]
Here's an improved version the patch that gracefully handles the case
where creation of the worker pool is unsuccessful due to an exception or
cancelled thread.
What do you think?
Mark
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Futures: Avoid creating the worker pool more than once --]
[-- Type: text/x-diff, Size: 2039 bytes --]
From b0d936a348b916e73e9071abeb7baae3d7c126d3 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Wed, 7 Nov 2012 08:39:42 -0500
Subject: [PATCH] Futures: Avoid creating the worker pool more than once.
* module/ice-9/futures.scm (%create-workers!): Use 'with-mutex' in case
an exception is thrown. Within the critical section, check to make
sure the worker pool hasn't already been created by another thread.
---
module/ice-9/futures.scm | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
index 0f64b5c..7fbccf6 100644
--- a/module/ice-9/futures.scm
+++ b/module/ice-9/futures.scm
@@ -19,6 +19,7 @@
(define-module (ice-9 futures)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
+ #:use-module (ice-9 threads)
#:use-module (ice-9 q)
#:export (future make-future future? touch))
@@ -157,15 +158,20 @@ touched."
(define %workers '())
(define (%create-workers!)
- (lock-mutex %futures-mutex)
- (set! %workers
- (unfold (lambda (i) (>= i %worker-count))
- (lambda (i)
- (call-with-new-thread process-futures))
- 1+
- 0))
- (set! create-workers! (lambda () #t))
- (unlock-mutex %futures-mutex))
+ (with-mutex
+ %futures-mutex
+ ;; Setting 'create-workers!' to a no-op is an optimization, but it is
+ ;; still possible for '%create-workers!' to be called more than once
+ ;; from different threads. Therefore, to avoid creating %workers more
+ ;; than once (and thus creating too many threads), we check to make
+ ;; sure %workers is empty within the critical section.
+ (when (null? %workers)
+ (set! %workers
+ (unfold (lambda (i) (>= i %worker-count))
+ (lambda (i) (call-with-new-thread process-futures))
+ 1+
+ 0))
+ (set! create-workers! (lambda () #t)))))
(define create-workers!
(lambda () (%create-workers!)))
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-07 13:46 ` Mark H Weaver
@ 2012-11-07 14:14 ` Daniel Hartwig
2012-11-16 23:15 ` Ludovic Courtès
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Hartwig @ 2012-11-07 14:14 UTC (permalink / raw
To: Mark H Weaver; +Cc: guile-devel
On 7 November 2012 21:46, Mark H Weaver <mhw@netris.org> wrote:
> Here's an improved version the patch that gracefully handles the case
> where creation of the worker pool is unsuccessful due to an exception or
> cancelled thread.
>
> What do you think?
Looks clean. Nice work picking up on this race condition.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-06 23:07 [PATCH] Futures: Avoid creating the worker pool more than once Mark H Weaver
2012-11-07 13:46 ` Mark H Weaver
@ 2012-11-08 19:11 ` Ludovic Courtès
2012-11-10 3:13 ` Mark H Weaver
1 sibling, 1 reply; 9+ messages in thread
From: Ludovic Courtès @ 2012-11-08 19:11 UTC (permalink / raw
To: guile-devel
Mark H Weaver <mhw@netris.org> skribis:
> + ;; setting 'create-workers!' to a no-op is an optimization, but it is
> + ;; still possible for '%create-workers!' to be called more than once
> + ;; from different threads. Therefore, to avoid creating %workers more
> + ;; than once (and thus creating too many threads), we check to make
> + ;; sure %workers is empty within the critical section.
Do you have a scenario where this happens?
Ludo’.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-08 19:11 ` Ludovic Courtès
@ 2012-11-10 3:13 ` Mark H Weaver
2012-11-10 13:53 ` Ludovic Courtès
0 siblings, 1 reply; 9+ messages in thread
From: Mark H Weaver @ 2012-11-10 3:13 UTC (permalink / raw
To: Ludovic Courtès; +Cc: guile-devel
ludo@gnu.org (Ludovic Courtès) writes:
> Mark H Weaver <mhw@netris.org> skribis:
>
>> + ;; setting 'create-workers!' to a no-op is an optimization, but it is
>> + ;; still possible for '%create-workers!' to be called more than once
>> + ;; from different threads. Therefore, to avoid creating %workers more
>> + ;; than once (and thus creating too many threads), we check to make
>> + ;; sure %workers is empty within the critical section.
>
> Do you have a scenario where this happens?
I'd hoped that the comment above was clear, but let me try again.
If two threads simultaneously call 'make-future' before the worker pool
has been created, then both threads can call 'create-workers!' (before
it has been rebound), and from there both can enter '%create-workers!'.
At that point, unless one of the threads is killed, it is guaranteed
that both will enter the critical section: first one, and then the other
(after the first one has unlocked the mutex). Thus, two worker pools
will be created (though the first worker pool will become inaccessible).
Mark
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-10 3:13 ` Mark H Weaver
@ 2012-11-10 13:53 ` Ludovic Courtès
0 siblings, 0 replies; 9+ messages in thread
From: Ludovic Courtès @ 2012-11-10 13:53 UTC (permalink / raw
To: Mark H Weaver; +Cc: guile-devel
Mark H Weaver <mhw@netris.org> skribis:
> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Mark H Weaver <mhw@netris.org> skribis:
>>
>>> + ;; setting 'create-workers!' to a no-op is an optimization, but it is
>>> + ;; still possible for '%create-workers!' to be called more than once
>>> + ;; from different threads. Therefore, to avoid creating %workers more
>>> + ;; than once (and thus creating too many threads), we check to make
>>> + ;; sure %workers is empty within the critical section.
>>
>> Do you have a scenario where this happens?
>
> I'd hoped that the comment above was clear, but let me try again.
Yes, what wasn’t clear to me is how practical this is, and whether
somebody encountered that situation in real life.
Anyway, please apply.
Thanks,
Ludo’.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-07 13:46 ` Mark H Weaver
2012-11-07 14:14 ` Daniel Hartwig
@ 2012-11-16 23:15 ` Ludovic Courtès
2012-11-30 19:01 ` Mark H Weaver
1 sibling, 1 reply; 9+ messages in thread
From: Ludovic Courtès @ 2012-11-16 23:15 UTC (permalink / raw
To: guile-devel
Hi Mark,
Mark H Weaver <mhw@netris.org> skribis:
> From b0d936a348b916e73e9071abeb7baae3d7c126d3 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <mhw@netris.org>
> Date: Wed, 7 Nov 2012 08:39:42 -0500
> Subject: [PATCH] Futures: Avoid creating the worker pool more than once.
>
> * module/ice-9/futures.scm (%create-workers!): Use 'with-mutex' in case
> an exception is thrown. Within the critical section, check to make
> sure the worker pool hasn't already been created by another thread.
> ---
> module/ice-9/futures.scm | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
>
> diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
> index 0f64b5c..7fbccf6 100644
> --- a/module/ice-9/futures.scm
> +++ b/module/ice-9/futures.scm
> @@ -19,6 +19,7 @@
> (define-module (ice-9 futures)
> #:use-module (srfi srfi-1)
> #:use-module (srfi srfi-9)
> + #:use-module (ice-9 threads)
> #:use-module (ice-9 q)
> #:export (future make-future future? touch))
I just realized that this patch introduces a circular dependency between
the two modules.
Can you think of a way to avoid it?
Thanks,
Ludo’.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-16 23:15 ` Ludovic Courtès
@ 2012-11-30 19:01 ` Mark H Weaver
2012-11-30 20:14 ` Ludovic Courtès
0 siblings, 1 reply; 9+ messages in thread
From: Mark H Weaver @ 2012-11-30 19:01 UTC (permalink / raw
To: Ludovic Courtès; +Cc: guile-devel
Hi Ludovic,
Sorry for not responding to this earlier.
ludo@gnu.org (Ludovic Courtès) writes:
> Mark H Weaver <mhw@netris.org> skribis:
>
>> diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
>> index 0f64b5c..7fbccf6 100644
>> --- a/module/ice-9/futures.scm
>> +++ b/module/ice-9/futures.scm
>> @@ -19,6 +19,7 @@
>> (define-module (ice-9 futures)
>> #:use-module (srfi srfi-1)
>> #:use-module (srfi srfi-9)
>> + #:use-module (ice-9 threads)
>> #:use-module (ice-9 q)
>> #:export (future make-future future? touch))
>
> I just realized that this patch introduces a circular dependency between
> the two modules.
Why is this a problem? I think circular dependencies are fine as long
as there is not a cycle composed entirely of syntactic keyword
dependencies.
After introducing this circular dependency, I successfully bootstrapped
from scratch without difficulties.
What am I missing here?
Mark
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Futures: Avoid creating the worker pool more than once
2012-11-30 19:01 ` Mark H Weaver
@ 2012-11-30 20:14 ` Ludovic Courtès
0 siblings, 0 replies; 9+ messages in thread
From: Ludovic Courtès @ 2012-11-30 20:14 UTC (permalink / raw
To: Mark H Weaver; +Cc: guile-devel
Hi Mark,
Mark H Weaver <mhw@netris.org> skribis:
> Why is this a problem? I think circular dependencies are fine as long
> as there is not a cycle composed entirely of syntactic keyword
> dependencies.
You’re right that it’s fine in some cases, but in general sometimes
works “by chance”, and should be avoided as a rule of thumb.
Ludo’.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-11-30 20:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-06 23:07 [PATCH] Futures: Avoid creating the worker pool more than once Mark H Weaver
2012-11-07 13:46 ` Mark H Weaver
2012-11-07 14:14 ` Daniel Hartwig
2012-11-16 23:15 ` Ludovic Courtès
2012-11-30 19:01 ` Mark H Weaver
2012-11-30 20:14 ` Ludovic Courtès
2012-11-08 19:11 ` Ludovic Courtès
2012-11-10 3:13 ` Mark H Weaver
2012-11-10 13:53 ` Ludovic Courtès
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).