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