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

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

[ericbavier]

From: Eric Bavier <bavier@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

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

[Ludovic Courtès]

Hello,

ericbavier@centurylink.net skribis:

> From: Eric Bavier <bavier@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’.

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

[Eric Bavier]

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

> Hello,
> 
> ericbavier@centurylink.net skribis:
> 
>> From: Eric Bavier <bavier@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

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

[Ludovic Courtès]

Eric Bavier <ericbavier@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’.

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

[Eric Bavier]

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

> Eric Bavier <ericbavier@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