[PATCH core-updates 0/2] build-system/go: Enable parallelism.

  • Done
  • quality assurance status badge
Details
5 participants
  • Efraim Flashner
  • Sarah Morgensen
  • Maxim Cournoyer
  • Mathieu Othacehe
  • zimoun
Owner
unassigned
Submitted by
Sarah Morgensen
Severity
normal
S
S
Sarah Morgensen wrote on 7 Aug 2021 06:45
(address . guix-patches@gnu.org)
cover.1628309588.git.iskarian@mgsn.dev
Hello Guix,

These patches give the Go build system the standard parallelism levers. I would
have thought that Go was already detecting the correct number for GOMAXPROCS,
but after I made this same change for the bootstrapping Go compiler, Efraim
found that it cut the build time nearly in half [0].


--
Sarah Morgensen (2):
build-system/go: Honor #:parallel-build?.
build-system/go: Honor #:parallel-tests?.

guix/build-system/go.scm | 5 +++++
guix/build/go-build-system.scm | 12 ++++++++++--
2 files changed, 15 insertions(+), 2 deletions(-)


base-commit: 693d75e859150601145b7f7303f61d4f48e76927
--
2.31.1
S
S
Sarah Morgensen wrote on 7 Aug 2021 06:48
[PATCH core-updates 1/2] build-system/go: Honor #:parallel-build?.
(address . 49919@debbugs.gnu.org)
1ea32faddea717239b3d56cfa7e647fc1d48faa7.1628309588.git.iskarian@mgsn.dev
guix/build/go-build-system.scm (build): Honor #:parallel-build?.
guix/build-system/go.scm (go-build): Add PARALLEL-BUILD? parameter.
[builder]: Use it.
---
guix/build-system/go.scm | 3 +++
guix/build/go-build-system.scm | 7 ++++++-
2 files changed, 9 insertions(+), 1 deletion(-)

Toggle diff (56 lines)
diff --git a/guix/build-system/go.scm b/guix/build-system/go.scm
index 100d1db4b6..a5aa21b99e 100644
--- a/guix/build-system/go.scm
+++ b/guix/build-system/go.scm
@@ -3,6 +3,7 @@
;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;; Copyright © 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -129,6 +130,7 @@ commit hash and its date rather than a proper release tag."
(unpack-path "")
(build-flags ''())
(tests? #t)
+ (parallel-build? #t)
(allow-go-reference? #f)
(system (%current-system))
(guile #f)
@@ -153,6 +155,7 @@ commit hash and its date rather than a proper release tag."
#:unpack-path #$unpack-path
#:build-flags #$build-flags
#:tests? #$tests?
+ #:parallel-build? #$parallel-build?
#:allow-go-reference? #$allow-go-reference?
#:inputs #$(input-tuples->gexp inputs)))))
diff --git a/guix/build/go-build-system.scm b/guix/build/go-build-system.scm
index 227df820db..521bad9667 100644
--- a/guix/build/go-build-system.scm
+++ b/guix/build/go-build-system.scm
@@ -5,6 +5,7 @@
;;; Copyright © 2020 Jack Hill <jackhill@jackhill.us>
;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -216,8 +217,12 @@ unpacking."
(_ #f))
inputs))))
-(define* (build #:key import-path build-flags #:allow-other-keys)
+(define* (build #:key import-path build-flags (parallel-build? #t)
+ #:allow-other-keys)
"Build the package named by IMPORT-PATH."
+ (let* ((njobs (if parallel-build? (parallel-job-count) 1)))
+ (setenv "GOMAXPROCS" (number->string njobs)))
+
(with-throw-handler
#t
(lambda _
--
2.31.1
S
S
Sarah Morgensen wrote on 7 Aug 2021 06:48
[PATCH core-updates 2/2] build-system/go: Honor #:parallel-tests?.
(address . 49919@debbugs.gnu.org)
0ec3e2c95c5be19dd9c07dd21f7f55283ebdc9da.1628309588.git.iskarian@mgsn.dev
guix/build/go-build-system.scm (build): Honor #:parallel-tests?.
guix/build-system/go.scm (go-build): Add PARALLEL-TESTS? parameter.
[builder]: Use it.
---
guix/build-system/go.scm | 2 ++
guix/build/go-build-system.scm | 5 ++++-
2 files changed, 6 insertions(+), 1 deletion(-)

Toggle diff (40 lines)
diff --git a/guix/build-system/go.scm b/guix/build-system/go.scm
index a5aa21b99e..7b66163779 100644
--- a/guix/build-system/go.scm
+++ b/guix/build-system/go.scm
@@ -131,6 +131,7 @@ commit hash and its date rather than a proper release tag."
(build-flags ''())
(tests? #t)
(parallel-build? #t)
+ (parallel-tests? #t)
(allow-go-reference? #f)
(system (%current-system))
(guile #f)
@@ -156,6 +157,7 @@ commit hash and its date rather than a proper release tag."
#:build-flags #$build-flags
#:tests? #$tests?
#:parallel-build? #$parallel-build?
+ #:parallel-tests? #$parallel-tests?
#:allow-go-reference? #$allow-go-reference?
#:inputs #$(input-tuples->gexp inputs)))))
diff --git a/guix/build/go-build-system.scm b/guix/build/go-build-system.scm
index 521bad9667..0ad580a484 100644
--- a/guix/build/go-build-system.scm
+++ b/guix/build/go-build-system.scm
@@ -239,9 +239,12 @@ unpacking."
(invoke "go" "env"))))
;; Can this also install commands???
-(define* (check #:key tests? import-path #:allow-other-keys)
+(define* (check #:key tests? import-path (parallel-tests? #t)
+ #:allow-other-keys)
"Run the tests for the package named by IMPORT-PATH."
(when tests?
+ (let* ((njobs (if parallel-tests? (parallel-job-count) 1)))
+ (setenv "GOMAXPROCS" (number->string njobs)))
(invoke "go" "test" import-path))
#t)
--
2.31.1
M
M
Mathieu Othacehe wrote on 19 Aug 2021 17:13
Re: bug#49919: [PATCH core-updates 0/2] build-system/go: Enable parallelism.
(name . Sarah Morgensen)(address . iskarian@mgsn.dev)(address . 49919@debbugs.gnu.org)
87wnohcvrj.fsf@gnu.org
Hello Sarah,

Toggle quote (5 lines)
> These patches give the Go build system the standard parallelism levers. I would
> have thought that Go was already detecting the correct number for GOMAXPROCS,
> but after I made this same change for the bootstrapping Go compiler, Efraim
> found that it cut the build time nearly in half [0].

This looks good, but I'd rather have Cuirass fixed before pushing this
series so that we can evaluate the impact on all Go packages and
potentially find some regressions.

Thanks,

Mathieu
Z
Z
zimoun wrote on 31 Aug 2021 12:03
Re: [bug#49919] [PATCH core-updates 0/2] build-system/go: Enable parallelism.
8635qqorrs.fsf@gmail.com
Hi,

On Fri, 06 Aug 2021 at 21:45, Sarah Morgensen <iskarian@mgsn.dev> wrote:

Toggle quote (7 lines)
> These patches give the Go build system the standard parallelism levers. I would
> have thought that Go was already detecting the correct number for GOMAXPROCS,
> but after I made this same change for the bootstrapping Go compiler, Efraim
> found that it cut the build time nearly in half [0].
>
> [0] https://issues.guix.gnu.org/49616

Honoring the parallelism, do the Go packages still build
deterministically? If not, the default should be still non parallel, as
it is for Haskell packages, IIRC. However, maybe there is room for a
package transformation ’--with-parallelism’ to easily turn on when speed
matters more than reproducibility.

All the best,
simon
S
S
Sarah Morgensen wrote on 31 Aug 2021 18:06
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 49919@debbugs.gnu.org)
86fsup1tvq.fsf@mgsn.dev
Hello,

zimoun <zimon.toutoune@gmail.com> writes:

Toggle quote (17 lines)
> Hi,
>
> On Fri, 06 Aug 2021 at 21:45, Sarah Morgensen <iskarian@mgsn.dev> wrote:
>
>> These patches give the Go build system the standard parallelism levers. I would
>> have thought that Go was already detecting the correct number for GOMAXPROCS,
>> but after I made this same change for the bootstrapping Go compiler, Efraim
>> found that it cut the build time nearly in half [0].
>>
>> [0] https://issues.guix.gnu.org/49616
>
> Honoring the parallelism, do the Go packages still build
> deterministically? If not, the default should be still non parallel, as
> it is for Haskell packages, IIRC. However, maybe there is room for a
> package transformation ’--with-parallelism’ to easily turn on when speed
> matters more than reproducibility.

I haven't personally tested this aside from a few casual runs with
--check, but from everything I understand about Go, parallelism does not
affect build determinism. Each build unit should still be compiled by a
single worker sequentially, but parallelism enables separate build units
to be compiled in parallel.

--
Sarah
E
E
Efraim Flashner wrote on 15 Dec 2021 11:35
(name . Mathieu Othacehe)(address . othacehe@gnu.org)
YbnE7XfH6JJcqCKE@3900XT
On Thu, Aug 19, 2021 at 05:13:52PM +0200, Mathieu Othacehe wrote:
Toggle quote (13 lines)
>
> Hello Sarah,
>
> > These patches give the Go build system the standard parallelism levers. I would
> > have thought that Go was already detecting the correct number for GOMAXPROCS,
> > but after I made this same change for the bootstrapping Go compiler, Efraim
> > found that it cut the build time nearly in half [0].
>
> This looks good, but I'd rather have Cuirass fixed before pushing this
> series so that we can evaluate the impact on all Go packages and
> potentially find some regressions.
>

Friendly ping to see if Cuirass is ready for these patches to be pushed.

--
Efraim Flashner <efraim@flashner.co.il> ????? ?????
GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted
-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEoov0DD5VE3JmLRT3Qarn3Mo9g1EFAmG5xO0ACgkQQarn3Mo9
g1GflA/9G2WUlly3Yqa7YVYLsykPKPd9fXwTr6LYhwQTdM8EunVPrDTnPPG50hA+
uzvs+ZbDR+rHD3Yo9DtWKCaekaSU7WeaQjJVOLQCz92JpYzAvhq8luniDHY7rZaR
QpSPdgC3NofNZUI/SRpqcutH10rUZ/Ecrov1sES4N6/+NM/Lvewgz01EsSFEyJ3I
yO1ETiK/TriyiNMwdoVdtiWTTt/RKrnvmI6Wv5qsGFdRmLVDNrxcakmAce2xKhz3
ky+NqMyePw0rpr7GCJbZh6OmVLRAg+eMaBgZvHqzLjsbtrhC5YuNwY9DLYJZKVbx
+5zL1NcvQAOwv7Z25f35XSUQq0pev86E0q63gve0xOtir9YuAeflhOsrgOlU3cF6
o/FE8Cxk7fD0lz7XVAeLVvy3S0GplfWFOZDyaHbMYiNHBcgCNJ+FXPMZDwGvo87j
HTM+lISs8tP2oW3vJ5oFft0gJ2jVLqRdyXJA7/CyhW26y07PeVKXx1Rv/5A9Wr7w
izmnNjUOsy+JE9Ol93cXjI8FWd1S+Tryhwp9tRQDJJspl3Qr2N2WuVspjsopzRgd
8GX9gGggs3a2ICL9QrbIX+Hcw8g9d/Toiv8Dhp2BB5wgzYByik1DdAULwxV87NNw
aSUsK0lo6H80J7qQtZAcWkVeINe76BHB/XvhKCmA/rpTC8FqLvk=
=kEkm
-----END PGP SIGNATURE-----


M
M
Mathieu Othacehe wrote on 15 Dec 2021 11:43
(name . Efraim Flashner)(address . efraim@flashner.co.il)
87wnk6jghe.fsf@gnu.org
Hey,

Toggle quote (2 lines)
> Friendly ping to see if Cuirass is ready for these patches to be pushed.

Not really, I need to reconfigure Berlin and I'm hitting multiple
issues, such as a php build failure :(.

Mathieu
M
M
Maxim Cournoyer wrote on 20 Jan 21:49 +0100
(name . Mathieu Othacehe)(address . othacehe@gnu.org)
871qabenm6.fsf@gmail.com
Hello,

Mathieu Othacehe <othacehe@gnu.org> writes:

Toggle quote (7 lines)
> Hey,
>
>> Friendly ping to see if Cuirass is ready for these patches to be pushed.
>
> Not really, I need to reconfigure Berlin and I'm hitting multiple
> issues, such as a php build failure :(.

Queued for applying locally, at last. Closing.

--
Thanks,
Maxim
Closed
?