[bug#44567] [PATCH] publish: Improve HTTP performance when not using --cache.

[bug#44567] [PATCH] publish: Improve HTTP performance when not using --cache.

[Maxim Cournoyer]

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.
---
 guix/scripts/publish.scm | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

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

[bug#44567] [PATCH] publish: Improve HTTP performance when not using --cache.

[Ludovic Courtès]

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

bug#44567: [PATCH] publish: Improve HTTP performance when not using --cache.

[Maxim Cournoyer]

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

[bug#44567] [PATCH] publish: Improve HTTP performance when not using --cache.

[Ludovic Courtès]

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