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

Done

Details

2 participants

Ludovic Courtès
Christopher Baines

Owner: unassigned

Submitted by: Christopher Baines

Severity: normal

Debbugs page

Christopher Baines wrote 5 years ago

Recipients:(address . guix-patches@gnu.org)

Message-ID:8736d7rnzg.fsf@cbaines.net

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(-)

Christopher Baines wrote 5 years ago

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

Recipients:(address . 38754@debbugs.gnu.org)

Message-ID:20191226180104.10888-1-mail@cbaines.net

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(-)

Toggle diff (81 lines)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))
 
 
 ;;;
@@ -142,6 +144,9 @@
     ((_ package (G_ message) rest ...)
      (%make-warning package message rest ...))))
 
+(define %lint-checker-store-connection
+  (make-parameter #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

Christopher Baines wrote 5 years ago

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

Recipients:(address . 38754@debbugs.gnu.org)

Message-ID:20191226180104.10888-2-mail@cbaines.net

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(-)

Toggle diff (39 lines)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

Ludovic Courtès wrote 5 years ago

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

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 38754@debbugs.gnu.org)

Message-ID:8736d11nim.fsf@gnu.org

Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (5 lines)

> 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.

Toggle quote (4 lines)

> 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’.

Ludovic Courtès wrote 5 years ago

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

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 38754@debbugs.gnu.org)

Message-ID:87lfqtzcma.fsf@gnu.org

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (4 lines)

> + (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’.

Christopher Baines wrote 5 years ago

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 38754@debbugs.gnu.org)

Message-ID:878smtqtfm.fsf@cbaines.net

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

Toggle quote (9 lines)

> 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?

Toggle quote (7 lines)

> 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

Ludovic Courtès wrote 5 years ago

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 38754@debbugs.gnu.org)

Message-ID:87lfqsxsxn.fsf@gnu.org

Hi Chris!

Christopher Baines <mail@cbaines.net> skribis:

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

Toggle quote (17 lines)>> 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’.

Christopher Baines wrote 5 years ago

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

Recipients:(address . 38754@debbugs.gnu.org)

Message-ID:20200315210631.5334-4-mail@cbaines.net

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(-)

Toggle diff (70 lines)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

Christopher Baines wrote 5 years ago

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

Recipients:(address . 38754@debbugs.gnu.org)

Message-ID:20200315210631.5334-3-mail@cbaines.net

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(-)

Toggle diff (62 lines)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

Christopher Baines wrote 5 years ago

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

Recipients:(address . 38754@debbugs.gnu.org)

Message-ID:20200315210631.5334-2-mail@cbaines.net

* 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(-)

Toggle diff (20 lines)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

Christopher Baines wrote 5 years ago

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

Recipients:(address . 38754@debbugs.gnu.org)

Message-ID:20200315210631.5334-1-mail@cbaines.net

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(-)

Toggle diff (27 lines)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?))
 
 
 ;;;
@@ -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

Christopher Baines wrote 5 years ago

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

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 38754@debbugs.gnu.org)

Message-ID:87mu8hcndr.fsf@cbaines.net

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

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

Toggle quote (35 lines)>>> 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

Ludovic Courtès wrote 5 years ago

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 38754@debbugs.gnu.org)

Message-ID:87imiunjf4.fsf@gnu.org

Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (7 lines)

> 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’.

Christopher Baines wrote 5 years ago

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 38754-done@debbugs.gnu.org)

Message-ID:87eeth5y8g.fsf@cbaines.net

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

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

Closed

Your comment

This issue is archived.

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

To respond to this issue using the mumi CLI, first switch to it

mumi current 38754

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

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date