| Message ID | 0ec7f0270fcccec730808f9210f074cd5339961f.camel@telenet.be |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#50384,v2] Optimise search-patch (reducing I/O) | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
Maxime Devos schreef op zo 05-09-2021 om 21:48 [+0200]: > Ludovic Courtès schreef op zo 05-09-2021 om 00:04 [+0200]: > > Maxime Devos <maximedevos@telenet.be> skribis: > > [..] > I added two patches adding (limited) dependency tracking to compile-all.scm. > If a patch file is now modified or deleted, the corresponding package modules > will be recompiled. This should remove the ‘evilness’ I think. Oops, I forgot to include the following change to build-aux/compile-all.scm: (or (not (file-exists? go)) (file-mtime<? go file) - (any (cut file-mtime<? go <>) extra-dependencies)))) + (any (lambda (dependency) + (or (not (file-exists? dependency)) + (file-mtime<? go dependency))) extra-dependencies)))) It will be included in the v3. Greetings, Maxime.
Hi Maxime, Thanks for looking at optimising stuff. :-) On Sun, 05 Sep 2021 at 21:48, Maxime Devos <maximedevos@telenet.be> wrote: > Let's compare the numbers again! This time, I've run > > echo powersave |sudo tee /sys/devices/system/cpu/cpufreq/policy{0,1,2,3}/scaling_governor > > to make sure the CPU frequency doesn't change. On a hot (disk) cache: > > # After the patch series > time GUIX_PROFILING="rpc gc" ./the-optimised-guix/bin/guix build -d --no-grafts pigx [...] > So on a hot disk cache, there doesn't appear to be any improvement > (except for ‘time spent in GC’ -- presumably that's due to the optimisations > to guix/base32.scm). Which kind of disk do you have? SSD, spinning HDD, other? > What about a cold cache? > > # After the patch series > > sync && echo 3 | sudo tee /proc/sys/vm/drop_caches > ./the-optimised-guix/bin/guix --help > time GUIX_PROFILING="rpc gc" ./the-optimised-guix/bin/guix build -d --no-grafts pigx > /gnu/store/fq6x8d2vcm6sbjkimg7g8kcgb4c5xv1b-pigx-0.0.3.drv > Remote procedure call summary: 5949 RPCs > built-in-builders ... 1 > add-to-store ... 3 > add-to-store/tree ... 26 > add-temp-root ... 195 > valid-path? ... 195 > add-text-to-store ... 5529 > Garbage collection statistics: > heap size: 93.85 MiB > allocated: 312.03 MiB > GC times: 17 > time spent in GC: 3.37 seconds (23% of user time) > > real 1m39,178s > user 0m14,557s > sys 0m0,990s How the average (against 3 examples) looks like? > It seems that if the disk cache is cold, the time-to-derivation decreases > a little by this patch series. Much less than I had hoped for though; I'll > have to look into other areas for interesting performance gains ... Please update the number using your diff showed in [1]. :-) 1: <http://issues.guix.gnu.org/50384#4> All the best, simon
zimoun schreef op ma 06-09-2021 om 10:39 [+0200]: > Hi Maxime, > > Thanks for looking at optimising stuff. :-) > > On Sun, 05 Sep 2021 at 21:48, Maxime Devos <maximedevos@telenet.be> wrote: > > > Let's compare the numbers again! This time, I've run > > > > echo powersave |sudo tee /sys/devices/system/cpu/cpufreq/policy{0,1,2,3}/scaling_governor > > > > to make sure the CPU frequency doesn't change. On a hot (disk) cache: > > > > # After the patch series > > time GUIX_PROFILING="rpc gc" ./the-optimised-guix/bin/guix build -d --no-grafts pigx > > [...] > > > So on a hot disk cache, there doesn't appear to be any improvement > > (except for ‘time spent in GC’ -- presumably that's due to the optimisations > > to guix/base32.scm). > > Which kind of disk do you have? SSD, spinning HDD, other? A spinning disk, presumably a HDD. FWIW, it's a ‘TOSHIBA MQ01ABD100 (AX1P2C)’ that has been ‘on’ for 10 months and 10 days, according to ‘SMART-data and selftests’ I just noticed it has ‘2304 bad sectors’. Maybe I should make backups and run file system checks? > > > What about a cold cache? > > > > # After the patch series > > > > sync && echo 3 | sudo tee /proc/sys/vm/drop_caches > > ./the-optimised-guix/bin/guix --help > > time GUIX_PROFILING="rpc gc" ./the-optimised-guix/bin/guix build -d --no-grafts pigx > > /gnu/store/fq6x8d2vcm6sbjkimg7g8kcgb4c5xv1b-pigx-0.0.3.drv > > Remote procedure call summary: 5949 RPCs > > built-in-builders ... 1 > > add-to-store ... 3 > > add-to-store/tree ... 26 > > add-temp-root ... 195 > > valid-path? ... 195 > > add-text-to-store ... 5529 > > Garbage collection statistics: > > heap size: 93.85 MiB > > allocated: 312.03 MiB > > GC times: 17 > > time spent in GC: 3.37 seconds (23% of user time) > > > > real 1m39,178s > > user 0m14,557s > > sys 0m0,990s > > How the average (against 3 examples) looks like? I'll try to optimise more things and report the average for the v3. > > It seems that if the disk cache is cold, the time-to-derivation decreases > > a little by this patch series. Much less than I had hoped for though; I'll > > have to look into other areas for interesting performance gains ... > > Please update the number using your diff showed in [1]. :-) > > > 1: <http://issues.guix.gnu.org/50384#4> Are you referring to: > Oops, I forgot to include the following change to build-aux/compile-all.scm: > (or (not (file-exists? go)) > (file-mtime<? go file) > - (any (cut file-mtime<? go <>) extra-dependencies)))) > + (any (lambda (dependency) > + (or (not (file-exists? dependency)) > + (file-mtime<? go dependency))) extra-dependencies)))) That's compilation-time only, so that cannot affect the timing of "guix build -d ...". Geetings, Maxime.
Hello, Maxime Devos <maximedevos@telenet.be> skribis: >> To address this, ‘local-file’ could store the inode/mtime + computed >> store file name (rather than the SHA256). ‘local-file-compiler’ would >> check whether the actual file has matching inode/mtime before returning >> the computed store file name. Problem is that inode/mtime are >> guaranteed to differ once you’ve run “make install”. :-/ > > An additional problem is that 'local-file-compiler' would have to 'stat' > the file even if it is already in the store, undoing the (fairly limited?) > performance gains of this patch series. > > The dependency tracking avoids this. OK. >> Intuitively, I’d have imagined a cache populated at run time; it would >> map, say, file name/inode/mtime to a store file name. ‘add-to-store’ >> (or some wrapper above it) would check the cache and return the store >> file name directly, unless ‘valid-path?’ says it no longer exists. >> Downside is that this would be a per-user cache and you’d still pay the >> cost until it’s warm. Advantage is that you could easily tell whether >> it’s stale. >> >> Thoughts? > > Intuitively, I'd have imagined doing as much as possible at compilation time. Of course, but it’s important for the caching model to match “reality”, which is that patch files live independently of the source files that refer to them. I’d all be fine if ‘local-file’ were to inline file contents at macro-expansion time, because then we could be sure the hash and contents match (but I’m not saying we should do this…). What we could do is have a boolean saying whether the cached value is authoritative, similar to what’s in (gnu packages). That way, when using ./pre-inst-env or passing a -L flag or setting GUIX_PACKAGE_PATH, the cached value would not be authoritative; we’d be safe, without needing ad hoc dependency tracking. Thoughts? [...] > Because fixed-output-path is now called more often, I've added a patch > optimising (guix base32). [...] > From e5dc46800597023dfc1c9d53cc6e0db2f3999022 Mon Sep 17 00:00:00 2001 > From: Maxime Devos <maximedevos@telenet.be> > Date: Sat, 4 Sep 2021 15:35:51 +0200 > Subject: [PATCH v2 3/9] gexp: Allow computing the hash of the local file in > advance. > > The new field is currently unused. The following patches will > populate and use the field to reduce the time-to-derivation > when the file is already interned in the store. > > * guix/gexp.scm > (<local-file>): Add sha256 field. > (%local-file): Add sha256 argument for populating the field. > (local-file-compiler): Adjust 'match' expression. [...] > +;; repeated 'stat' calls. Allow computing the hash of the file in advance, > +;; to avoid having to send the file to the daemon when it is already interned > +;; in the store. > (define-record-type <local-file> > - (%%local-file file absolute name recursive? select?) > + (%%local-file file absolute name sha256 recursive? select?) > local-file? > (file local-file-file) ;string > (absolute %local-file-absolute-file-name) ;promise string > (name local-file-name) ;string > + (sha256 local-file-sha256) ;sha256 bytevector | #f Could we store the result of ‘fixed-output-path’ rather than the SHA256, while we’re at it? Again, care must be taken because it’s possible to set NIX_STORE_DIR at run time, which may invalidate the pre-computed store file name. Can we make hash/file name computation a feature of ‘local-file’ rather than one of ‘search-patch’ as in these patches? I’d rather not provide a way to override this new field. There are cases where we cannot know the value of ‘recursive?’ at expansion time, for instance if the user wrote: (local-file "foo.txt" #:recursive? r) In that case, we cannot compute the hash or file name. Thanks, Ludo’.
From ccbaa1a4b447c67f997a63494a82c8e2b905dccd Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sun, 5 Sep 2021 16:28:33 +0200
Subject: [PATCH v2 9/9] base32: Reduce GC pressure in
make-bytevector->base32-string.
The following code has been used to compare performance:
;; first 20 bytes of sha256 of #vu8(#xde #xad #xbe #xef)
(define bv #vu8(95 120 195 50 116 228 63 169 222 86 89 38 92 29 145 126 37 192 55 34))
,profile
(let loop ((n 0))
(when (< n #e1e6)
((@ (guix base32) bytevector->nix-base32-string) bv)
(loop (+ n 1))))
Before this change, the output was:
[...]
Sample count: 1140
Total time: 27.465560018 seconds (10.659331433 seconds in GC)
After this change, the output was:
[...]
Sample count: 957
Total time: 20.478847143 seconds (6.139721189 seconds in GC)
* guix/base32.scm
(make-bytevector->base32-string): Eliminate 'reverse', use mutation instead.
---
guix/base32.scm | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/guix/base32.scm b/guix/base32.scm
index 49f191ba26..e76bf35ecc 100644
--- a/guix/base32.scm
+++ b/guix/base32.scm
@@ -141,12 +141,18 @@ the previous application or INIT."
(define (make-bytevector->base32-string quintet-fold base32-chars)
(lambda (bv)
"Return a base32 encoding of BV using BASE32-CHARS as the alphabet."
- (let ((chars (quintet-fold (lambda (q r)
- (cons (vector-ref base32-chars q)
- r))
- '()
- bv)))
- (list->string (reverse chars)))))
+ ;; Mutation can be avoided with 'reverse'. However, that would
+ ;; make this procedure about 30% slower due to the extra GC pressure.
+ (let* ((start (cons #f #f))
+ (end (quintet-fold (lambda (q r)
+ (define pair
+ (cons (vector-ref base32-chars q) #f))
+ (set-cdr! r pair)
+ pair)
+ start
+ bv)))
+ (set-cdr! end '())
+ (list->string (cdr start)))))
(define %nix-base32-chars
;; See `libutil/hash.cc'.
--
2.33.0