New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP/2 performance regression #470

Closed
kazu-yamamoto opened this Issue Nov 26, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@kazu-yamamoto
Contributor

kazu-yamamoto commented Nov 26, 2015

Now, I established HTTP/2 benchmark environment with h2load.

Unfortunately, it appeared that the current HTTP/2 performance is really bad. With mighty conf route +RTS -A32m -N16, we get:

finished in 62.97s,  12500.51 req/s, 7.65MB/s

If we use warp-3.1.3, we get:

finished in  5.15s, 194156.77 req/s, 117.59MB/s

Both are plain HTTP/2, not HTTP/2 over TLS.

For record, plain HTTP/1.1 results in:

finished in 3.97s, 252197.84 req/s, 193.85MB/s

@awpr I guess that this regression is caused by turning files to streamings. After fixing #459, I will consider how to improve the HTTP/2 performance.

@kazu-yamamoto kazu-yamamoto self-assigned this Nov 26, 2015

@kazu-yamamoto kazu-yamamoto added the http2 label Nov 26, 2015

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Dec 2, 2015

I confirmed that push API is not related to this regression. So, I believe that the main reason is converting files to streams. This introduces the following overhead for files:

  • Executing one more worker
  • Executing a waiter
  • Synchronizing the sender and a waiter
  • Manipulating TBQueue
@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Dec 2, 2015

Random thoughts for performance:

  • Don't converting files and builders to streams
  • For streams, a waiter should be executed on demand. That is, when the sender notices that a TBQueue is empty, the sender executes a waiter for the TBQueue. When TBQueue becomes non-empty, the waiter enqueues the TBQueue to the priority queue and exits.
@awpr

This comment has been minimized.

Member

awpr commented Dec 2, 2015

Since the only reason for removing the file case in the HTTP/2 API was that it didn't (appear to) have any benefit there, and it made it slightly simpler for a response to be just a function rather than an ADT, I'd agree it makes sense to add files and builders back as a top-level alternative to streams if there's a significant performance benefit.

As far as I know, I'm the only current user of HTTP2Application, so changing it doesn't cost much (other than a version bump).

As long as we're changing it, there's something else that's been bothering me: there's no easy way for clients of WAI to make an application that opportunistically uses HTTP/2 features if they're available. For example, to have Yesod automatically push resources referenced by a page, it would have to make an HTTP2Application that does so, and separately an Application that doesn't. To fix this, we need a type that's a superset of both Application and HTTP2Application (i.e. which can be projected onto either to get a full-featured application). To make things simpler, that type may as well be HTTP2Application. Practically, I think that just means adding a ResponseRaw equivalent as well as the file and builder responses (and all HTTP/2 handlers would always serve the backup response for it). Then Yesod could target HTTP2Application using server push freely, and downgrade it to HTTP/1.1.

@awpr

This comment has been minimized.

Member

awpr commented Dec 2, 2015

Another thought: if I recall correctly, the way workers are currently set up means that only 10 requests can be executing pre-Response at a time (i.e. inside the IO action returned by Responder). This is because the worker only requests to be replaced with another one when it starts streaming.

Changing files to streams moved more work into that IO action, which might severely limit throughput by introducing contention over this part of the request. I could imagine this being a major factor in the performance difference. Do things get better if you increase the number of idling workers to 100 or 1000? If so, it might make sense to look closer at how workers are spawned and replaced.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Dec 9, 2015

My observations:

  • We definitely need the push API.
  • I don't like the current trailer API because it does not make use of sender's buffer. For instance, checksum should be repeatedly calculated over the buffer and sent as a trailer.
  • The idea of HTTP2Appliction and promotion is good.
  • Network.WAI.Internal should not provide the range APIs. They should be implemented either in http-types or Warp inside
@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Dec 9, 2015

I may increase the number of workers from 10 to 20, but not to 100 (or larger). Remember the max concurrency is typically 100. So, for instance, 1,000 is meaningless. Increasing the number of workers results in increasing contention to the output priority queue. I think we should keep it as small as possible.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Dec 9, 2015

@awpr I'm very sorry. But I would like to start over the HTTP/2 implementation from Warp 3.1.3. It's a pity to say that this removes all your efforts.

I believe that the performance of HTTP/2 should be better than HTTP/1. For example, h2o gets such a result. After improving performance of HTTP/2 based on Warp 3.1.3, I would like to add the push API again.

Here is a topic branch for this: https://github.com/yesodweb/wai/tree/http2-starting-over

@snoyberg If we merge this topic branch, some APIs will be removed from WAI, WarpTLS and Warp. Is it OK to bump the major versions? Or, can we find a way to maintain backward compatibility?

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Dec 14, 2015

Pinging @snoyberg

@snoyberg

This comment has been minimized.

Member

snoyberg commented Dec 14, 2015

We can bump the major versions, but if we do so, I'd like to do it sooner rather than later so it gets picked up by LTS 4.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Dec 16, 2015

Merged. This topic branch improves the performance of HTTP/2 and ensures no space leak.

finished in 4.59s, 217877.46 req/s, 133.20MB/s

But HTTP/2 is still slower than HTTP/1 at this moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment