From 1a3158ad94c92639e07d4c3ef74dd019dbc485db Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Fri, 14 Aug 2015 14:07:43 +0000 Subject: [PATCH] removed HackMpm and MaxWrite config directives, added dynamic write size behaviour according to recommendations from Ilya Grigorik similar to Apache Traffic Server git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1695920 13f79535-47bb-0310-9956-ffa450edef68 --- docs/manual/mod/mod_h2.xml | 34 ----------------------- modules/http2/h2_config.c | 48 -------------------------------- modules/http2/h2_config.h | 5 ---- modules/http2/h2_conn.c | 8 ++---- modules/http2/h2_conn_io.c | 56 +++++++++++++++++++++++++++++--------- modules/http2/h2_conn_io.h | 4 ++- 6 files changed, 48 insertions(+), 107 deletions(-) diff --git a/docs/manual/mod/mod_h2.xml b/docs/manual/mod/mod_h2.xml index 47a0e99c56..47d8a3a702 100644 --- a/docs/manual/mod/mod_h2.xml +++ b/docs/manual/mod/mod_h2.xml @@ -280,40 +280,6 @@ - - H2BufferWriteMax - Maximum size of write on a HTTP/2 connection. - H2BufferWriteMax bytes - H2BufferWriteMax 16384 - - server config - virtual host - - -

- This directive sets maximum amount of data sent out in a single - write on a http/2 connection. It only takes effect when - H2BufferOutput is switched on. -

- This directive affects performance of underlying TLS transports. TLS - transforms each write into an encrypted record. Clients need - to receive all of the record in order to decrypt it. Larger sizes - result in better server performance, shorter sizes can affect web - page paint timings. -

- BufferSize should be a multiple of H2BufferWriteMax. - H2BufferWriteMax, if larger than 16k, should be a multiple of 16k, - since this is the TLS max record size. Be aware that there are TLS - extensions to limit the record size to powers of 2 less than 16k. -

- Example - - H2BufferWriteMax 8000 - - - - - H2SessionExtraFiles Number of Extra File Handles diff --git a/modules/http2/h2_config.c b/modules/http2/h2_config.c index 4a589f3243..8bb267ec6f 100644 --- a/modules/http2/h2_config.c +++ b/modules/http2/h2_config.c @@ -46,11 +46,9 @@ static h2_config defconf = { NULL, /* no alt-svcs */ -1, /* alt-svc max age */ 0, /* serialize headers */ - 1, /* hack mpm event */ 1, /* h2 direct mode */ -1, /* buffer output, by default only for TLS */ 64*1024, /* buffer size */ - 16*1024, /* out write max */ 5, /* # session extra files */ }; @@ -76,12 +74,9 @@ static void *h2_config_create(apr_pool_t *pool, conf->stream_max_mem_size = DEF_VAL; conf->alt_svc_max_age = DEF_VAL; conf->serialize_headers = DEF_VAL; - conf->hack_mpm_event = DEF_VAL; conf->h2_direct = DEF_VAL; conf->buffer_output = DEF_VAL; - conf->buffer_output = DEF_VAL; conf->buffer_size = DEF_VAL; - conf->write_max = DEF_VAL; conf->session_extra_files = DEF_VAL; return conf; } @@ -121,11 +116,9 @@ void *h2_config_merge(apr_pool_t *pool, void *basev, void *addv) n->alt_svcs = add->alt_svcs? add->alt_svcs : base->alt_svcs; n->alt_svc_max_age = H2_CONFIG_GET(add, base, alt_svc_max_age); n->serialize_headers = H2_CONFIG_GET(add, base, serialize_headers); - n->hack_mpm_event = H2_CONFIG_GET(add, base, hack_mpm_event); n->h2_direct = H2_CONFIG_GET(add, base, h2_direct); n->buffer_output = H2_CONFIG_GET(add, base, buffer_output); n->buffer_size = H2_CONFIG_GET(add, base, buffer_size); - n->write_max = H2_CONFIG_GET(add, base, write_max); n->session_extra_files = H2_CONFIG_GET(add, base, session_extra_files); return n; @@ -152,16 +145,12 @@ int h2_config_geti(h2_config *conf, h2_config_var_t var) return H2_CONFIG_GET(conf, &defconf, alt_svc_max_age); case H2_CONF_SER_HEADERS: return H2_CONFIG_GET(conf, &defconf, serialize_headers); - case H2_CONF_HACK_MPM_EVENT: - return H2_CONFIG_GET(conf, &defconf, hack_mpm_event); case H2_CONF_DIRECT: return H2_CONFIG_GET(conf, &defconf, h2_direct); case H2_CONF_BUFFER_OUTPUT: return H2_CONFIG_GET(conf, &defconf, buffer_output); case H2_CONF_BUFFER_SIZE: return H2_CONFIG_GET(conf, &defconf, buffer_size); - case H2_CONF_WRITE_MAX: - return H2_CONFIG_GET(conf, &defconf, write_max); case H2_CONF_SESSION_FILES: return H2_CONFIG_GET(conf, &defconf, session_extra_files); default: @@ -304,22 +293,6 @@ static const char *h2_conf_set_buffer_size(cmd_parms *parms, return NULL; } -static const char *h2_conf_set_write_max(cmd_parms *parms, - void *arg, const char *value) -{ - h2_config *cfg = h2_config_sget(parms->server); - apr_int64_t max = (int)apr_atoi64(value); - if (max <= 0) { - return "value must be a positive number"; - } - else if (max > cfg->buffer_size) { - return "value must be less than H2BufferSize"; - } - cfg->write_max = (int)max; - (void)arg; - return NULL; -} - static const char *h2_conf_set_session_extra_files(cmd_parms *parms, void *arg, const char *value) { @@ -350,23 +323,6 @@ static const char *h2_conf_set_serialize_headers(cmd_parms *parms, return "value must be On or Off"; } -static const char *h2_conf_set_hack_mpm_event(cmd_parms *parms, - void *arg, const char *value) -{ - h2_config *cfg = h2_config_sget(parms->server); - if (!strcasecmp(value, "On")) { - cfg->hack_mpm_event = 1; - return NULL; - } - else if (!strcasecmp(value, "Off")) { - cfg->hack_mpm_event = 0; - return NULL; - } - - (void)arg; - return "value must be On or Off"; -} - static const char *h2_conf_set_direct(cmd_parms *parms, void *arg, const char *value) { @@ -423,16 +379,12 @@ const command_rec h2_cmds[] = { RSRC_CONF, "set the maximum age (in seconds) that client can rely on alt-svc information"), AP_INIT_TAKE1("H2SerializeHeaders", h2_conf_set_serialize_headers, NULL, RSRC_CONF, "on to enable header serialization for compatibility"), - AP_INIT_TAKE1("H2HackMpmEvent", h2_conf_set_hack_mpm_event, NULL, - RSRC_CONF, "on to enable a hack that makes mpm_event working with mod_h2"), AP_INIT_TAKE1("H2Direct", h2_conf_set_direct, NULL, RSRC_CONF, "on to enable direct HTTP/2 mode"), AP_INIT_TAKE1("H2BufferOutput", h2_conf_set_buffer_output, NULL, RSRC_CONF, "on to enable output buffering, default for TLS"), AP_INIT_TAKE1("H2BufferSize", h2_conf_set_buffer_size, NULL, RSRC_CONF, "size of outgoing buffer in bytes"), - AP_INIT_TAKE1("H2BufferWriteMax", h2_conf_set_write_max, NULL, - RSRC_CONF, "maximum number of bytes in a outgoing write"), AP_INIT_TAKE1("H2SessionExtraFiles", h2_conf_set_session_extra_files, NULL, RSRC_CONF, "number of extra file a session might keep open"), { NULL, NULL, NULL, 0, 0, NULL } diff --git a/modules/http2/h2_config.h b/modules/http2/h2_config.h index 2de6f876ca..9bd7ea95d6 100644 --- a/modules/http2/h2_config.h +++ b/modules/http2/h2_config.h @@ -33,11 +33,9 @@ typedef enum { H2_CONF_ALT_SVCS, H2_CONF_ALT_SVC_MAX_AGE, H2_CONF_SER_HEADERS, - H2_CONF_HACK_MPM_EVENT, H2_CONF_DIRECT, H2_CONF_BUFFER_OUTPUT, H2_CONF_BUFFER_SIZE, - H2_CONF_WRITE_MAX, H2_CONF_SESSION_FILES, } h2_config_var_t; @@ -55,12 +53,9 @@ typedef struct h2_config { int alt_svc_max_age; /* seconds clients can rely on alt-svc info*/ int serialize_headers; /* Use serialized HTTP/1.1 headers for processing, better compatibility */ - int hack_mpm_event; /* If mpm_event is detected, perform a hack - on stream connections to make it work */ int h2_direct; /* if mod_h2 is active directly */ int buffer_output; /* if output buffering shall be used */ int buffer_size; /* size of buffer for outgoing data */ - int write_max; /* max number of bytes for a write op */ int session_extra_files; /* # of extra files a session may keep open */ } h2_config; diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c index 5d5e717080..a37276238e 100644 --- a/modules/http2/h2_conn.c +++ b/modules/http2/h2_conn.c @@ -412,9 +412,7 @@ apr_status_t h2_conn_prep(h2_task_env *env, conn_rec *master, h2_worker *worker) /* all fine */ break; case H2_MPM_EVENT: - if (h2_config_geti(cfg, H2_CONF_HACK_MPM_EVENT)) { - fix_event_conn(&env->c, master); - } + fix_event_conn(&env->c, master); break; default: /* fingers crossed */ @@ -471,9 +469,7 @@ apr_status_t h2_conn_init(struct h2_task_env *env, struct h2_worker *worker) /* all fine */ break; case H2_MPM_EVENT: - if (h2_config_geti(cfg, H2_CONF_HACK_MPM_EVENT)) { - fix_event_conn(&env->c, master); - } + fix_event_conn(&env->c, master); break; default: /* fingers crossed */ diff --git a/modules/http2/h2_conn_io.c b/modules/http2/h2_conn_io.c index f876a1f95a..749965e1fb 100644 --- a/modules/http2/h2_conn_io.c +++ b/modules/http2/h2_conn_io.c @@ -28,6 +28,11 @@ #include "h2_h2.h" #include "h2_util.h" +#define WRITE_SIZE_INITIAL 1300 +#define WRITE_SIZE_MAX (16*1024) +#define WRITE_SIZE_IDLE_USEC (1*APR_USEC_PER_SEC) +#define WRITE_SIZE_THRESHOLD (1*1024*1024) + apr_status_t h2_conn_io_init(h2_conn_io *io, conn_rec *c) { h2_config *cfg = h2_config_get(c); @@ -36,7 +41,10 @@ apr_status_t h2_conn_io_init(h2_conn_io *io, conn_rec *c) io->input = apr_brigade_create(c->pool, c->bucket_alloc); io->output = apr_brigade_create(c->pool, c->bucket_alloc); io->buflen = 0; - io->max_write_size = h2_config_geti(cfg, H2_CONF_WRITE_MAX); + /* That is where we start with, + * see https://issues.apache.org/jira/browse/TS-2503 */ + io->write_size = WRITE_SIZE_INITIAL; + io->last_write = 0; io->buffer_output = h2_config_geti(cfg, H2_CONF_BUFFER_OUTPUT); if (io->buffer_output < 0) { @@ -175,27 +183,52 @@ static apr_status_t flush_out(apr_bucket_brigade *bb, void *ctx) { h2_conn_io *io = (h2_conn_io*)ctx; apr_status_t status; + apr_off_t bblen; ap_update_child_status(io->connection->sbh, SERVER_BUSY_WRITE, NULL); - - status = ap_pass_brigade(io->connection->output_filters, bb); - apr_brigade_cleanup(bb); + status = apr_brigade_length(bb, 1, &bblen); + if (status == APR_SUCCESS) { + status = ap_pass_brigade(io->connection->output_filters, bb); + if (status == APR_SUCCESS) { + io->bytes_written += (apr_size_t)bblen; + io->last_write = apr_time_now(); + } + apr_brigade_cleanup(bb); + } return status; } static apr_status_t bucketeer_buffer(h2_conn_io *io) { const char *data = io->buffer; apr_size_t remaining = io->buflen; - int bcount = (int)(remaining / io->max_write_size); apr_bucket *b; - int i; + int bcount, i; + + if (io->write_size > WRITE_SIZE_INITIAL + && (apr_time_now() - io->last_write) >= WRITE_SIZE_IDLE_USEC) { + /* long time not written, reset write size */ + io->write_size = WRITE_SIZE_INITIAL; + io->bytes_written = 0; + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, io->connection, + "h2_conn_io(%ld): timeout write size reset to %ld", + (long)io->connection->id, (long)io->write_size); + } + else if (io->write_size < WRITE_SIZE_MAX + && io->bytes_written >= WRITE_SIZE_THRESHOLD) { + /* connection is hot, use max size */ + io->write_size = WRITE_SIZE_MAX; + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, io->connection, + "h2_conn_io(%ld): threshold reached, write size now %ld", + (long)io->connection->id, (long)io->write_size); + } + bcount = (int)(remaining / io->write_size); for (i = 0; i < bcount; ++i) { - b = apr_bucket_transient_create(data, io->max_write_size, + b = apr_bucket_transient_create(data, io->write_size, io->output->bucket_alloc); APR_BRIGADE_INSERT_TAIL(io->output, b); - data += io->max_write_size; - remaining -= io->max_write_size; + data += io->write_size; + remaining -= io->write_size; } if (remaining > 0) { @@ -261,12 +294,9 @@ apr_status_t h2_conn_io_flush(h2_conn_io *io) if (io->unflushed) { apr_status_t status; if (io->buflen > 0) { - apr_bucket *b; ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, io->connection, "h2_conn_io: flush, flushing %ld bytes", (long)io->buflen); - b = apr_bucket_transient_create(io->buffer, io->buflen, - io->output->bucket_alloc); - APR_BRIGADE_INSERT_TAIL(io->output, b); + bucketeer_buffer(io); io->buflen = 0; } /* Append flush. diff --git a/modules/http2/h2_conn_io.h b/modules/http2/h2_conn_io.h index 9d1db6c9b4..084445ef2b 100644 --- a/modules/http2/h2_conn_io.h +++ b/modules/http2/h2_conn_io.h @@ -27,7 +27,9 @@ typedef struct { apr_bucket_brigade *input; apr_bucket_brigade *output; int buffer_output; - int max_write_size; + int write_size; + apr_time_t last_write; + apr_size_t bytes_written; char *buffer; apr_size_t buflen; -- 2.40.0