Re: [PATCH] build: Speed up .go compilation.

[Taylan Ulrich Bayırlı/Kammer]

address@hidden (Ludovic Courtès) writes:

> So, what speedup to you get compared to ‘make -jN’?

It should have the same improvement as 'guix pull' since it does
basically the same thing.  I measured to make things concrete, and on my
machine the run time of 'make -j4' decreases from ~26m to ~6m.

>> * Makefile.am: Call build-aux/compile-all.scm to compile many .scm files
>>   in a single process.
>
> Rather:
>
>   * Makefile.am (%.go, make-go): New rules.
>
> Also, the new script must be added to ‘EXTRA_DIST’.

Done and done.  Also added a part to the commit message for EXTRA_DIST.

>> +++ b/build-aux/compile-all.scm
>
> Please add a copyright/license header.

Whoops, done.

>> +(define (file->module file)
>> +  (map string->symbol
>> +       (string-split (string-drop-right file 4) #\/)))
>
> Does this work with out-of-tree builds?

Good catch.  There were a few reasons it failed, all fixed now.

(There were also existing problems with out-of-tree builds; see other
patch I sent.)

>> +(let* ((args (cdr (command-line)))
>> +       (target (car args))
>> +       (files (cdr args)))
>
> Please use ‘match’ for the win!  :-)

Done.  I really need to burn that into my coding habits.

>> +  (for-each
>> +   (lambda (file)
>> +     (let ((go (scm->go file)))
>> +       (unless (and (file-exists? go)
>> +                    (file-mtime<? file go))
>> +         (let ((module (file->module file)))
>> +           (format #t "  LOAD ~s~%" module)
>> +           (resolve-interface module)))))
>> +   files)
>
> Make the ‘lambda’ a named top-level procedure, for clarity.  Also add a
> reference to the evil bug that makes this hack necessary in the first
> place.
>
> Maybe it would be clearer to have:
>
>   (define (file-needs-compilation? file)
>     (file-mtime<? (scm->go file) file))
>
> and then to:
>
>   (let ((files (filter file-needs-compilation? files)))
>     ;; …
>     )

Indeed, done.

>> +  (with-target target
>> +    (lambda ()
>> +      (let ((mutex (make-mutex)))
>> +        (par-for-each
>> +         (lambda (file)
>> +           (let ((go (scm->go file)))
>> +             (unless (and (file-exists? go)
>> +                          (file-mtime<? file go))
>> +               (with-mutex mutex
>> +                 (format #t "  GUILEC ~s~%" file)
>> +                 (force-output))
>> +               (compile-file file #:output-file go #:opts compile-options)
>> +               (with-mutex mutex
>> +                 (format #t "  WROTE ~s~%" go)
>> +                 (force-output)))))
>> +         files)))))
>
> Ditto: name the lambda.

Here the lambda cannot entirely be factored out since the 'mutex' from
the lexical scope is needed.  I still factored out the actual code and
pass the mutex as an argument to the procedure in a minimal lambda, as
seen in the new patch below; I hope it's readable this way.

> I would use ~a instead of ~s, to match the current output, and remove
> the “WROTE” output.

Done.

> Could you send an updated patch?

Attached at the bottom.

> It would be awesome if you could check that ‘make distcheck’ still
> passes, and also make sure things behave correctly when modifying just
> one file and running ‘make’, things like that.

I'm having headaches with distcheck.  Currently it bails out because I'm
missing tex.  Debian's version is apparently too old, and Guix's version
is huge and has been downloading for many hours now.  I'll report back
again when I'm done with that.

I've ran 'make' on my branch containing this patch a couple times in the
past after rebasing on master, so that a subset of the .scm files would
be recompiled, and didn't have issues.  I can't imagine a way it would
break either, for any given subset of all the .scm files.  I hope I'm
not overseeing anything.

> Thank you!

Thanks for the review! :-)

Other notable changes in this version of the patch:

- I noticed guix/config.scm and guix/tests.scm are not in MODULES in
  Makefile.am (intentionally?), so I added them to the make-go rule.

- I removed the MKDIR_P loop in the make-go rule, and do the equivalent
  in the Scheme code now.

- The target host and top source directory are now passed to the script
  via lowercase environment variables, which makes the code a little
  simpler.  I hope this is stylistically fine.

> Ludo’.

Taylan


>From 697950b82ea86f7b7438e586bbf4efae3e87d8f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Taylan=20Ulrich=20Bay=C4=B1rl=C4=B1/Kammer?=
 <address@hidden>
Date: Thu, 5 Nov 2015 23:42:45 +0100
Subject: [PATCH] build: Speed up .go compilation.

* build-aux/compile-all.scm: New file.
* Makefile.am (EXTRA_DIST): Add it.
(%.go, make-go): New rules.
---
 Makefile.am               | 22 +++++--------
 build-aux/compile-all.scm | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 14 deletions(-)
 create mode 100644 build-aux/compile-all.scm

diff --git a/Makefile.am b/Makefile.am
index 760caed..dfdc7b5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -302,6 +302,7 @@ EXTRA_DIST =                                                
\
   CODE-OF-CONDUCT                                      \
   .dir-locals.el                                       \
   build-aux/build-self.scm                             \
+  build-aux/compile-all.scm                            \
   build-aux/hydra/gnu-system.scm                       \
   build-aux/hydra/demo-os.scm                          \
   build-aux/hydra/guix.scm                             \
@@ -341,14 +342,6 @@ CLEANFILES =                                       \
   $(GOBJECTS)                                  \
   $(SCM_TESTS:tests/%.scm=%.log)
 
-AM_V_GUILEC = $(AM_V_GUILEC_$(V))
-AM_V_GUILEC_ = $(AM_V_GUILEC_$(AM_DEFAULT_VERBOSITY))
-AM_V_GUILEC_0 = @echo "  GUILEC" $@;
-
-# Flags passed to 'guild compile'.
-GUILD_COMPILE_FLAGS =                          \
-  -Wformat -Wunbound-variable -Warity-mismatch
-
 # Unset 'GUILE_LOAD_COMPILED_PATH' altogether while compiling.  Otherwise, if
 # $GUILE_LOAD_COMPILED_PATH contains $(moduledir), we may find .go files in
 # there that are newer than the local .scm files (for instance because the
@@ -358,14 +351,15 @@ GUILD_COMPILE_FLAGS =                             \
 #
 # XXX: Use the C locale for when Guile lacks
 # 
<http://git.sv.gnu.org/cgit/guile.git/commit/?h=stable-2.0&id=e2c6bf3866d1186c60bacfbd4fe5037087ee5e3f>.
-.scm.go:
-       $(AM_V_GUILEC)$(MKDIR_P) `dirname "$@"` ;                       \
+%.go: make-go ; @:
+make-go: $(MODULES) guix/config.scm guix/tests.scm
        unset GUILE_LOAD_COMPILED_PATH ;                                \
        LC_ALL=C                                                        \
+       host=$(host) srcdir="$(top_srcdir)"                             \
        $(top_builddir)/pre-inst-env                                    \
-       $(GUILD) compile -L "$(top_builddir)" -L "$(top_srcdir)"        \
-         $(GUILD_COMPILE_FLAGS) --target="$(host)"                     \
-         -o "$@" "$<"
+       $(GUILE) -L "$(top_builddir)" -L "$(top_srcdir)"                \
+         --no-auto-compile                                             \
+         -s "$(top_srcdir)"/build-aux/compile-all.scm $^
 
 SUFFIXES = .go
 
@@ -457,6 +451,6 @@ assert-final-inputs-self-contained:
        $(top_builddir)/pre-inst-env "$(GUILE)"                         \
          "$(top_srcdir)/build-aux/check-final-inputs-self-contained.scm"
 
-.PHONY: sync-descriptions gen-ChangeLog gen-AUTHORS clean-go
+.PHONY: sync-descriptions gen-ChangeLog gen-AUTHORS clean-go make-go
 .PHONY: assert-no-store-file-names assert-binaries-available
 .PHONY: assert-final-inputs-self-contained
diff --git a/build-aux/compile-all.scm b/build-aux/compile-all.scm
new file mode 100644
index 0000000..924ec39
--- /dev/null
+++ b/build-aux/compile-all.scm
@@ -0,0 +1,82 @@
+;;; GNU Guix --- Functional package management for GNU
+;;; Copyright © 2016 Taylan Ulrich Bayırlı/Kammer <address@hidden>
+;;;
+;;; This file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+(use-modules (system base target)
+             (ice-9 match)
+             (ice-9 threads)
+             (guix build utils))
+
+(define compile-options '(format unbound-variable arity-mismatch))
+
+(define host (getenv "host"))
+
+(define srcdir (getenv "srcdir"))
+
+(define (relative-file file)
+  (if (string-prefix? (string-append srcdir "/") file)
+      (string-drop file (+ 1 (string-length srcdir)))
+      file))
+
+(define (file-mtime<? f1 f2)
+  (< (stat:mtime (stat f1))
+     (stat:mtime (stat f2))))
+
+(define (scm->go file)
+  (let* ((relative (relative-file file))
+         (without-extension (string-drop-right relative 4)))
+    (string-append without-extension ".go")))
+
+(define (file-needs-compilation? file)
+  (let ((go (scm->go file)))
+    (or (not (file-exists? go))
+        (file-mtime<? go file))))
+
+(define (file->module file)
+  (let* ((relative (relative-file file))
+         (module-path (string-drop-right relative 4)))
+    (map string->symbol
+         (string-split module-path #\/))))
+
+;;; To work around <http://bugs.gnu.org/15602> (FIXME), we want to load all
+;;; files to be compiled first.  We do this via resolve-interface so that the
+;;; top-level of each file (module) is only executed once.
+(define (load-module-file file)
+  (let ((module (file->module file)))
+    (format #t "  LOAD ~a~%" module)
+    (resolve-interface module)))
+
+(define (compile-file* file output-mutex)
+  (let ((go (scm->go file)))
+    (with-mutex output-mutex
+      (format #t "  GUILEC ~a~%" go)
+      (force-output))
+    (mkdir-p (dirname go))
+    (with-target host
+      (lambda ()
+        (compile-file file
+                      #:output-file go
+                      #:opts compile-options)))))
+
+(match (command-line)
+  ((_ . files)
+   (let ((files (filter file-needs-compilation? files)))
+     (for-each load-module-file files)
+     (let ((mutex (make-mutex)))
+       (par-for-each (lambda (file)
+                       (compile-file* file mutex))
+                     files)))))
-- 
2.6.3