Re: Patch: Improve HTTP time by sending Connection:close

[Andrei Borzenkov]

On Tue, Nov 15, 2016 at 4:21 PM, Daniel Kiper <address@hidden> wrote:
> On Sat, Nov 12, 2016 at 09:26:17AM -0800, Walter Huf wrote:
>> GRUB's HTTP module declares support for HTTP/1.1, which defaults to
>> Connection:keepalive. At the end of the content, the server holds the TCP
>> connection open waiting for the next request.
>> It seems that grub_net_poll_cards() is watching for the HTTP module to set
>
> Do you have a feeling or are you sure? I think that we have to be sure here.
>

See http://savannah.gnu.org/bugs/?49531 which has more details.

>> net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to return
>> to processing. However, HTTP module only sets that flag in specific
>> conditions:
>>
>>    - parse_line detects that we are at the end of downloading a chunked
>>    Transfer-Encoding
>>    - http_err detects a problem with the underlying TCP connection
>>    - http_receive has queued 20 netbuffer packets for processing
>>
>> If the file is small and takes less than 20 packets, and the server is not
>> using chunked encoding, grub_net_poll_cards() will wait the full 400ms
>> before continuing to process and finish the file download.
>>
>> This patch sets Connection:close, which will tell the server to close the
>> connection as soon as it has finished sending the file. GRUB closes any
>> connections that are left open (in http_seek), so it does not change
>
> Why in http_seek? Is it correct or not?
>
>> performance. When the server disconnects, I think it triggers http_err and
>
> "I think" is too soft. You have to be sure here.
>
>> then quits out of grub_net_poll_cards early.
>
> Lack of SOB.
>

I must have missed it. Is it now required?

> Have you tested this patch with large/huge number of files to transfer? I have
> a feeling that it can slowdown whole transfer in such cases due to number of
> connects/disconnects. Maybe this feature should be conditional thing.
>

Currently grub will always establish new connection in http_open ->
http_establish, so it should not change anything from grub PoV, but
reduce amount of lingering connections on server. But I would really
like to explore another patch in above bug report - gracefully close
connection when we finished receive data. That said, this one does not
hurt as long as we do not reuse existing connection.