[bug#38754] [PATCH 0/2] Speed up the derivation linter.

[bug#38754] [PATCH 0/2] Speed up the derivation linter.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

These patches speed up the derivation linter by reducing the number of
times a connection to the guix-daemon is established. In testing on my
computer, the time to lint all guix packages with the derivation linter
drops from around 6 minutes to around 3 minutes.


Christopher Baines (2):
  guix: lint: Add an optional parameter for a store connection.
  scripts: lint: Set the %link-checker-store-connection parameter.

 guix/lint.scm         | 42 ++++++++++++++++++++++++++----------------
 guix/scripts/lint.scm | 22 +++++++++++++---------
 2 files changed, 39 insertions(+), 25 deletions(-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

[bug#38754] [PATCH 1/2] guix: lint: Add an optional parameter for a store connection.

[Christopher Baines]

Previously, the derivation lint checker establishes a connection to the store
for each supported system of each package. This change uses the same store
connection for all supported systems, with the option of setting a parameter
for a store connection which will be used instead of establishing a new
connection.

Previously, running the derivation linter for all packages would take around 6
and a half minutes, with this change, without setting the
%lint-checker-store-connection parameter, the time is reduced to around 4
minutes.

* guix/lint.scm (%lint-checker-store-connection): New parameter.
(check-derivation): Arrange the code so that it's possible to either run with
the store from the new parameter, or open a new connection via the with-store
syntax.
---
 guix/lint.scm | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index cd2ea571ed..19498db857 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -100,7 +100,9 @@
             lint-checker?
             lint-checker-name
             lint-checker-description
-            lint-checker-check))
+            lint-checker-check
+
+            %lint-checker-store-connection))
 
 \f
 ;;;
@@ -142,6 +144,9 @@
     ((_ package (G_ message) rest ...)
      (%make-warning package message rest ...))))
 
+(define %lint-checker-store-connection
+  (make-parameter #f))
+
 \f
 ;;;
 ;;; Checkers
@@ -887,7 +892,7 @@ descriptions maintained upstream."
 
 (define (check-derivation package)
   "Emit a warning if we fail to compile PACKAGE to a derivation."
-  (define (try system)
+  (define (try store system)
     (catch #t
       (lambda ()
         (guard (c ((store-protocol-error? c)
@@ -900,25 +905,30 @@ descriptions maintained upstream."
                                  (G_ "failed to create ~a derivation: ~a")
                                  (list system
                                        (condition-message c)))))
-          (with-store store
-            ;; Disable grafts since it can entail rebuilds.
-            (parameterize ((%graft? #f))
-              (package-derivation store package system #:graft? #f)
-
-              ;; If there's a replacement, make sure we can compute its
-              ;; derivation.
-              (match (package-replacement package)
-                (#f #t)
-                (replacement
-                 (package-derivation store replacement system
-                                     #:graft? #f)))))))
+          ;; Disable grafts since it can entail rebuilds.
+          (parameterize ((%graft? #f))
+            (package-derivation store package system #:graft? #f)
+
+            ;; If there's a replacement, make sure we can compute its
+            ;; derivation.
+            (match (package-replacement package)
+              (#f #t)
+              (replacement
+               (package-derivation store replacement system
+                                   #:graft? #f))))))
       (lambda args
         (make-warning package
                       (G_ "failed to create ~a derivation: ~s")
                       (list system args)))))
 
-  (filter lint-warning?
-          (map try (package-supported-systems package))))
+  (define (check-with-store store)
+    (filter lint-warning?
+            (map (cut try store <>) (package-supported-systems package))))
+
+  (or (and=> (%lint-checker-store-connection)
+             check-with-store)
+      (with-store store
+        (check-with-store store))))
 
 (define (check-license package)
   "Warn about type errors of the 'license' field of PACKAGE."
-- 
2.24.1

[bug#38754] [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection parameter.

[Christopher Baines]

If set, this parameter provides a store connection used by the derivation
linter. Without this being set, the derivation linter establishes a new
connection for each package. With this change, I saw the time taken to lint
all packages with the derivation linter drop from over 4 minutes to around 3
minutes.

* guix/scripts/lint.scm (guix-lint): Set the %lint-checker-store-connection)
parameter.
---
 guix/scripts/lint.scm | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 8d08c484f5..47c104217d 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -28,6 +28,7 @@
 
 (define-module (guix scripts lint)
   #:use-module (guix packages)
+  #:use-module (guix store)
   #:use-module (guix lint)
   #:use-module (guix ui)
   #:use-module (guix scripts)
@@ -167,12 +168,15 @@ run the checkers on all packages.\n"))
                              (_ #f))
                            (reverse opts)))
          (checkers (or (assoc-ref opts 'checkers) %all-checkers)))
-    (cond
-     ((assoc-ref opts 'list?)
-      (list-checkers-and-exit checkers))
-     ((null? args)
-      (fold-packages (lambda (p r) (run-checkers p checkers)) '()))
-     (else
-      (for-each (lambda (spec)
-                  (run-checkers (specification->package spec) checkers))
-                args)))))
+    (with-store store
+      (parameterize
+          ((%lint-checker-store-connection store))
+        (cond
+         ((assoc-ref opts 'list?)
+          (list-checkers-and-exit checkers))
+         ((null? args)
+          (fold-packages (lambda (p r) (run-checkers p checkers)) '()))
+         (else
+          (for-each (lambda (spec)
+                      (run-checkers (specification->package spec) checkers))
+                    args)))))))
-- 
2.24.1

[bug#38754] [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection parameter.

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> +    (with-store store
> +      (parameterize
> +          ((%lint-checker-store-connection store))

Actually it means that now ‘guix lint’ systematically connects to the
daemon.

I wonder if we could arrange to open the connection lazily, and to
somehow carry state across linter invocations.  Perhaps
‘check-derivation’ should be monadic, with a field in <checker>
indicating that.  Sounds complicated though.

Thoughts?

Ludo’.

[bug#38754] [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection parameter.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]


Ludovic Courtès <ludo@gnu.org> writes:

> Christopher Baines <mail@cbaines.net> skribis:
>
>> +    (with-store store
>> +      (parameterize
>> +          ((%lint-checker-store-connection store))
>
> Actually it means that now ‘guix lint’ systematically connects to the
> daemon.

I guess that's the effect, were you meaning this would make a better
message in the commit?

> I wonder if we could arrange to open the connection lazily, and to
> somehow carry state across linter invocations.  Perhaps
> ‘check-derivation’ should be monadic, with a field in <checker>
> indicating that.  Sounds complicated though.
>
> Thoughts?

I did wonder if the code could somehow transparently be made more
efficient. Quite often database clients manage a pool of connections,
and when you perform a database operation, a connection from the pool is
checked out, and then returned once you're finished. But as you say,
this could be complicated. I think parameters can be set with
connections, and I'm not quiet sure what the interface should be.

I also did think about somehow passing the store connection in to the
lint checker more explicitly, but I'm not sure how to generalise that.

At least to me, a parameter to store the connection in seemed like a
simple if inelegant approach. I'm quite happy to look in to other
approaches though if you have thoughts on what might be good.

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

[bug#38754] [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection parameter.

[Ludovic Courtès]

Hi Chris!

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Christopher Baines <mail@cbaines.net> skribis:
>>
>>> +    (with-store store
>>> +      (parameterize
>>> +          ((%lint-checker-store-connection store))
>>
>> Actually it means that now ‘guix lint’ systematically connects to the
>> daemon.
>
> I guess that's the effect, were you meaning this would make a better
> message in the commit?

I mean that it’s a visible change.  Before, you could run all the
linters but this one without having a daemon running; now you need a
daemon up and running.

>> I wonder if we could arrange to open the connection lazily, and to
>> somehow carry state across linter invocations.  Perhaps
>> ‘check-derivation’ should be monadic, with a field in <checker>
>> indicating that.  Sounds complicated though.
>>
>> Thoughts?
>
> I did wonder if the code could somehow transparently be made more
> efficient. Quite often database clients manage a pool of connections,
> and when you perform a database operation, a connection from the pool is
> checked out, and then returned once you're finished. But as you say,
> this could be complicated. I think parameters can be set with
> connections, and I'm not quiet sure what the interface should be.
>
> I also did think about somehow passing the store connection in to the
> lint checker more explicitly, but I'm not sure how to generalise that.

There could be a <checker> field indicating either that (1) the
procedure takes an optional store parameter, or that (2) the procedure
is monadic in ‘%store-monad’.

#2 seems more complicated to implement that #1 though.

For #1, ‘guix lint’ could check whether:

  (any checker-require-store? checkers)

is true, and if it is, it could open a connection and pass it on as
needed.

WDYT?

If that seems good to you, I guess you can go ahead with it (let’s just
not lose our hair on it!).

Thanks,
Ludo’.

[bug#38754] [PATCH 1/4] lint: Add a requires-store? field to the checker record.

[Christopher Baines]

This can then be used to mark checkers that require a store connection, which
will enable passing a connection in, avoiding the overhead of establishing a
connection inside the check function when it's run for lots of different
packages.

* guix/lint.scm (<lint-checker>): Add requires-store? to the record type.
---
 guix/lint.scm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index 24fbf05202..2a084382c6 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -100,7 +100,8 @@
             lint-checker?
             lint-checker-name
             lint-checker-description
-            lint-checker-check))
+            lint-checker-check
+            lint-checker-requires-store?))
 
 \f
 ;;;
@@ -155,7 +156,9 @@
   ;; 'certainty' level.
   (name        lint-checker-name)
   (description lint-checker-description)
-  (check       lint-checker-check))
+  (check       lint-checker-check)
+  (requires-store? lint-checker-requires-store?
+                   (default #f)))
 
 (define (properly-starts-sentence? s)
   (string-match "^[(\"'`[:upper:][:digit:]]" s))
-- 
2.25.0

[bug#38754] [PATCH 2/4] lint: Mark the derivation checker as requiring a store connection.

[Christopher Baines]

* guix/lint.scm (%local-checkers): Mark the derivation checker as requiring a
store connection.
---
 guix/lint.scm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index 2a084382c6..b20510b45d 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -1330,9 +1330,10 @@ or a list thereof")
      (description "Check for autogenerated tarballs")
      (check       check-source-unstable-tarball))
    (lint-checker
-     (name        'derivation)
-     (description "Report failure to compile a package to a derivation")
-     (check       check-derivation))
+     (name            'derivation)
+     (description     "Report failure to compile a package to a derivation")
+     (check           check-derivation)
+     (requires-store? #t))
    (lint-checker
     (name        'patch-file-names)
     (description "Validate file names and availability of patches")
-- 
2.25.0

[bug#38754] [PATCH 3/4] lint: Add a #:store argument to check-derivation

[Christopher Baines]

This can then be used to avoid opening up a store connection each time a
package needs checking.

* guix/lint.scm (check-derivation): Add a #:store argument, and pull the
handling of the store connection out of the try function.
---
 guix/lint.scm | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index b20510b45d..cfe3be2302 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -917,9 +917,9 @@ descriptions maintained upstream."
    (define exception-with-kind-and-args?
      (const #f))))
 
-(define (check-derivation package)
+(define* (check-derivation package #:key store)
   "Emit a warning if we fail to compile PACKAGE to a derivation."
-  (define (try system)
+  (define (try store system)
     (catch #t     ;TODO: Remove 'catch' when Guile 2.x is no longer supported.
       (lambda ()
         (guard (c ((store-protocol-error? c)
@@ -938,25 +938,29 @@ descriptions maintained upstream."
                                  (G_ "failed to create ~a derivation: ~a")
                                  (list system
                                        (condition-message c)))))
-          (with-store store
-            ;; Disable grafts since it can entail rebuilds.
-            (parameterize ((%graft? #f))
-              (package-derivation store package system #:graft? #f)
-
-              ;; If there's a replacement, make sure we can compute its
-              ;; derivation.
-              (match (package-replacement package)
-                (#f #t)
-                (replacement
-                 (package-derivation store replacement system
-                                     #:graft? #f)))))))
+          (parameterize ((%graft? #f))
+            (package-derivation store package system #:graft? #f)
+
+            ;; If there's a replacement, make sure we can compute its
+            ;; derivation.
+            (match (package-replacement package)
+              (#f #t)
+              (replacement
+               (package-derivation store replacement system
+                                   #:graft? #f))))))
       (lambda args
         (make-warning package
                       (G_ "failed to create ~a derivation: ~s")
                       (list system args)))))
 
-  (filter lint-warning?
-          (map try (package-supported-systems package))))
+  (define (check-with-store store)
+    (filter lint-warning?
+            (map (cut try store <>) (package-supported-systems package))))
+
+  ;; For backwards compatability, don't rely on store being set
+  (or (and=> store check-with-store)
+      (with-store store
+        (check-with-store store))))
 
 (define (check-license package)
   "Warn about type errors of the 'license' field of PACKAGE."
-- 
2.25.0

[bug#38754] [PATCH 4/4] scripts: lint: Handle store connections for lint checkers.

[Christopher Baines]

Rather than individual checkers opening up a connection to the store for each
package to check, if any checker requires a store connection, open a
connection and pass it to all checkers that would use it. This makes running
the derivation checker much faster for multiple packages.

* guix/scripts/lint.scm (run-checkers): Add a #:store argument, and pass the
store to checkers if they require a store connection.
(guix-lint): Establish a store connection if any checker requires one, and
pass it through to run-checkers.
---
 guix/scripts/lint.scm | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 8d08c484f5..97ffd57301 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -30,6 +30,7 @@
   #:use-module (guix packages)
   #:use-module (guix lint)
   #:use-module (guix ui)
+  #:use-module (guix store)
   #:use-module (guix scripts)
   #:use-module (guix scripts build)
   #:use-module (gnu packages)
@@ -53,7 +54,7 @@
              (lint-warning-message lint-warning))))
    warnings))
 
-(define (run-checkers package checkers)
+(define* (run-checkers package checkers #:key store)
   "Run the given CHECKERS on PACKAGE."
   (let ((tty? (isatty? (current-error-port))))
     (for-each (lambda (checker)
@@ -63,7 +64,9 @@
                           (lint-checker-name checker))
                   (force-output (current-error-port)))
                 (emit-warnings
-                 ((lint-checker-check checker) package)))
+                 (if (lint-checker-requires-store? checker)
+                     ((lint-checker-check checker) package #:store store)
+                     ((lint-checker-check checker) package))))
               checkers)
     (when tty?
       (format (current-error-port) "\x1b[K")
@@ -167,12 +170,27 @@ run the checkers on all packages.\n"))
                              (_ #f))
                            (reverse opts)))
          (checkers (or (assoc-ref opts 'checkers) %all-checkers)))
-    (cond
-     ((assoc-ref opts 'list?)
+
+    (when (assoc-ref opts 'list?)
       (list-checkers-and-exit checkers))
-     ((null? args)
-      (fold-packages (lambda (p r) (run-checkers p checkers)) '()))
-     (else
-      (for-each (lambda (spec)
-                  (run-checkers (specification->package spec) checkers))
-                args)))))
+
+    (let ((any-lint-checker-requires-store?
+           (any lint-checker-requires-store? checkers)))
+
+      (define (call-maybe-with-store proc)
+        (if any-lint-checker-requires-store?
+            (with-store store
+              (proc store))
+            (proc #f)))
+
+      (call-maybe-with-store
+       (lambda (store)
+         (cond
+          ((null? args)
+           (fold-packages (lambda (p r) (run-checkers p checkers
+                                                      #:store store)) '()))
+          (else
+           (for-each (lambda (spec)
+                       (run-checkers (specification->package spec) checkers
+                                     #:store store))
+                     args))))))))
-- 
2.25.0

[bug#38754] [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection parameter.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]


Ludovic Courtès <ludo@gnu.org> writes:

> Hi Chris!
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Christopher Baines <mail@cbaines.net> skribis:
>>>
>>>> +    (with-store store
>>>> +      (parameterize
>>>> +          ((%lint-checker-store-connection store))
>>>
>>> Actually it means that now ‘guix lint’ systematically connects to the
>>> daemon.
>>
>> I guess that's the effect, were you meaning this would make a better
>> message in the commit?
>
> I mean that it’s a visible change.  Before, you could run all the
> linters but this one without having a daemon running; now you need a
> daemon up and running.

Ah, yeah, that's a good point.

>>> I wonder if we could arrange to open the connection lazily, and to
>>> somehow carry state across linter invocations.  Perhaps
>>> ‘check-derivation’ should be monadic, with a field in <checker>
>>> indicating that.  Sounds complicated though.
>>>
>>> Thoughts?
>>
>> I did wonder if the code could somehow transparently be made more
>> efficient. Quite often database clients manage a pool of connections,
>> and when you perform a database operation, a connection from the pool is
>> checked out, and then returned once you're finished. But as you say,
>> this could be complicated. I think parameters can be set with
>> connections, and I'm not quiet sure what the interface should be.
>>
>> I also did think about somehow passing the store connection in to the
>> lint checker more explicitly, but I'm not sure how to generalise that.
>
> There could be a <checker> field indicating either that (1) the
> procedure takes an optional store parameter, or that (2) the procedure
> is monadic in ‘%store-monad’.
>
> #2 seems more complicated to implement that #1 though.
>
> For #1, ‘guix lint’ could check whether:
>
>   (any checker-require-store? checkers)
>
> is true, and if it is, it could open a connection and pass it on as
> needed.
>
> WDYT?
>
> If that seems good to you, I guess you can go ahead with it (let’s just
> not lose our hair on it!).

I've finally got around to looking at this again. I've sent some new
patches which add a field to the <lint-checker> record, and adjust the
running of lint checkers as well as the derivation checker to use a
single store connection.

Let me know what you think?

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

[bug#38754] [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection parameter.

[Ludovic Courtès]

Hi,

Christopher Baines <mail@cbaines.net> skribis:

> I've finally got around to looking at this again. I've sent some new
> patches which add a field to the <lint-checker> record, and adjust the
> running of lint checkers as well as the derivation checker to use a
> single store connection.
>
> Let me know what you think?

LGTM, thank you!

Ludo’.

bug#38754: [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection parameter.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]


Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> I've finally got around to looking at this again. I've sent some new
>> patches which add a field to the <lint-checker> record, and adjust the
>> running of lint checkers as well as the derivation checker to use a
>> single store connection.
>>
>> Let me know what you think?
>
> LGTM, thank you!

Great, I've pushed this to master now as
57e12aad6dfc2d12567164144dd15161e66f32d5.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

[bug#38754] [PATCH 0/2] Speed up the derivation linter.

[Ludovic Courtès]

Hi,

Christopher Baines <mail@cbaines.net> skribis:

> These patches speed up the derivation linter by reducing the number of
> times a connection to the guix-daemon is established. In testing on my
> computer, the time to lint all guix packages with the derivation linter
> drops from around 6 minutes to around 3 minutes.

Neat.

> Christopher Baines (2):
>   guix: lint: Add an optional parameter for a store connection.
>   scripts: lint: Set the %link-checker-store-connection parameter.

LGTM, thanks!  :-)

Ludo’.