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

[Daniel Kiper]

On Tue, Nov 15, 2016 at 01:43:16PM -0800, Walter Huf wrote:
> GRUB has a bug where it waits a minimum of 400ms for every file it fetches
> over HTTP, unless the server serves it with Transfer-Encoding:chunked or
> the file just happens to be split into 20 TCP packets. When using
> pxeboot.img built with just pxe and http module (following instructions
> from https://www.gnu.org/software/grub/manual/html_node/Network.html), this
> causes an initial text menu to take about 7 seconds to load with all the
> modules being dynamically fetched.

I agree that this looks like a bug.

> The SOB (statement of benefit?) of this patch is to fix this bug with the
> smallest change to existing data structures and logic.

I meant Signed-off-by: ...
In your case it should look like:

Signed-off-by: Walter Huf <address@hidden>

> GRUB specifically checks for existing connections that are attached to file
> objects when seeking and closes them. New file requests don't have any
> sockets attached, and so it always opens a new connection to fetch new
> files. Adding the Connection:close header does not reduce performance.

I am not sure that I correctly understand what is written above but it seems
to me that we have a problem with HTTP connection close. So, I think that
there are two ways to fix it:
  - if server accepts Connection:keepalive then we should use existing
    connection as long as possible and transfer all/(maximum number of?)
    files,
  - if it does not then we should close HTTP connection immediately
    after file transfer and open another one for a new transfer.

> The alternate patch on that bug ticket is a larger change, involving
> changing a (module-internal) data structure. I was also less sure of the
> flow of logic in http_receive(), so I did not immediately suggest it. It
> seems that the GRUB project is understandably conservative about changes,
> so I first provided the change that would have the smallest side-effect.

Patch size is important factor but not crucial one. First of all it
should properly fix a given bug and do not introduce regressions.

> I couched my phrasing with "I believe" and "I think" because I am not sure
> if I've fully understood the 3000+ lines of undocumented C code in net.c,
> netbuff.c, http.c, and tcp.c in only one week. I submitted this bug to

I prefer to spend more time on code/functionality analysis than provide fix 
which
is based on vague imagination. So, that is why I am asking for more details.

> bring a performance problem to your attention, and included a possible

Thanks for doing that!

> patch that fixes it for me, in the hope that someone on the project who is
> familiar with this code can review it and offer feedback.

I think that we are doing that right now.

> If the suggestion is to instead implement a more complete implementation of
> Connection:keepalive, I can try that out and offer a rough patch to improve

I am not convinced that we should play with Connection:keepalive.
Please look above.

> that. As a newcomer to this project, I would feel more comfortable
> contributing a small change to fix the problem immediately instead of a
> large possibly-breaking change.

As I said earlier, size of a given patch is not very important (at least for 
me).
It have to be correct. So, please dive into the code and understand how it 
works.
If you are not sure send questions to the list then somebody familiar with a 
given
chunk will try to explain what is going on. We are happy to help. Though you 
must
put some effort to provide correct patch too. Otherwise it does not work.

Daniel