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

  • Done
  • quality assurance status badge
Details
2 participants
  • ericbavier
  • Ludovic Courtès
Owner
unassigned
Submitted by
ericbavier
Severity
normal

Debbugs page

ericbavier wrote 5 years ago
(address . guix-patches@gnu.org)(name . Eric Bavier)(address . bavier@member.fsf.org)
20190925020706.6034-1-ericbavier@centurylink.net
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(-)

Toggle diff (27 lines)
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
Ludovic Courtès wrote 5 years ago
(address . ericbavier@centurylink.net)
87pnjnpfsz.fsf@gnu.org
Hello,

ericbavier@centurylink.net skribis:

Toggle quote (30 lines)
> 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. :-)

Toggle quote (9 lines)
> +++ 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’.
Eric Bavier wrote 5 years ago
(name . Ludovic Courtès)(address . ludo@gnu.org)
58295170.37303056.1569505365844.JavaMail.zimbra@centurylink.net
----- On Sep 26, 2019, at 4:28 AM, Ludovic Courtès ludo@gnu.org wrote:

Toggle quote (39 lines)
> 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".

Toggle quote (13 lines)
>
>> +++ 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?

Toggle quote (3 lines)
>
> Anyway, LGTM! :-)

Thanks for reviewing.

--
`~Eric
Ludovic Courtès wrote 5 years ago
(name . Eric Bavier)(address . ericbavier@centurylink.net)
87tv8yixmb.fsf@gnu.org
Eric Bavier <ericbavier@centurylink.net> skribis:

Toggle quote (14 lines)
>>> +++ 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’.
Eric Bavier wrote 5 years ago
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 37510-done@debbugs.gnu.org)
1740315284.38306958.1569643268369.JavaMail.zimbra@centurylink.net
----- On Sep 26, 2019, at 3:57 PM, Ludovic Courtès ludo@gnu.org wrote:

Toggle quote (18 lines)
> 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
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 37510@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 37510
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help