[PATCH] lint: Split the derivation lint checker by system.

  • Open
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Christopher Baines
  • zimoun
Owner
unassigned
Submitted by
Christopher Baines
Severity
normal
C
C
Christopher Baines wrote on 6 Nov 2022 14:55
(address . guix-patches@gnu.org)
20221106135532.5724-1-mail@cbaines.net
Currently, if you attempt to run the derivation checker on all packages, the
Guile process will run out of memory. I think a contributing factor to this is
that the checker forces an inefficient order when you want to generate
derivations for all the supported systems of each package, constantly
switching system then package.

This problem also impacts the Guix Data Service, since it tries to run the
derivation checker for all packages.

The changes in this commit to split the derivation lint checker in to several,
one for each system, means that you can now treat each system separately,
which should be better for caching purposes.

If it's desirable to keep some notion of checking all supported systems for a
single package, I think lint checker groups could be added, so that you could
ask for the "derivation" checker, and this would run all the derivation
checkers.

* guix/lint.scm (check-derivation): Adapt to make-check-derivation-for-system.
(%derivation-checkers): New variable.
(%local-checkers): Include all %derivation-checkers.
* doc/guix.texi (Invoking guix lint): Update.
---
doc/guix.texi | 4 +-
guix/lint.scm | 139 +++++++++++++++++++++++++++++---------------------
2 files changed, 83 insertions(+), 60 deletions(-)

Toggle diff (216 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 7806b21a0f..8d4989a60c 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -14422,9 +14422,9 @@ Parse the @code{source} URL to determine if a tarball from GitHub is
autogenerated or if it is a release tarball. Unfortunately GitHub's
autogenerated tarballs are sometimes regenerated.
-@item derivation
+@item derivation/SYSTEM
Check that the derivation of the given packages can be successfully
-computed for all the supported systems (@pxref{Derivations}).
+computed for the specified system (@pxref{Derivations}).
@item profile-collisions
Check whether installing the given packages in a profile would lead to
diff --git a/guix/lint.scm b/guix/lint.scm
index 8e3976171f..f692856f42 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -52,6 +52,7 @@ (define-module (guix lint)
#:use-module (guix memoization)
#:use-module (guix profiles)
#:use-module (guix monads)
+ #:use-module (guix platform)
#:use-module (guix scripts)
#:use-module ((guix ui) #:select (texi->plain-text fill-paragraph))
#:use-module (guix gnu-maintenance)
@@ -98,7 +99,6 @@ (define-module (guix lint)
check-patch-file-names
check-patch-headers
check-synopsis-style
- check-derivation
check-home-page
check-name
check-source
@@ -116,6 +116,8 @@ (define-module (guix lint)
check-haskell-stackage
check-tests-true
+ make-check-derivation-for-system
+
lint-warning
lint-warning?
lint-warning-package
@@ -1369,56 +1371,6 @@ (define (check-phases-deltas deltas)
(append-map check-phases-delta deltas))
(find-phase-deltas package check-phases-deltas))
-(define* (check-derivation package #:key store)
- "Emit a warning if we fail to compile PACKAGE to a derivation."
- (define (try store system)
- (guard (c ((store-protocol-error? c)
- (make-warning package
- (G_ "failed to create ~a derivation: ~a")
- (list system
- (store-protocol-error-message c))))
- ((exception-with-kind-and-args? c)
- (make-warning package
- (G_ "failed to create ~a derivation: ~s")
- (list system
- (cons (exception-kind c)
- (exception-args c)))))
- ((message-condition? c)
- (make-warning package
- (G_ "failed to create ~a derivation: ~a")
- (list system
- (condition-message c))))
- ((formatted-message? c)
- (let ((str (apply format #f
- (formatted-message-string c)
- (formatted-message-arguments c))))
- (make-warning package
- (G_ "failed to create ~a derivation: ~a")
- (list system str))))
- (else
- (make-warning package
- (G_ "failed to create ~a derivation: ~a")
- (list system c))))
- (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))))))
-
- (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-profile-collisions package #:key store)
"Check for collisions that would occur when installing PACKAGE as a result
of the propagated inputs it pulls in."
@@ -1843,13 +1795,88 @@ (define (check-formatting package)
(G_ "source file not found"))))))))
'())))
+(define (make-check-derivation-for-system system)
+ (define (try package proc)
+ (guard (c ((store-protocol-error? c)
+ (make-warning package
+ (G_ "failed to create ~a derivation: ~a")
+ (list system
+ (store-protocol-error-message c))))
+ ((exception-with-kind-and-args? c)
+ (make-warning package
+ (G_ "failed to create ~a derivation: ~s")
+ (list system
+ (cons (exception-kind c)
+ (exception-args c)))))
+ ((message-condition? c)
+ (make-warning package
+ (G_ "failed to create ~a derivation: ~a")
+ (list system
+ (condition-message c))))
+ ((formatted-message? c)
+ (let ((str (apply format #f
+ (formatted-message-string c)
+ (formatted-message-arguments c))))
+ (make-warning package
+ (G_ "failed to create ~a derivation: ~a")
+ (list system str))))
+ (else
+ (make-warning package
+ (G_ "failed to create ~a derivation: ~a")
+ (list system c))))
+ (proc)))
+
+
+
+ (lambda* (package #:key store)
+ "Emit a warning if we fail to compile PACKAGE to a derivation."
+
+ (define (check-with-store store)
+ (if (member system (package-supported-systems package))
+ (filter
+ lint-warning?
+ (map (cut try package <>)
+ (list
+ (lambda ()
+ (parameterize ((%graft? #f))
+ (package-derivation store package system #:graft? #f)))
+ (lambda ()
+ ;; If there's a replacement, make sure we can compute its
+ ;; derivation.
+ (match (package-replacement package)
+ (#f #t)
+ (replacement
+ (parameterize ((%graft? #f))
+ (package-derivation store replacement system
+ #:graft? #f))))))))
+ '()))
+
+ ;; For backwards compatability, don't rely on store being set
+ (or (and=> store check-with-store)
+ (with-store store
+ (check-with-store store)))))
+
;;;
;;; List of checkers.
;;;
+(define %derivation-checkers
+ (map (lambda (system)
+ (lint-checker
+ (name (string->symbol
+ (simple-format #f "derivation/~A" system)))
+ (description
+ (simple-format
+ #f
+ "Report failure to compile a package to a derivation for ~A"
+ system))
+ (check (make-check-derivation-for-system system))
+ (requires-store? #t)))
+ (systems)))
+
(define %local-checkers
- (list
+ (cons*
(lint-checker
(name 'name)
(description "Validate package names")
@@ -1901,11 +1928,6 @@ (define %local-checkers
(name 'source-unstable-tarball)
(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)
- (requires-store? #t))
(lint-checker
(name 'profile-collisions)
(description "Report collisions that would occur due to propagated inputs")
@@ -1922,7 +1944,8 @@ (define %local-checkers
(lint-checker
(name 'formatting)
(description "Look for formatting issues in the source")
- (check check-formatting))))
+ (check check-formatting))
+ %derivation-checkers))
(define %network-dependent-checkers
(list
--
2.37.3
C
C
Christopher Baines wrote on 7 Nov 2022 18:37
(name . Christopher Baines)(address . mail@cbaines.net)
87cz9ypshf.fsf@cbaines.net
Christopher Baines <mail@cbaines.net> writes:

Toggle quote (3 lines)
> This problem also impacts the Guix Data Service, since it tries to run the
> derivation checker for all packages.

This patch has now been processed by qa.guix.gnu.org. Looking at the
logs for the Guix Data Service processing the base and target revision,
and the change is more significant than I'd imagined:

Base:

inferior heap after cleanup: 1739.0 MiB used (5160.0 MiB heap)
debug: Finished getting formatting lint warnings, took 349 seconds
debug: Finished fetching inferior lint warnings, took 3782 seconds

Target:

inferior heap after cleanup: 1152.0 MiB used (1778.0 MiB heap)
debug: Finished getting derivation/aarch64-linux lint warnings, took 334 seconds
debug: Finished fetching inferior lint warnings, took 3285 seconds


So with the changes, it's a little faster, but the main difference is
that the heap ~3GiB smaller, so ~34% of what it was previously.

I did notice that this also subtly differs from how the linter behaved
previously, since some packages define support for systems not defined
through the platform module.


Personally, I think this change is still a good one. Maybe we can add a
separate linter to go round and check that packages don't declare
support for systems that aren't in the platform module.

Unless anyone objects, I'll like to push this sooner rather than later,
as I think the excessive heap size in the inferior process is not ideal.

Thanks,

Chris
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmNpRIxfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XflUg//bzvSQ+wbaAL3FO9EQz6zGmVeDOgqbkEe
rX+iCqLk0ZvYAzTMUYmz+z55ozfJChkhB5TwA71s68hOKZQFEvkI6lwUylaryy4a
ovkttrcn6hEtr5E0KglToltSj5F8IHlQ3BT4uc0re2/zWIfrsFUEbjI2HLH99xyA
CKxH1ENy0GboVZBH8SHYRLTTWefMRkMUCq6c2hWpZeOSYHuqf4Gk6KXc6oEln6js
FBkFeH9N1V2607/KakrCY851zevbjXc5LD+0OXXN6ALR9qXzrb+gp00U/5gJ54zd
pNlzZSanRC8R+LM3bAKOQxe1womVv3/QIczJCTclXL7NTVT/5a3WYlrbIldoPMhN
ET2XVjRzHz4WheprtTzkm/F2JdcbFPNsra9K+uDzRHX2AH98JzklDhq58AzsxF+s
hXge0BCv3rsJpjgQHtp8sW26VdyBssuZYMqEE9XbX+Jzvr++DtN4rgDu2nXWJOX/
RMns3KiK6U5yP3Gq6dI8YCtdxreHPbAPjfH829H5/gPCqE4wDcyinb1YCCAwsy7K
4ELkFjg/L7CEFfiuB5N+wuGq2bpxF2YNwWMs5QMq/tlGGSSXkGVWDwsaxKS+Y0XI
GBzO77Hsjxm1fNpuBowP0ZaQKYnBtEwl5gbUYkeAnIDBqgdZECcGPTGOEh/jEQFN
KQljNj7kxtE=
=ALR6
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 11 Nov 2022 22:57
Re: bug#59078: [PATCH] lint: Split the derivation lint checker by system.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 59078@debbugs.gnu.org)
87k041dui7.fsf@gnu.org
Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (18 lines)
> Currently, if you attempt to run the derivation checker on all packages, the
> Guile process will run out of memory. I think a contributing factor to this is
> that the checker forces an inefficient order when you want to generate
> derivations for all the supported systems of each package, constantly
> switching system then package.
>
> This problem also impacts the Guix Data Service, since it tries to run the
> derivation checker for all packages.
>
> The changes in this commit to split the derivation lint checker in to several,
> one for each system, means that you can now treat each system separately,
> which should be better for caching purposes.
>
> If it's desirable to keep some notion of checking all supported systems for a
> single package, I think lint checker groups could be added, so that you could
> ask for the "derivation" checker, and this would run all the derivation
> checkers.

The ‘derivation’ checker was added for this purpose: making sure that a
package’s derivation can be computed for all the supported systems.
Previously it was easy to overlook that kind of breakage.

I think it’s important to keep a ‘derivation’ checker that does this.


Now, the memory consumption you report is unacceptable and this needs to
be addressed.

Most (all?) caches are now per-session (associated with
<store-connection>). Since ‘guix lint’ uses a single session, those
caches keep growing because there’s no eviction mechanism in place.

A hack like the one below should work around that. Could you check how
well it works for you? It can also be helpful to set:

GUIX_PROFILING=gc GUIX_PROFILING_EVENTS=after-gc

Longer-term we should have a proper cache eviction mechanism in place.
It’s not been a priority because “normal” applications such as ‘guix
package’ don’t need it.

Thanks,
Ludo’.
Toggle diff (55 lines)
diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 9920c3ee62..6d9b9e96a9 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -209,23 +209,31 @@ (define (parse-options)
(list-checkers-and-exit checkers))
(with-error-handling
- (let ((any-lint-checker-requires-store?
- (any lint-checker-requires-store? checkers)))
+ (let ((store-needed? (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 (package)
- (run-checkers package checkers
- #:store store))
- args)))))))))
+ (cond
+ ((null? args)
+ (let loop ((packages (fold-packages cons '()))
+ (processed 0)
+ (store #f))
+ (match packages
+ ((package . rest)
+ (let ((store (and store-needed?
+ (if store
+ ;; XXX: Periodically start a new session
+ ;; with empty caches to avoid unbounded
+ ;; heap growth caused by the 'derivation'
+ ;; checker.
+ (if (zero? (modulo processed 1000))
+ (begin
+ (close-connection store)
+ (open-connection))
+ store)
+ (open-connection)))))
+ (run-checkers package checkers #:store store)
+ (loop rest (+ 1 processed) store))))))
+ (else
+ (for-each (lambda (package)
+ (run-checkers package checkers
+ #:store store))
+ args)))))))
C
C
Christopher Baines wrote on 13 Nov 2022 18:27
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 59078@debbugs.gnu.org)
87bkpau4kl.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (26 lines)
> Christopher Baines <mail@cbaines.net> skribis:
>
>> Currently, if you attempt to run the derivation checker on all packages, the
>> Guile process will run out of memory. I think a contributing factor to this is
>> that the checker forces an inefficient order when you want to generate
>> derivations for all the supported systems of each package, constantly
>> switching system then package.
>>
>> This problem also impacts the Guix Data Service, since it tries to run the
>> derivation checker for all packages.
>>
>> The changes in this commit to split the derivation lint checker in to several,
>> one for each system, means that you can now treat each system separately,
>> which should be better for caching purposes.
>>
>> If it's desirable to keep some notion of checking all supported systems for a
>> single package, I think lint checker groups could be added, so that you could
>> ask for the "derivation" checker, and this would run all the derivation
>> checkers.
>
> The ‘derivation’ checker was added for this purpose: making sure that a
> package’s derivation can be computed for all the supported systems.
> Previously it was easy to overlook that kind of breakage.
>
> I think it’s important to keep a ‘derivation’ checker that does this.

What aspect of it do you think is important?

I realise just splitting the checker in to several doesn't quite pick up
on all the same problems, but an additional checker could be added that
flags when packages support systems that don't appear as platforms. I
think that would catch everything that the derivation checker currently
does.

If it's a "we need a singular checker" kind of problem, I was thinking
of having some kind of lint checker groups. So you could have a group
called "derivation", which runs all of the relevant checkers.

Toggle quote (10 lines)
> Now, the memory consumption you report is unacceptable and this needs to
> be addressed.
>
> Most (all?) caches are now per-session (associated with
> <store-connection>). Since ‘guix lint’ uses a single session, those
> caches keep growing because there’s no eviction mechanism in place.
>
> A hack like the one below should work around that. Could you check how
> well it works for you?

I tried in the Guix Data Service processing packages in chunks of 1000
plus closing the store connection after each batch, and that led to a
heap size of 3090MiB. But this is still higher than 1778MiB heap usage I
got just by splitting the derivation linter.

Fundamentally, I think it's still going to be more memory/time efficient
to move towards processing the derivations by system, rather than by
package.

Thanks,

Chris
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmNxLlpfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XezfA/9FHf6b7aHZTFe1eu4v2YibxOX8g0s87O1
2GVyMEG5qc3ome2130/430dATjtdQu20SIyswavSHLTQTPchFuwTi3AE0z5un15l
doeMEHtM+PHDgTl0alxE9UAfWIRuliUnGzbtjJco921zbVr+pPQ9qT+//XFlAfZx
X64XVcvlvGELVww0i5mrLVbukeI8rErcsHW0EcLkBw5El4JxN+54CDr3t6KnF+2O
yh63HVgfWj9GOZUDQH1iuEktAoRccZl5CWKHO8qHVaeW4nt/0wrvAj/XO3XI4xXp
elqg7WRZdj5fPFHqvwBtrFnMa/QI2+vBsJ7qvN1XDLWhxfeSgiDF7Hoh20gVowHt
3KcEWp0tN092ciyTFr1UrQeLeXxz+rxmo55gytQzINqJxzVAOWDQKgTq2v1bx4+w
/DGS32Br7aZQiTBEhVaoQQVGX7rZHhFEKLL1algNHCKKdAhROkKIANBV43Go54SP
M2/lRY2j0CvSRcbwxAo0nWNcwM1/zMK/knjenQ1GUpd1W9ysKlfcXw7emjwAR1TU
SmxlMUKqlnoMTYj0lt/HG+ueAOpvpmGUaGSM5311us0CdJ7x5c8VqQm8lNfAfBGB
RspmDeyYm0cO3lgngq4SD5aCMPLtDRK6S5lVQ9SFumXPBfkNmYe21jOO9fpOo814
xXLfKg+C/LI=
=/WZa
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 14 Nov 2022 13:51
(name . Christopher Baines)(address . mail@cbaines.net)(address . 59078@debbugs.gnu.org)
87h6z1puli.fsf@gnu.org
Hi!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (2 lines)
> Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (8 lines)
>> The ‘derivation’ checker was added for this purpose: making sure that a
>> package’s derivation can be computed for all the supported systems.
>> Previously it was easy to overlook that kind of breakage.
>>
>> I think it’s important to keep a ‘derivation’ checker that does this.
>
> What aspect of it do you think is important?

I meant that it’s important to have a single ‘derivation’ checker that
checks derivations for all the supported systems. Packagers should be
able to run ‘guix lint -c derivation PKG’ and be confident that it’s
fine for all systems.

Toggle quote (13 lines)
>> Now, the memory consumption you report is unacceptable and this needs to
>> be addressed.
>>
>> Most (all?) caches are now per-session (associated with
>> <store-connection>). Since ‘guix lint’ uses a single session, those
>> caches keep growing because there’s no eviction mechanism in place.
>>
>> A hack like the one below should work around that. Could you check how
>> well it works for you?
>
> I tried in the Guix Data Service processing packages in chunks of 1000
> plus closing the store connection after each batch,

How was it implemented? Was it after the caches came into
<store-connection>?

Toggle quote (3 lines)
> and that led to a heap size of 3090MiB. But this is still higher than
> 1778MiB heap usage I got just by splitting the derivation linter.

I didn’t take the time to do it, but it would be nice to see, with the
patch I gave, how ‘guix lint -c derivations’ behaves.

Thanks,
Ludo’.
C
C
Christopher Baines wrote on 15 Nov 2022 09:35
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 59078@debbugs.gnu.org)
87leocfsnm.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (21 lines)
> Hi!
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> The ‘derivation’ checker was added for this purpose: making sure that a
>>> package’s derivation can be computed for all the supported systems.
>>> Previously it was easy to overlook that kind of breakage.
>>>
>>> I think it’s important to keep a ‘derivation’ checker that does this.
>>
>> What aspect of it do you think is important?
>
> I meant that it’s important to have a single ‘derivation’ checker that
> checks derivations for all the supported systems. Packagers should be
> able to run ‘guix lint -c derivation PKG’ and be confident that it’s
> fine for all systems.

I think we can still keep that by adding support for grouping lint
checkers. So have a derivation group, instead of a single checker.

Maybe that'll change the command slightly to 'guix lint -g derivation
PKG', but I think that can be equivalent.

Toggle quote (16 lines)
>>> Now, the memory consumption you report is unacceptable and this needs to
>>> be addressed.
>>>
>>> Most (all?) caches are now per-session (associated with
>>> <store-connection>). Since ‘guix lint’ uses a single session, those
>>> caches keep growing because there’s no eviction mechanism in place.
>>>
>>> A hack like the one below should work around that. Could you check how
>>> well it works for you?
>>
>> I tried in the Guix Data Service processing packages in chunks of 1000
>> plus closing the store connection after each batch,
>
> How was it implemented? Was it after the caches came into
> <store-connection>?

I'm not sure what aspect of the implementation is important, but I think
it's working correctly. Closing the store connection wasn't very
easy. Previously there was a fresh store connection with each call to
inferior-eval-with-store, but for this test I close the connections in
%store-table and then clear the hash table between the
inferior-eval-with-store calls.

Toggle quote (6 lines)
>> and that led to a heap size of 3090MiB. But this is still higher than
>> 1778MiB heap usage I got just by splitting the derivation linter.
>
> I didn’t take the time to do it, but it would be nice to see, with the
> patch I gave, how ‘guix lint -c derivations’ behaves.

I've put some numbers below, with no changes the last batch to finish
processing leaves the heap at 7755MiB [1], then Guile crashes after
that.

With the patch you sent, the heap size seems to stabilise at 4042MiB
[2]. It also crashes at the end due to the match block not matching '(),
but that's not important.

I also hacked the lint script to run the checkers in the same manor as
the Guix Data Service, so one checker at a time rather than one package
at a time. That led to a max heap size of 3505MiB [3].

By adding in batching (as the Guix Data Service already does), I think
it's possible to further reduce this to the 1778MiB number I give above.

Reducing the memory usage helps reduce the cost/improve the throughput
of loading data in to the Guix Data Service which is my primary
motivation here. I'm also not only concerned with reducing the peak
memory usage, but trying to have an implementation that'll gracefully
handle more systems being supported in the future.

It's for that second point that I think arranging the derivation linting
so that it's possible to process each system in turn is important for
the Guix Data Service, so that when new platforms are added, the memory
usage won't grow as much.


1: no batching, one derivation checker
1065.0 MiB
1409.0 MiB
2089.0 MiB
2297.0 MiB
2513.0 MiB
2705.0 MiB
3077.0 MiB
3373.0 MiB
3557.0 MiB
3661.0 MiB
3901.0 MiB
3997.0 MiB
4147.0 MiB
4491.0 MiB
4635.0 MiB
5899.0 MiB
7755.0 MiB

2: batches of 1000 with fresh store connection, one derivation checker
1057.0 MiB
1137.0 MiB
1481.0 MiB
1481.0 MiB
1481.0 MiB
1481.0 MiB
1697.0 MiB
1697.0 MiB
1697.0 MiB
1761.0 MiB
1761.0 MiB
1761.0 MiB
1937.0 MiB
1977.0 MiB
1985.0 MiB
3065.0 MiB
3633.0 MiB
4041.0 MiB
4042.0 MiB
4042.0 MiB
4042.0 MiB

3: multiple derivation checkers, batched by system
1297.0 MiB
1545.0 MiB
1873.0 MiB
2113.0 MiB
2353.0 MiB
2609.0 MiB
2961.0 MiB
3193.0 MiB
3505.0 MiB
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmNzYi1fFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9Xf2Tg/+MEKPBt1Ibu6sWQD9Xia/nR7MTimBHcOw
dA8/WR13He3MwjxwaBjSI+T09RVYokukCYbpf4UjkxgU3np+bDyAUj7XA/C01m6b
67jYpuZ0+11Jgm4iLbZWh8O7YVX+K8hilnLynFTEU4X5ns0way4L1pGrN19LmnPt
dS0Dh7Ul3j5rCHMMWjCrrK9/p51snm12lZCV1uk9bvhYrkoEC/ByO/1T9o3FS7h7
WD3dmBN3eqwtfe4VsPVBf+6XREBDlxDTBPlxsGd+RsmghFkR5k34WQP5otyAP+R7
CpeKZ2K/BoU1CJduz5fMQQUtAumASGBS+symysyIUyRIp2hcZyHTp4YbvbkjxwIe
9CS/Oq/qIQFoOpFfX8qGb/23EJc761YnO+73Pjk42ggWwO+nl45tN20T4jwUTblJ
7L+RoKBhK+pdn/ojJfFdk1CoSnwM0sihkhkuEAaNJ6lDr8wYSb2q5O73ZzKoBCUT
lw1PrPDNEv3D2BQ1w1mO1dste6X0Jxb7ZbIGUJRGLkU+p4+RII3N59/EXceiq69w
WOaFaYvo/Cij5y2ZPGocIgZofHaFImd3A1oBoa3YgaAA2hopQqugbDBLz6WLnVMR
+gIMgzd0UhHoPGIXI3ZsQkGsYW3eA1sY++hBxzkciBMG4n/+etsfk9SJswWz7je9
QpTiSyazp4M=
=m6Vq
-----END PGP SIGNATURE-----

Z
Z
zimoun wrote on 15 Nov 2022 10:03
Re: [bug#59078] [PATCH] lint: Split the derivation lint checker by system.
(address . 59078@debbugs.gnu.org)
86r0y4io7q.fsf@gmail.com
Hi,

On Mon, 14 Nov 2022 at 13:51, Ludovic Courtès <ludo@gnu.org> wrote:

Toggle quote (5 lines)
> I meant that it’s important to have a single ‘derivation’ checker that
> checks derivations for all the supported systems. Packagers should be
> able to run ‘guix lint -c derivation PKG’ and be confident that it’s
> fine for all systems.

The CLI invokation is unrelated to the invoked checkers, no? As Chris
is proposing, it seems being worth to group some checkers. For
instance, we already have the option ’-n, --no-network’ which does that
but probably at the wrong level.

Maybe we could have another command line option,

guix lint --group=no-network
guix lint --group=derivation
guix lint --group=no-network,derivation

I do not know…


Cheers,
simon
L
L
Ludovic Courtès wrote on 17 Nov 2022 18:22
Re: bug#59078: [PATCH] lint: Split the derivation lint checker by system.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 59078@debbugs.gnu.org)
87a64ph4wu.fsf@gnu.org
Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (12 lines)
> I've put some numbers below, with no changes the last batch to finish
> processing leaves the heap at 7755MiB [1], then Guile crashes after
> that.
>
> With the patch you sent, the heap size seems to stabilise at 4042MiB
> [2]. It also crashes at the end due to the match block not matching '(),
> but that's not important.
>
> I also hacked the lint script to run the checkers in the same manor as
> the Guix Data Service, so one checker at a time rather than one package
> at a time. That led to a max heap size of 3505MiB [3].

Thanks for testing and reporting back!

Toggle quote (6 lines)
> Reducing the memory usage helps reduce the cost/improve the throughput
> of loading data in to the Guix Data Service which is my primary
> motivation here. I'm also not only concerned with reducing the peak
> memory usage, but trying to have an implementation that'll gracefully
> handle more systems being supported in the future.

Understood. From the viewpoint of core Guix, I’d like to understand
where all this memory is going; this is unreasonable. The heap profiler
I posted recently¹, coupled with ‘gcprof’, might help us understand
what’s going on.


Toggle quote (2 lines)
> 1: no batching, one derivation checker

[...]

Toggle quote (25 lines)
> 7755.0 MiB
>
> 2: batches of 1000 with fresh store connection, one derivation checker
> 1057.0 MiB
> 1137.0 MiB
> 1481.0 MiB
> 1481.0 MiB
> 1481.0 MiB
> 1481.0 MiB
> 1697.0 MiB
> 1697.0 MiB
> 1697.0 MiB
> 1761.0 MiB
> 1761.0 MiB
> 1761.0 MiB
> 1937.0 MiB
> 1977.0 MiB
> 1985.0 MiB
> 3065.0 MiB
> 3633.0 MiB
> 4041.0 MiB
> 4042.0 MiB
> 4042.0 MiB
> 4042.0 MiB

This seems to indicate a memory leak outside <store-connection>, such as
a static cache. “GUIX_PROFILING=memoization” may give a hint.

I’d really like to get it solved before we come up with workarounds like
those we’re talking about. (However I’ll postpone this one because I’d
like to focus on the release for now.)

Thanks,
Ludo’.
S
S
Simon Tournier wrote on 27 Jan 2023 18:48
(name . Ludovic Courtès)(address . ludo@gnu.org)
875ycroom4.fsf_-_@gmail.com
Hi,

On mar., 15 nov. 2022 at 10:03, zimoun <zimon.toutoune@gmail.com> wrote:
Toggle quote (18 lines)
> On Mon, 14 Nov 2022 at 13:51, Ludovic Courtès <ludo@gnu.org> wrote:
>
>> I meant that it’s important to have a single ‘derivation’ checker that
>> checks derivations for all the supported systems. Packagers should be
>> able to run ‘guix lint -c derivation PKG’ and be confident that it’s
>> fine for all systems.
>
> The CLI invokation is unrelated to the invoked checkers, no? As Chris
> is proposing, it seems being worth to group some checkers. For
> instance, we already have the option ’-n, --no-network’ which does that
> but probably at the wrong level.
>
> Maybe we could have another command line option,
>
> guix lint --group=no-network
> guix lint --group=derivation
> guix lint --group=no-network,derivation

What about this?


Cheers,
simon
L
L
Ludovic Courtès wrote on 31 Jan 2023 17:33
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87tu06u0iz.fsf@gnu.org
Hi,

Simon Tournier <zimon.toutoune@gmail.com> skribis:

Toggle quote (21 lines)
> On mar., 15 nov. 2022 at 10:03, zimoun <zimon.toutoune@gmail.com> wrote:
>> On Mon, 14 Nov 2022 at 13:51, Ludovic Courtès <ludo@gnu.org> wrote:
>>
>>> I meant that it’s important to have a single ‘derivation’ checker that
>>> checks derivations for all the supported systems. Packagers should be
>>> able to run ‘guix lint -c derivation PKG’ and be confident that it’s
>>> fine for all systems.
>>
>> The CLI invokation is unrelated to the invoked checkers, no? As Chris
>> is proposing, it seems being worth to group some checkers. For
>> instance, we already have the option ’-n, --no-network’ which does that
>> but probably at the wrong level.
>>
>> Maybe we could have another command line option,
>>
>> guix lint --group=no-network
>> guix lint --group=derivation
>> guix lint --group=no-network,derivation
>
> What about this?

In general, being able to tell which category a checker belongs to, and
then being able to select checkers by categories sounds like a useful
improvement to me.

I don’t think it solves the problem Christopher initially reported
though (about memory consumption of the ‘derivation’ checker.)

Ludo’.
Z
Z
zimoun wrote on 1 Feb 2023 10:47
Re: [bug#59078] [PATCH] lint: Split the derivation lint checker by system.
(name . Ludovic Courtès)(address . ludo@gnu.org)
868rhhvhsz.fsf@gmail.com
Hi Ludo,

On Tue, 31 Jan 2023 at 17:33, Ludovic Courtès <ludo@gnu.org> wrote:

Toggle quote (7 lines)
> In general, being able to tell which category a checker belongs to, and
> then being able to select checkers by categories sounds like a useful
> improvement to me.
>
> I don’t think it solves the problem Christopher initially reported
> though (about memory consumption of the ‘derivation’ checker.)

Indeed. My understanding of the proposal is to split some checkers (as
derivation) and then group them to have the expected behaviour. You
wrote [1]:

I meant that it’s important to have a single ‘derivation’
checker that checks derivations for all the supported systems.
Packagers should be able to run ‘guix lint -c derivation PKG’
and be confident that it’s fine for all systems.

and Chris answered [2]:

I think we can still keep that by adding support for grouping
lint checkers. So have a derivation group, instead of a single
checker.

Maybe that'll change the command slightly to 'guix lint -g derivation
PKG', but I think that can be equivalent.

where I try to feed Chris’s proposal. :-)

I do not know if it is the correct level but being able to run “smaller“
checkers appears to me worth to try.


Cheers,
simon
?
Your comment

Commenting via the web interface is currently disabled.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 59078
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