Message ID | 20201111035727.11184-1-maxim.cournoyer@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [bug#44567] publish: Improve HTTP performance when not using --cache. | expand |
Context | Check | Description |
---|---|---|
cbaines/submitting builds | success | |
cbaines/issue | success | View issue |
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
Hi Maxim, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > This change harmonizes the way we configure the buffer sizes and the socket > options, so that we don't forget to change it at one place like it happened in > commit 5e3d169945935b53325e6b738a307ba286751259. Using a greater socket > buffer size reportedly improves throughput tenfold. > > The solution was found by Ludovic Courtès. > > * guix/scripts/publish.scm (%default-buffer-size) > (%default-socket-options): New variables. > * guix/scripts/publish.scm (configure-socket): New procedure. > (compress-nar): Use %default-buffer-size for the buffer size, increased from > 128 to 208 KiB. > (nar-response-port): Likewise, increased from 64 to 208 KiB. > (http-write): Use configure-socket to set socket options. > (open-server-socket): Likewise. Apologies for not noticing this before pushing 1cbda46d4aae5ba9bd89a1837f0d81a29653ed7b. What you propose here is nicer. > +(define %default-buffer-size > + (* 208 1024)) Why 208? Did you notice a difference compared to 128KiB? (I’m fine either way, just wondering.) Perhaps add a comment as to what buffer we’re talking about. > +(define %default-socket-options Maybe add: ;; List of options passed to 'setsockopt' when transmitting files. > + (list (list SO_SNDBUF %default-buffer-size))) > (make-gzip-output-port (response-port response) > #:level level > - #:buffer-size (* 64 1024))) > + #:buffer-size %default-buffer-size)) Does the gzip buffer size have to match the TCP buffer size? I’d say not necessarily, so perhaps we should have a separate variable for the gzip buffer size (or keep it as is). That’s it, thank you! Ludo’.
Hi Ludovic, Ludovic Courtès <ludo@gnu.org> writes: > Hi Maxim, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> This change harmonizes the way we configure the buffer sizes and the socket >> options, so that we don't forget to change it at one place like it happened in >> commit 5e3d169945935b53325e6b738a307ba286751259. Using a greater socket >> buffer size reportedly improves throughput tenfold. >> >> The solution was found by Ludovic Courtès. >> >> * guix/scripts/publish.scm (%default-buffer-size) >> (%default-socket-options): New variables. >> * guix/scripts/publish.scm (configure-socket): New procedure. >> (compress-nar): Use %default-buffer-size for the buffer size, increased from >> 128 to 208 KiB. >> (nar-response-port): Likewise, increased from 64 to 208 KiB. >> (http-write): Use configure-socket to set socket options. >> (open-server-socket): Likewise. > > Apologies for not noticing this before pushing > 1cbda46d4aae5ba9bd89a1837f0d81a29653ed7b. What you propose here is > nicer. No worries! >> +(define %default-buffer-size >> + (* 208 1024)) > > Why 208? Did you notice a difference compared to 128KiB? (I’m fine > either way, just wondering.) I didn't observe any meaningful difference, it perhaps felt more 'steady'. The value comes from what Linux, the kernel, configures as a max value by default (see 'man tcp' and 'cat /proc/sys/net/core/wmem_max'). > Perhaps add a comment as to what buffer we’re talking about. Done. >> +(define %default-socket-options > > Maybe add: ;; List of options passed to 'setsockopt' when transmitting files. > >> + (list (list SO_SNDBUF %default-buffer-size))) > >> (make-gzip-output-port (response-port response) >> #:level level >> - #:buffer-size (* 64 1024))) >> + #:buffer-size %default-buffer-size)) > > Does the gzip buffer size have to match the TCP buffer size? I’d say > not necessarily, so perhaps we should have a separate variable for the > gzip buffer size (or keep it as is). It doesn't, but if we're going to pick some value, it seems the network one (SO_SNDBUF) should be the lowest common denominator. Since we're augmenting the previous value, I've just all made them agree. Another variable should be created the day we have a rationale to give them different values, I'd say. > That’s it, thank you! Thanks for the review! Pushed to the 1.2.0-version branch. Maxim
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > I didn't observe any meaningful difference, it perhaps felt more > 'steady'. The value comes from what Linux, the kernel, configures as a > max value by default (see 'man tcp' and 'cat > /proc/sys/net/core/wmem_max'). I see, makes sense! >>> (make-gzip-output-port (response-port response) >>> #:level level >>> - #:buffer-size (* 64 1024))) >>> + #:buffer-size %default-buffer-size)) >> >> Does the gzip buffer size have to match the TCP buffer size? I’d say >> not necessarily, so perhaps we should have a separate variable for the >> gzip buffer size (or keep it as is). > > It doesn't, but if we're going to pick some value, it seems the network > one (SO_SNDBUF) should be the lowest common denominator. Since we're > augmenting the previous value, I've just all made them agree. OK. >> That’s it, thank you! > > Thanks for the review! Pushed to the 1.2.0-version branch. Great, thanks! Ludo’.
diff --git a/guix/scripts/publish.scm b/guix/scripts/publish.scm index e8faf379e2..8a07f2300e 100644 --- a/guix/scripts/publish.scm +++ b/guix/scripts/publish.scm @@ -2,6 +2,7 @@ ;;; Copyright © 2015 David Thompson <davet@gnu.org> ;;; Copyright © 2020 by Amar M. Singh <nly@disroot.org> ;;; Copyright © 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org> +;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -250,6 +251,18 @@ usage." ("WantMassQuery" . 0) ("Priority" . 100))) +(define %default-buffer-size + (* 208 1024)) + +(define %default-socket-options + (list (list SO_SNDBUF %default-buffer-size))) + +(define* (configure-socket socket #:key (level SOL_SOCKET) + (options %default-socket-options)) + "Apply multiple option tuples in OPTIONS to SOCKET, using LEVEL." + (for-each (cut apply setsockopt socket level <>) + options)) + (define (signed-string s) "Sign the hash of the string S with the daemon's key. Return a canonical sexp for the signature." @@ -569,7 +582,7 @@ requested using POOL." (lambda (port) (write-file item port)) #:level (compression-level compression) - #:buffer-size (* 128 1024)) + #:buffer-size %default-buffer-size) (rename-file (string-append nar ".tmp") nar)) ('lzip ;; Note: the file port gets closed along with the lzip port. @@ -858,7 +871,7 @@ or if EOF is reached." ;; 'make-gzip-output-port' wants a file port. (make-gzip-output-port (response-port response) #:level level - #:buffer-size (* 64 1024))) + #:buffer-size %default-buffer-size)) (($ <compression> 'lzip level) (make-lzip-output-port (response-port response) #:level level)) @@ -883,6 +896,7 @@ blocking." client)) (port (begin (force-output client) + (configure-socket client) (nar-response-port response compression)))) ;; XXX: Given our ugly workaround for <http://bugs.gnu.org/21093> in ;; 'render-nar', BODY here is just the file name of the store item. @@ -912,7 +926,7 @@ blocking." size) client)) (output (response-port response))) - (setsockopt client SOL_SOCKET SO_SNDBUF (* 128 1024)) + (configure-socket client) (if (file-port? output) (sendfile output input size) (dump-port input output)) @@ -1057,7 +1071,8 @@ methods, return the applicable compression." (define (open-server-socket address) "Return a TCP socket bound to ADDRESS, a socket address." (let ((sock (socket (sockaddr:fam address) SOCK_STREAM 0))) - (setsockopt sock SOL_SOCKET SO_REUSEADDR 1) + (configure-socket sock #:options (cons (list SO_REUSEADDR 1) + %default-socket-options)) (bind sock address) sock))