GNU bug report logs - #37510
[PATCH 1/1] compile: Fix race condition on completion progress.

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 37510 in the body.
You can then email your comments to 37510 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: ericbavier <at> centurylink.net
To: guix-patches <at> gnu.org
Cc: Eric Bavier <bavier <at> member.fsf.org>
Subject: [PATCH 1/1] compile: Fix race condition on completion progress.
Date: Tue, 24 Sep 2019 21:07:06 -0500

From: Eric Bavier <bavier <at> member.fsf.org>

This prevent a race condition where multiple compilation threads could report
the same completion.

* guix/build/compile.scm (compile-files)<completed>: Increment in same mutex
region as the compilation is reported.


Further reading:

When compiling many scheme files, or with '-j1', this is not usually a
problem, but with multiple build jobs and a handful of scheme files to update,
you may encounter unexpected output.  E.g. I recently saw this from `make -j2`:

```
Compiling Scheme modules...
[ 25%] LOAD     gnu/packages/haskell.scm
;;; note: source file ./gnu/packages/haskell.scm
;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/haskell.go
;;; note: source file ./gnu/packages/haskell.scm
;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/haskell.go
[ 50%] LOAD     gnu/packages/idris.scm
;;; note: source file ./gnu/packages/idris.scm
;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/idris.go
;;; note: source file ./gnu/packages/idris.scm
;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/idris.go
[ 75%] GUILEC   gnu/packages/haskell.go
[ 75%] GUILEC   gnu/packages/idris.go
make[2]: Leaving directory '/home/bavier/projects/guix'
make[1]: Leaving directory '/home/bavier/projects/guix'
```
---
 guix/build/compile.scm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/guix/build/compile.scm b/guix/build/compile.scm
index c127456fd0..f77e49340a 100644
--- a/guix/build/compile.scm
+++ b/guix/build/compile.scm
@@ -173,7 +173,8 @@ files are for HOST, a GNU triplet such as \"x86_64-linux-gnu\"."
 
   (define (build file)
     (with-mutex progress-lock
-      (report-compilation file total completed))
+      (report-compilation file total completed)
+      (set! completed (+ 1 completed)))
 
     ;; Exit as soon as something goes wrong.
     (exit-on-exception
@@ -185,9 +186,7 @@ files are for HOST, a GNU triplet such as \"x86_64-linux-gnu\"."
                          #:output-file (string-append build-directory "/"
                                                       (scm->go relative))
                          #:opts (append warning-options
-                                        (optimization-options relative)))))))
-    (with-mutex progress-lock
-      (set! completed (+ 1 completed))))
+                                        (optimization-options relative))))))))
 
   (with-augmented-search-path %load-path source-directory
     (with-augmented-search-path %load-compiled-path build-directory
-- 
2.23.0

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: ericbavier <at> centurylink.net
Cc: 37510 <at> debbugs.gnu.org, Eric Bavier <bavier <at> member.fsf.org>
Subject: Re: [bug#37510] [PATCH 1/1] compile: Fix race condition on completion
 progress.
Date: Thu, 26 Sep 2019 11:28:28 +0200

Hello,

ericbavier <at> centurylink.net skribis:

> From: Eric Bavier <bavier <at> member.fsf.org>
>
> This prevent a race condition where multiple compilation threads could report
> the same completion.
>
> * guix/build/compile.scm (compile-files)<completed>: Increment in same mutex
> region as the compilation is reported.
>
>
> Further reading:
>
> When compiling many scheme files, or with '-j1', this is not usually a
> problem, but with multiple build jobs and a handful of scheme files to update,
> you may encounter unexpected output.  E.g. I recently saw this from `make -j2`:
>
> ```
> Compiling Scheme modules...
> [ 25%] LOAD     gnu/packages/haskell.scm
> ;;; note: source file ./gnu/packages/haskell.scm
> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/haskell.go
> ;;; note: source file ./gnu/packages/haskell.scm
> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/haskell.go
> [ 50%] LOAD     gnu/packages/idris.scm
> ;;; note: source file ./gnu/packages/idris.scm
> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/idris.go
> ;;; note: source file ./gnu/packages/idris.scm
> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/idris.go
> [ 75%] GUILEC   gnu/packages/haskell.go
> [ 75%] GUILEC   gnu/packages/idris.go

I think it’s expected: it shows completion at the time we started to
build these files.  Compilation of haskell.scm and idris.scm started at
the same time, and at that point we had built 75% of the files.  I agree
it’s confusing though.  :-)

> +++ b/guix/build/compile.scm
> @@ -173,7 +173,8 @@ files are for HOST, a GNU triplet such as \"x86_64-linux-gnu\"."
>  
>    (define (build file)
>      (with-mutex progress-lock
> -      (report-compilation file total completed))
> +      (report-compilation file total completed)
> +      (set! completed (+ 1 completed)))

Here ‘completed’ is incremented before the thing is even started.

Anyway, LGTM!  :-)

Ludo’.

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

From: Eric Bavier <ericbavier <at> centurylink.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 37510 <at> debbugs.gnu.org, bavier <bavier <at> member.fsf.org>
Subject: Re: [bug#37510] [PATCH 1/1] compile: Fix race condition on
 completion progress.
Date: Thu, 26 Sep 2019 09:42:45 -0400 (EDT)

----- On Sep 26, 2019, at 4:28 AM, Ludovic Courtès ludo <at> gnu.org wrote:

> Hello,
> 
> ericbavier <at> centurylink.net skribis:
> 
>> From: Eric Bavier <bavier <at> member.fsf.org>
>>
>> This prevent a race condition where multiple compilation threads could report
>> the same completion.
>>
>> * guix/build/compile.scm (compile-files)<completed>: Increment in same mutex
>> region as the compilation is reported.
>>
>>
>> Further reading:
>>
>> When compiling many scheme files, or with '-j1', this is not usually a
>> problem, but with multiple build jobs and a handful of scheme files to update,
>> you may encounter unexpected output.  E.g. I recently saw this from `make -j2`:
>>
>> ```
>> Compiling Scheme modules...
>> [ 25%] LOAD     gnu/packages/haskell.scm
>> ;;; note: source file ./gnu/packages/haskell.scm
>> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/haskell.go
>> ;;; note: source file ./gnu/packages/haskell.scm
>> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/haskell.go
>> [ 50%] LOAD     gnu/packages/idris.scm
>> ;;; note: source file ./gnu/packages/idris.scm
>> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/idris.go
>> ;;; note: source file ./gnu/packages/idris.scm
>> ;;;       newer than compiled /home/bavier/projects/guix/gnu/packages/idris.go
>> [ 75%] GUILEC   gnu/packages/haskell.go
>> [ 75%] GUILEC   gnu/packages/idris.go
> 
> I think it’s expected: it shows completion at the time we started to
> build these files.  Compilation of haskell.scm and idris.scm started at
> the same time, and at that point we had built 75% of the files.  I agree
> it’s confusing though.  :-)

Right.  If we did indeed expect to see completion at the time we started, then I would expect to see "0%" displayed with the first "LOAD".

> 
>> +++ b/guix/build/compile.scm
>> @@ -173,7 +173,8 @@ files are for HOST, a GNU triplet such as
>> \"x86_64-linux-gnu\"."
>>  
>>    (define (build file)
>>      (with-mutex progress-lock
>> -      (report-compilation file total completed))
>> +      (report-compilation file total completed)
>> +      (set! completed (+ 1 completed)))
> 
> Here ‘completed’ is incremented before the thing is even started.

Maybe a more generic name like "progress" would be appropriate?

> 
> Anyway, LGTM!  :-)

Thanks for reviewing.

-- 
`~Eric

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Eric Bavier <ericbavier <at> centurylink.net>
Cc: 37510 <at> debbugs.gnu.org, bavier <bavier <at> member.fsf.org>
Subject: Re: [bug#37510] [PATCH 1/1] compile: Fix race condition on completion
 progress.
Date: Thu, 26 Sep 2019 22:57:48 +0200

Eric Bavier <ericbavier <at> centurylink.net> skribis:

>>> +++ b/guix/build/compile.scm
>>> @@ -173,7 +173,8 @@ files are for HOST, a GNU triplet such as
>>> \"x86_64-linux-gnu\"."
>>>  
>>>    (define (build file)
>>>      (with-mutex progress-lock
>>> -      (report-compilation file total completed))
>>> +      (report-compilation file total completed)
>>> +      (set! completed (+ 1 completed)))
>> 
>> Here ‘completed’ is incremented before the thing is even started.
>
> Maybe a more generic name like "progress" would be appropriate?

Yes, probably!

Thanks,
Ludo’.

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

From: Eric Bavier <ericbavier <at> centurylink.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 37510-done <at> debbugs.gnu.org
Subject: Re: [bug#37510] [PATCH 1/1] compile: Fix race condition on
 completion progress.
Date: Sat, 28 Sep 2019 00:01:08 -0400 (EDT)

----- On Sep 26, 2019, at 3:57 PM, Ludovic Courtès ludo <at> gnu.org wrote:

> Eric Bavier <ericbavier <at> centurylink.net> skribis:
> 
>>>> +++ b/guix/build/compile.scm
>>>> @@ -173,7 +173,8 @@ files are for HOST, a GNU triplet such as
>>>> \"x86_64-linux-gnu\"."
>>>>  
>>>>    (define (build file)
>>>>      (with-mutex progress-lock
>>>> -      (report-compilation file total completed))
>>>> +      (report-compilation file total completed)
>>>> +      (set! completed (+ 1 completed)))
>>> 
>>> Here ‘completed’ is incremented before the thing is even started.
>>
>> Maybe a more generic name like "progress" would be appropriate?
> 
> Yes, probably!

Ok, pushed with a rename to "progress" in commit 21391f8c83.

-- 
`~Eric

Previous Next

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