You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ic...@apache.org on 2015/10/30 12:29:51 UTC
svn commit: r1711451 - in /httpd/httpd/trunk/modules/http2: h2_conn_io.c
h2_conn_io.h h2_session.c h2_stream.c h2_stream.h
Author: icing
Date: Fri Oct 30 11:29:50 2015
New Revision: 1711451
URL: http://svn.apache.org/viewvc?rev=1711451&view=rev
Log:
fixing unbuffered output handling for h2c
Modified:
httpd/httpd/trunk/modules/http2/h2_conn_io.c
httpd/httpd/trunk/modules/http2/h2_conn_io.h
httpd/httpd/trunk/modules/http2/h2_session.c
httpd/httpd/trunk/modules/http2/h2_stream.c
httpd/httpd/trunk/modules/http2/h2_stream.h
Modified: httpd/httpd/trunk/modules/http2/h2_conn_io.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn_io.c?rev=1711451&r1=1711450&r2=1711451&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_conn_io.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_conn_io.c Fri Oct 30 11:29:50 2015
@@ -52,28 +52,9 @@ apr_status_t h2_conn_io_init(h2_conn_io
io->input = apr_brigade_create(c->pool, c->bucket_alloc);
io->output = apr_brigade_create(c->pool, c->bucket_alloc);
io->buflen = 0;
- /* That is where we start with,
- * see https://issues.apache.org/jira/browse/TS-2503 */
- io->tls_warmup_size = h2_config_geti64(cfg, H2_CONF_TLS_WARMUP_SIZE);
- io->tls_cooldown_usecs = (h2_config_geti(cfg, H2_CONF_TLS_COOLDOWN_SECS)
- * APR_USEC_PER_SEC);
- io->write_size = WRITE_SIZE_INITIAL;
- io->last_write = 0;
- io->buffer_output = h2_h2_is_tls(c);
-
- if (APLOGctrace1(c)) {
- ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, io->connection,
- "h2_conn_io(%ld): init, buffering=%d, warmup_size=%ld, cd_secs=%f",
- io->connection->id, io->buffer_output, (long)io->tls_warmup_size,
- ((float)io->tls_cooldown_usecs/APR_USEC_PER_SEC));
- }
- /* Currently we buffer only for TLS output. The reason this gives
- * improved performance is that buckets send to the mod_ssl network
- * filter will be encrypted in chunks. There is a special filter
- * that tries to aggregate data, but that does not work well when
- * bucket sizes alternate between tiny frame headers and large data
- * chunks.
- */
+ io->is_tls = h2_h2_is_tls(c);
+ io->buffer_output = io->is_tls;
+
if (io->buffer_output) {
io->bufsize = WRITE_BUFFER_SIZE;
io->buffer = apr_pcalloc(c->pool, io->bufsize);
@@ -82,6 +63,27 @@ apr_status_t h2_conn_io_init(h2_conn_io
io->bufsize = 0;
}
+ if (io->is_tls) {
+ /* That is where we start with,
+ * see https://issues.apache.org/jira/browse/TS-2503 */
+ io->warmup_size = h2_config_geti64(cfg, H2_CONF_TLS_WARMUP_SIZE);
+ io->cooldown_usecs = (h2_config_geti(cfg, H2_CONF_TLS_COOLDOWN_SECS)
+ * APR_USEC_PER_SEC);
+ io->write_size = WRITE_SIZE_INITIAL;
+ }
+ else {
+ io->warmup_size = 0;
+ io->cooldown_usecs = 0;
+ io->write_size = io->bufsize;
+ }
+
+ if (APLOGctrace1(c)) {
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, io->connection,
+ "h2_conn_io(%ld): init, buffering=%d, warmup_size=%ld, cd_secs=%f",
+ io->connection->id, io->buffer_output, (long)io->warmup_size,
+ ((float)io->cooldown_usecs/APR_USEC_PER_SEC));
+ }
+
return APR_SUCCESS;
}
@@ -91,6 +93,11 @@ void h2_conn_io_destroy(h2_conn_io *io)
io->output = NULL;
}
+int h2_conn_io_is_buffered(h2_conn_io *io)
+{
+ return io->bufsize > 0;
+}
+
static apr_status_t h2_conn_io_bucket_read(h2_conn_io *io,
apr_read_type_e block,
h2_conn_io_on_read_cb on_read_cb,
@@ -206,10 +213,10 @@ static apr_status_t pass_out(apr_bucket_
}
ap_update_child_status(io->connection->sbh, SERVER_BUSY_WRITE, NULL);
- status = apr_brigade_length(bb, 1, &bblen);
+ status = apr_brigade_length(bb, 0, &bblen);
if (status == APR_SUCCESS) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, io->connection,
- "h2_conn_io(%ld): flush, brigade %ld bytes",
+ "h2_conn_io(%ld): pass_out brigade %ld bytes",
io->connection->id, (long)bblen);
status = ap_pass_brigade(io->connection->output_filters, bb);
if (status == APR_SUCCESS) {
@@ -231,8 +238,8 @@ static apr_status_t bucketeer_buffer(h2_
int bcount, i;
if (io->write_size > WRITE_SIZE_INITIAL
- && (io->tls_cooldown_usecs > 0)
- && (apr_time_now() - io->last_write) >= io->tls_cooldown_usecs) {
+ && (io->cooldown_usecs > 0)
+ && (apr_time_now() - io->last_write) >= io->cooldown_usecs) {
/* long time not written, reset write size */
io->write_size = WRITE_SIZE_INITIAL;
io->bytes_written = 0;
@@ -241,7 +248,7 @@ static apr_status_t bucketeer_buffer(h2_
(long)io->connection->id, (long)io->write_size);
}
else if (io->write_size < WRITE_SIZE_MAX
- && io->bytes_written >= io->tls_warmup_size) {
+ && io->bytes_written >= io->warmup_size) {
/* connection is hot, use max size */
io->write_size = WRITE_SIZE_MAX;
ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, io->connection,
@@ -272,7 +279,7 @@ apr_status_t h2_conn_io_write(h2_conn_io
apr_status_t status = APR_SUCCESS;
io->unflushed = 1;
- if (io->buffer_output) {
+ if (io->bufsize > 0) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, io->connection,
"h2_conn_io: buffering %ld bytes", (long)length);
while (length > 0 && (status == APR_SUCCESS)) {
@@ -297,12 +304,20 @@ apr_status_t h2_conn_io_write(h2_conn_io
}
}
+ else if (1) {
+ apr_bucket *b;
+
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, io->connection,
+ "h2_conn_io: passing %ld transient bytes to output filters",
+ (long)length);
+ b = apr_bucket_transient_create(buf,length, io->output->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(io->output, b);
+ status = pass_out(io->output, io);
+ }
else {
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, io->connection,
+ "h2_conn_io: writing %ld bytes to brigade", (long)length);
status = apr_brigade_write(io->output, pass_out, io, buf, length);
- if (status != APR_SUCCESS) {
- ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, io->connection,
- "h2_conn_io: write error");
- }
}
return status;
Modified: httpd/httpd/trunk/modules/http2/h2_conn_io.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn_io.h?rev=1711451&r1=1711450&r2=1711451&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_conn_io.h (original)
+++ httpd/httpd/trunk/modules/http2/h2_conn_io.h Fri Oct 30 11:29:50 2015
@@ -26,15 +26,16 @@ typedef struct {
conn_rec *connection;
apr_bucket_brigade *input;
apr_bucket_brigade *output;
- int buffer_output;
+
+ int is_tls;
+ apr_time_t cooldown_usecs;
+ apr_int64_t warmup_size;
+
int write_size;
apr_time_t last_write;
apr_int64_t bytes_written;
- apr_time_t tls_cooldown_usecs;
- apr_int64_t tls_warmup_size;
-
-
+ int buffer_output;
char *buffer;
apr_size_t buflen;
apr_size_t bufsize;
@@ -42,8 +43,11 @@ typedef struct {
} h2_conn_io;
apr_status_t h2_conn_io_init(h2_conn_io *io, conn_rec *c);
+
void h2_conn_io_destroy(h2_conn_io *io);
+int h2_conn_io_is_buffered(h2_conn_io *io);
+
typedef apr_status_t (*h2_conn_io_on_read_cb)(const char *data, apr_size_t len,
apr_size_t *readlen, int *done,
void *puser);
Modified: httpd/httpd/trunk/modules/http2/h2_session.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_session.c?rev=1711451&r1=1711450&r2=1711451&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_session.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_session.c Fri Oct 30 11:29:50 2015
@@ -249,6 +249,8 @@ static int before_frame_send_cb(nghttp2_
case NGHTTP2_GOAWAY:
session->flush = 1;
break;
+ case NGHTTP2_DATA:
+
default:
break;
@@ -557,6 +559,10 @@ static int on_send_data_cb(nghttp2_sessi
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c,
+ "h2_stream(%ld-%d): send_data_cb for %ld bytes",
+ session->id, (int)stream_id, (long)length);
+
status = send_data(session, (const char *)framehd, 9);
if (status == APR_SUCCESS) {
if (padlen) {
@@ -1124,6 +1130,27 @@ apr_status_t h2_session_close(h2_session
/* The session wants to send more DATA for the given stream.
*/
+
+typedef struct {
+ char *buf;
+ size_t offset;
+ h2_session *session;
+ h2_stream *stream;
+} cpy_ctx;
+
+static apr_status_t copy_buffer(void *ctx, const char *data, apr_size_t len)
+{
+ cpy_ctx *c = ctx;
+
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c->session->c,
+ "h2_stream(%ld-%d): copy %ld bytes for DATA #%ld",
+ c->session->id, c->stream->id,
+ (long)c->stream->data_frames_sent, (long)len);
+ memcpy(c->buf + c->offset, data, len);
+ c->offset += len;
+ return APR_SUCCESS;
+}
+
static ssize_t stream_data_cb(nghttp2_session *ng2s,
int32_t stream_id,
uint8_t *buf,
@@ -1153,9 +1180,25 @@ static ssize_t stream_data_cb(nghttp2_se
AP_DEBUG_ASSERT(!h2_stream_is_suspended(stream));
- status = h2_stream_prep_read(stream, &nread, &eos);
- if (nread) {
- *data_flags |= NGHTTP2_DATA_FLAG_NO_COPY;
+ if (h2_conn_io_is_buffered(&session->io)) {
+ status = h2_stream_prep_read(stream, &nread, &eos);
+ if (nread) {
+ *data_flags |= NGHTTP2_DATA_FLAG_NO_COPY;
+ }
+ }
+ else {
+ cpy_ctx ctx;
+ ctx.buf = (char *)buf;
+ ctx.offset = 0;
+ ctx.session = session;
+ ctx.stream = stream;
+
+ status = h2_stream_readx(stream, copy_buffer, &ctx, &nread, &eos);
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, session->c,
+ "h2_stream(%ld-%d): read %ld bytes (DATA #%ld)",
+ session->id, (int)stream_id, (long)nread,
+ (long)stream->data_frames_sent);
+ stream->data_frames_sent++;
}
switch (status) {
Modified: httpd/httpd/trunk/modules/http2/h2_stream.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream.c?rev=1711451&r1=1711450&r2=1711451&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_stream.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_stream.c Fri Oct 30 11:29:50 2015
@@ -112,6 +112,7 @@ apr_status_t h2_stream_set_response(h2_s
stream->bbout = apr_brigade_create(stream->pool,
stream->m->c->bucket_alloc);
}
+ /* TODO: this does not move complete file buckets.*/
status = h2_util_move(stream->bbout, bb, 16 * 1024, NULL,
"h2_stream_set_response");
}
@@ -271,8 +272,7 @@ apr_status_t h2_stream_prep_read(h2_stre
}
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, stream->m->c,
"h2_stream(%ld-%d): prep_read %s, len=%ld eos=%d",
- stream->m->id, stream->id,
- src, (long)*plen, *peos);
+ stream->m->id, stream->id, src, (long)*plen, *peos);
return status;
}
@@ -280,14 +280,31 @@ apr_status_t h2_stream_readx(h2_stream *
h2_io_data_cb *cb, void *ctx,
apr_size_t *plen, int *peos)
{
+ apr_status_t status = APR_SUCCESS;
+ const char *src;
+
if (stream->rst_error) {
return APR_ECONNRESET;
}
if (stream->bbout && !APR_BRIGADE_EMPTY(stream->bbout)) {
- return h2_util_bb_readx(stream->bbout, cb, ctx, plen, peos);
+ src = "stream";
+ status = h2_util_bb_readx(stream->bbout, cb, ctx, plen, peos);
+ if (status == APR_SUCCESS && !*peos && !*plen) {
+ apr_brigade_cleanup(stream->bbout);
+ return h2_stream_readx(stream, cb, ctx, plen, peos);
+ }
}
- return h2_mplx_out_readx(stream->m, stream->id,
- cb, ctx, plen, peos);
+ else {
+ src = "mplx";
+ status = h2_mplx_out_readx(stream->m, stream->id, cb, ctx, plen, peos);
+ }
+ if (status == APR_SUCCESS && !*peos && !*plen) {
+ status = APR_EAGAIN;
+ }
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, stream->m->c,
+ "h2_stream(%ld-%d): readx %s, len=%ld eos=%d",
+ stream->m->id, stream->id, src, (long)*plen, *peos);
+ return status;
}
Modified: httpd/httpd/trunk/modules/http2/h2_stream.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream.h?rev=1711451&r1=1711450&r2=1711451&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_stream.h (original)
+++ httpd/httpd/trunk/modules/http2/h2_stream.h Fri Oct 30 11:29:50 2015
@@ -59,6 +59,7 @@ struct h2_stream {
int aborted; /* was aborted */
int suspended; /* DATA sending has been suspended */
+ apr_size_t data_frames_sent;/* # of DATA frames sent out for this stream */
apr_pool_t *pool; /* the memory pool for this stream */
struct h2_request *request; /* the request made in this stream */
Re: svn commit: r1711451 - in /httpd/httpd/trunk/modules/http2: h2_conn_io.c h2_conn_io.h h2_session.c h2_stream.c h2_stream.h
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 01.11.2015 um 08:27 schrieb Christophe JAILLET <ch...@wanadoo.fr>:
>
> Hi,
>
> Le 30/10/2015 12:29, icing@apache.org a écrit :
>> [...]+ else if (1) {
>> + apr_bucket *b;
>> +
>> + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, io->connection,
>> + "h2_conn_io: passing %ld transient bytes to output filters",
>> + (long)length);
>> + b = apr_bucket_transient_create(buf,length, io->output->bucket_alloc);
>> + APR_BRIGADE_INSERT_TAIL(io->output, b);
>> + status = pass_out(io->output, io);
>> + }
>> else {
>> + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, io->connection,
>> + "h2_conn_io: writing %ld bytes to brigade", (long)length);
>> status = apr_brigade_write(io->output, pass_out, io, buf, length);
>> - if (status != APR_SUCCESS) {
>> - ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, io->connection,
>> - "h2_conn_io: write error");
>> - }
>> }
>> return status;
>
> Is the 'if (1)' intentional or a left-over?
Left-over about to be removed. Sorry for committing it like this. Fridays...
>> Modified: httpd/httpd/trunk/modules/http2/h2_session.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_session.c?rev=1711451&r1=1711450&r2=1711451&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http2/h2_session.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_session.c Fri Oct 30 11:29:50 2015
>> @@ -249,6 +249,8 @@ static int before_frame_send_cb(nghttp2_
>> case NGHTTP2_GOAWAY:
>> session->flush = 1;
>> break;
>> + case NGHTTP2_DATA:
>> +
>> default:
>> break;
>
> Is this intentional or a break, some code or a /* Fall through */ is missing?
A fall through that proved no longer necessary. removed.
Thanks for the review!
//Stefan
Re: svn commit: r1711451 - in /httpd/httpd/trunk/modules/http2:
h2_conn_io.c h2_conn_io.h h2_session.c h2_stream.c h2_stream.h
Posted by Christophe JAILLET <ch...@wanadoo.fr>.
Hi,
Le 30/10/2015 12:29, icing@apache.org a écrit :
> Author: icing
> Date: Fri Oct 30 11:29:50 2015
> New Revision: 1711451
>
> URL: http://svn.apache.org/viewvc?rev=1711451&view=rev
> Log:
> fixing unbuffered output handling for h2c
>
>
> Modified: httpd/httpd/trunk/modules/http2/h2_conn_io.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn_io.c?rev=1711451&r1=1711450&r2=1711451&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http2/h2_conn_io.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_conn_io.c Fri Oct 30 11:29:50 2015
> [...]
> @@ -297,12 +304,20 @@ apr_status_t h2_conn_io_write(h2_conn_io
> }
>
> }
> + else if (1) {
> + apr_bucket *b;
> +
> + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, io->connection,
> + "h2_conn_io: passing %ld transient bytes to output filters",
> + (long)length);
> + b = apr_bucket_transient_create(buf,length, io->output->bucket_alloc);
> + APR_BRIGADE_INSERT_TAIL(io->output, b);
> + status = pass_out(io->output, io);
> + }
> else {
> + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, io->connection,
> + "h2_conn_io: writing %ld bytes to brigade", (long)length);
> status = apr_brigade_write(io->output, pass_out, io, buf, length);
> - if (status != APR_SUCCESS) {
> - ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, io->connection,
> - "h2_conn_io: write error");
> - }
> }
>
> return status;
Is the 'if (1)' intentional or a left-over?
> Modified: httpd/httpd/trunk/modules/http2/h2_session.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_session.c?rev=1711451&r1=1711450&r2=1711451&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http2/h2_session.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_session.c Fri Oct 30 11:29:50 2015
> @@ -249,6 +249,8 @@ static int before_frame_send_cb(nghttp2_
> case NGHTTP2_GOAWAY:
> session->flush = 1;
> break;
> + case NGHTTP2_DATA:
> +
> default:
> break;
Is this intentional or a break, some code or a /* Fall through */ is
missing?
CJ