mod_http2: earlier slave connection allocator destroy, code cleanups

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1767128 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 31b0fac848cd31ec962d4ea2a0f874144c2a9c85..12b5f46f77277a8320f422fd992d4c68880db6ec 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_http2: allocators from slave connections is released earlier, resulting
+     in less overall memory use on busy, long lived connections.
+     [Stefan Eissing]
+     
   *) mpm_unix: Apache fails to start if previously crashed then restarted with
      the same PID (e.g. in container).  PR 60261.
      [Val <valentin.bremond gmail.com>, Yann Ylavic]

diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c
index a0915c3eb44ded54d3d6dcf4035543c25daa91e3..6f3a8cfe8ec63215abdf2edf71a98f418afca0bc 100644 (file)
--- a/modules/http2/h2_conn.c
+++ b/modules/http2/h2_conn.c
@@ -241,9 +241,9 @@ apr_status_t h2_conn_pre_close(struct h2_ctx *ctx, conn_rec *c)
     return status;
 }
 
-conn_rec *h2_slave_create(conn_rec *master, int slave_id, 
-                          apr_pool_t *parent, apr_allocator_t *allocator)
+conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent)
 {
+    apr_allocator_t *allocator;
     apr_pool_t *pool;
     conn_rec *c;
     void *cfg;
@@ -257,9 +257,7 @@ conn_rec *h2_slave_create(conn_rec *master, int slave_id,
      * independant of its parent pool in the sense that it can work in
      * another thread.
      */
-    if (!allocator) {
-        apr_allocator_create(&allocator);
-    }
+    apr_allocator_create(&allocator);
     apr_pool_create_ex(&pool, parent, NULL, allocator);
     apr_pool_tag(pool, "h2_slave_conn");
     apr_allocator_owner_set(allocator, pool);
@@ -311,21 +309,11 @@ conn_rec *h2_slave_create(conn_rec *master, int slave_id,
     return c;
 }
 
-void h2_slave_destroy(conn_rec *slave, apr_allocator_t **pallocator)
+void h2_slave_destroy(conn_rec *slave)
 {
-    apr_pool_t *parent;
-    apr_allocator_t *allocator = apr_pool_allocator_get(slave->pool);
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, slave,
                   "h2_slave_conn(%ld): destroy (task=%s)", slave->id,
                   apr_table_get(slave->notes, H2_TASK_ID_NOTE));
-    /* Attache the allocator to the parent pool and return it for
-     * reuse, otherwise the own is still the slave pool and it will
-     * get destroyed with it. */
-    parent = apr_pool_parent_get(slave->pool);
-    if (pallocator && parent) {
-        apr_allocator_owner_set(allocator, parent);
-        *pallocator = allocator;
-    }
     apr_pool_destroy(slave->pool);
 }
 

diff --git a/modules/http2/h2_conn.h b/modules/http2/h2_conn.h
index 13b20539b1a9f5de2390e9360dc412584f7b2462..79948644ae8c1738d93622cf4d40017fb6335590 100644 (file)
--- a/modules/http2/h2_conn.h
+++ b/modules/http2/h2_conn.h
@@ -66,9 +66,8 @@ typedef enum {
 h2_mpm_type_t h2_conn_mpm_type(void);
 
 
-conn_rec *h2_slave_create(conn_rec *master, int slave_id, 
-                          apr_pool_t *parent, apr_allocator_t *allocator);
-void h2_slave_destroy(conn_rec *slave, apr_allocator_t **pallocator);
+conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent);
+void h2_slave_destroy(conn_rec *slave);
 
 apr_status_t h2_slave_run_pre_connection(conn_rec *slave, apr_socket_t *csd);
 void h2_slave_run_connection(conn_rec *slave);

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index 0cff7b60fa1613153be833273d20c92bd82271de..72f11106edf3114950667cd673d2002bc1f1b823 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -226,11 +226,17 @@ static void purge_streams(h2_mplx *m)
 
 static void h2_mplx_destroy(h2_mplx *m)
 {
+    conn_rec **pslave;
     ap_assert(m);
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
                   "h2_mplx(%ld): destroy, tasks=%d", 
                   m->id, (int)h2_ihash_count(m->tasks));
     check_tx_free(m);
+    
+    while (m->spare_slaves->nelts > 0) {
+        pslave = (conn_rec **)apr_array_pop(m->spare_slaves);
+        h2_slave_destroy(*pslave);
+    }
     if (m->pool) {
         apr_pool_destroy(m->pool);
     }
@@ -297,6 +303,7 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr_pool_t *parent,
         m->q = h2_iq_create(m->pool, m->max_streams);
         m->sready = h2_ihash_create(m->pool, offsetof(h2_stream,id));
         m->tasks = h2_ihash_create(m->pool, offsetof(h2_task,stream_id));
+        m->redo_tasks = h2_ihash_create(m->pool, offsetof(h2_task, stream_id));
 
         m->stream_timeout = stream_timeout;
         m->workers = workers;
@@ -363,24 +370,20 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master)
         }
     }
     
-    if (task->output.beam) {
-        h2_beam_on_produced(task->output.beam, NULL, NULL);
-        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, 
-                      APLOGNO(03385) "h2_task(%s): destroy "
-                      "output beam empty=%d, holds proxies=%d", 
-                      task->id,
-                      h2_beam_empty(task->output.beam),
-                      h2_beam_holds_proxies(task->output.beam));
-    }
+    h2_beam_on_produced(task->output.beam, NULL, NULL);
+    ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, 
+                  APLOGNO(03385) "h2_task(%s): destroy "
+                  "output beam empty=%d, holds proxies=%d", 
+                  task->id,
+                  h2_beam_empty(task->output.beam),
+                  h2_beam_holds_proxies(task->output.beam));
     
     slave = task->c;
     reuse_slave = ((m->spare_slaves->nelts < m->spare_slaves->nalloc)
                    && !task->rst_error);
     
     h2_ihash_remove(m->tasks, task->stream_id);
-    if (m->redo_tasks) {
-        h2_ihash_remove(m->redo_tasks, task->stream_id);
-    }
+    h2_ihash_remove(m->redo_tasks, task->stream_id);
     h2_task_destroy(task);
 
     if (slave) {
@@ -389,7 +392,7 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master)
         }
         else {
             slave->sbh = NULL;
-            h2_slave_destroy(slave, NULL);
+            h2_slave_destroy(slave);
         }
     }
     
@@ -434,18 +437,15 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error)
     h2_iq_remove(m->q, stream->id);
     h2_ihash_remove(m->sready, stream->id);
     h2_ihash_remove(m->streams, stream->id);
-    if (stream->input) {
-        m->tx_handles_reserved += h2_beam_get_files_beamed(stream->input);
-        h2_beam_on_consumed(stream->input, NULL, NULL);
-        /* Let anyone blocked reading know that there is no more to come */
-        h2_beam_abort(stream->input);
-        /* Remove mutex after, so that abort still finds cond to signal */
-        h2_beam_mutex_set(stream->input, NULL, NULL, NULL);
-    }
-    if (stream->output) {
-        m->tx_handles_reserved += h2_beam_get_files_beamed(stream->output);
-    }
+    
     h2_stream_cleanup(stream);
+    m->tx_handles_reserved += h2_beam_get_files_beamed(stream->input);
+    h2_beam_on_consumed(stream->input, NULL, NULL);
+    /* Let anyone blocked reading know that there is no more to come */
+    h2_beam_abort(stream->input);
+    /* Remove mutex after, so that abort still finds cond to signal */
+    h2_beam_mutex_set(stream->input, NULL, NULL, NULL);
+    m->tx_handles_reserved += h2_beam_get_files_beamed(stream->output);
 
     task = h2_ihash_get(m->tasks, stream->id);
     if (task) {
@@ -459,7 +459,7 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error)
         }
         else {
             /* already finished */
-            task_destroy(m, task, 0);
+            task_destroy(m, task, 1);
         }
     }
     h2_stream_destroy(stream);
@@ -532,12 +532,8 @@ static int task_abort_connection(void *ctx, void *val)
         if (task->c) {
             task->c->aborted = 1;
         }
-        if (task->input.beam) {
-            h2_beam_abort(task->input.beam);
-        }
-        if (task->output.beam) {
-            h2_beam_abort(task->output.beam);
-        }
+        h2_beam_abort(task->input.beam);
+        h2_beam_abort(task->output.beam);
     }
     return 1;
 }
@@ -772,11 +768,9 @@ static apr_status_t out_close(h2_mplx *m, h2_task *task)
 
     ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, m->c,
                   "h2_mplx(%s): close", task->id);
-    if (task->output.beam) {
-        status = h2_beam_close(task->output.beam);
-        h2_beam_log(task->output.beam, task->stream_id, "out_close", m->c, 
-                    APLOG_TRACE2);
-    }
+    status = h2_beam_close(task->output.beam);
+    h2_beam_log(task->output.beam, task->stream_id, "out_close", m->c, 
+                APLOG_TRACE2);
     output_consumed_signal(m, task);
     have_out_data_for(m, stream, 0);
     return status;
@@ -897,7 +891,7 @@ static h2_task *next_stream_task(h2_mplx *m)
                 slave = *pslave;
             }
             else {
-                slave = h2_slave_create(m->c, stream->id, m->pool, NULL);
+                slave = h2_slave_create(m->c, stream->id, m->pool);
                 new_conn = 1;
             }
             
@@ -919,16 +913,13 @@ static h2_task *next_stream_task(h2_mplx *m)
                 m->max_stream_started = sid;
             }
 
-            if (stream->input) {
-                h2_beam_timeout_set(stream->input, m->stream_timeout);
-                h2_beam_on_consumed(stream->input, stream_input_consumed, m);
-                h2_beam_on_file_beam(stream->input, can_beam_file, m);
-                h2_beam_mutex_set(stream->input, beam_enter, task->cond, m);
-            }
-            if (stream->output) {
-                h2_beam_buffer_size_set(stream->output, m->stream_max_mem);
-                h2_beam_timeout_set(stream->output, m->stream_timeout);
-            }
+            h2_beam_timeout_set(stream->input, m->stream_timeout);
+            h2_beam_on_consumed(stream->input, stream_input_consumed, m);
+            h2_beam_on_file_beam(stream->input, can_beam_file, m);
+            h2_beam_mutex_set(stream->input, beam_enter, task->cond, m);
+            
+            h2_beam_buffer_size_set(stream->output, m->stream_max_mem);
+            h2_beam_timeout_set(stream->output, m->stream_timeout);
             ++m->workers_busy;
         }
     }
@@ -977,10 +968,8 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn)
         
         if (ngn) {
             apr_off_t bytes = 0;
-            if (task->output.beam) {
-                h2_beam_send(task->output.beam, NULL, APR_NONBLOCK_READ);
-                bytes += h2_beam_get_buffered(task->output.beam);
-            }
+            h2_beam_send(task->output.beam, NULL, APR_NONBLOCK_READ);
+            bytes += h2_beam_get_buffered(task->output.beam);
             if (bytes > 0) {
                 /* we need to report consumed and current buffered output
                  * to the engine. The request will be streamed out or cancelled,
@@ -1001,7 +990,7 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn)
         }
         
         stream = h2_ihash_get(m->streams, task->stream_id);
-        if (!m->aborted && stream && m->redo_tasks
+        if (!m->aborted && stream 
             && h2_ihash_get(m->redo_tasks, task->stream_id)) {
             /* reset and schedule again */
             h2_task_redo(task);
@@ -1152,9 +1141,6 @@ static apr_status_t unschedule_slow_tasks(h2_mplx *m)
     h2_task *task;
     int n;
     
-    if (!m->redo_tasks) {
-        m->redo_tasks = h2_ihash_create(m->pool, offsetof(h2_task, stream_id));
-    }
     /* Try to get rid of streams that occupy workers. Look for safe requests
      * that are repeatable. If none found, fail the connection.
      */

diff --git a/modules/http2/h2_ngn_shed.c b/modules/http2/h2_ngn_shed.c
index 2f5b7296177214f3129ec7441a457b6331144e34..45329102f77903997b81df69955485af28f25ddd 100644 (file)
--- a/modules/http2/h2_ngn_shed.c
+++ b/modules/http2/h2_ngn_shed.c
@@ -72,7 +72,7 @@ struct h2_req_engine {
     const char *type;      /* name of the engine type */
     apr_pool_t *pool;      /* pool for engine specific allocations */
     conn_rec *c;           /* connection this engine is assigned to */
-    h2_task *task;         /* the task this engine is base on, running in */
+    h2_task *task;         /* the task this engine is based on, running in */
     h2_ngn_shed *shed;
 
     unsigned int shutdown : 1; /* engine is being shut down */

diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index 27ed9197b51c30378092e816ea865bd2919fd78e..f743f4814ccae4c11ccf3cfbebfad802ce49d6c6 100644 (file)
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -82,7 +82,6 @@ apr_status_t h2_session_stream_done(h2_session *session, h2_stream *stream)
     ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c,
                   "h2_stream(%ld-%d): EOS bucket cleanup -> done", 
                   session->id, stream->id);
-    h2_ihash_remove(session->streams, stream->id);
     h2_mplx_stream_done(session->mplx, stream);
     
     dispatch_event(session, H2_SESSION_EV_STREAM_DONE, 0, NULL);
@@ -94,10 +93,9 @@ typedef struct stream_sel_ctx {
     h2_stream *candidate;
 } stream_sel_ctx;
 
-static int find_cleanup_stream(void *udata, void *sdata)
+static int find_cleanup_stream(h2_stream *stream, void *ictx)
 {
-    stream_sel_ctx *ctx = udata;
-    h2_stream *stream = sdata;
+    stream_sel_ctx *ctx = ictx;
     if (H2_STREAM_CLIENT_INITIATED(stream->id)) {
         if (!ctx->session->local.accepting
             && stream->id > ctx->session->local.accepted_max) {
@@ -121,7 +119,7 @@ static void cleanup_streams(h2_session *session)
     ctx.session = session;
     ctx.candidate = NULL;
     while (1) {
-        h2_ihash_iter(session->streams, find_cleanup_stream, &ctx);
+        h2_mplx_stream_do(session->mplx, find_cleanup_stream, &ctx);
         if (ctx.candidate) {
             h2_session_stream_done(session, ctx.candidate);
             ctx.candidate = NULL;
@@ -144,7 +142,6 @@ h2_stream *h2_session_open_stream(h2_session *session, int stream_id,
     stream = h2_stream_open(stream_id, stream_pool, session, 
                             initiated_on);
     nghttp2_session_set_stream_user_data(session->ngh2, stream_id, stream);
-    h2_ihash_add(session->streams, stream);
     
     if (req) {
         h2_stream_set_request(stream, req);
@@ -713,7 +710,6 @@ static void h2_session_destroy(h2_session *session)
 {
     ap_assert(session);    
 
-    h2_ihash_clear(session->streams);
     if (session->mplx) {
         h2_mplx_set_consumed_cb(session->mplx, NULL, NULL);
         h2_mplx_release_and_join(session->mplx, session->iowait);
@@ -927,8 +923,6 @@ static h2_session *h2_session_create_int(conn_rec *c,
             return NULL;
         }
         
-        session->streams = h2_ihash_create(session->pool,
-                                           offsetof(h2_stream, id));
         session->mplx = h2_mplx_create(c, session->pool, session->config, 
                                        session->s->timeout, workers);
         

diff --git a/modules/http2/h2_session.h b/modules/http2/h2_session.h
index 4d21cb86f5d22844b1e14e0086fc3a8e4213d76e..69b7a59c44a9583325be09cdcb08d2986ca9db7c 100644 (file)
--- a/modules/http2/h2_session.h
+++ b/modules/http2/h2_session.h
@@ -86,7 +86,6 @@ typedef struct h2_session {
     struct h2_workers *workers;     /* for executing stream tasks */
     struct h2_filter_cin *cin;      /* connection input filter context */
     h2_conn_io io;                  /* io on httpd conn filters */
-    struct h2_ihash_t *streams;     /* streams handled by this session */
     struct nghttp2_session *ngh2;   /* the nghttp2 session (internal use) */
 
     h2_session_state state;         /* state session is in */

diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c
index 3f70b3aa21d5665d932bbb12fd6a9e0fe02761de..e5e4efc6a0d951f0e223ac5d52fad035b2080814 100644 (file)
--- a/modules/http2/h2_task.c
+++ b/modules/http2/h2_task.c
@@ -403,7 +403,7 @@ static apr_status_t h2_filter_parse_h1(ap_filter_t* f, apr_bucket_brigade* bb)
  ******************************************************************************/
  
 int h2_task_can_redo(h2_task *task) {
-    if (task->input.beam && h2_beam_was_received(task->input.beam)) {
+    if (h2_beam_was_received(task->input.beam)) {
         /* cannot repeat that. */
         return 0;
     }
@@ -420,10 +420,8 @@ void h2_task_redo(h2_task *task)
 void h2_task_rst(h2_task *task, int error)
 {
     task->rst_error = error;
-    if (task->input.beam) {
-        h2_beam_abort(task->input.beam);
-    }
-    if (!task->worker_done && task->output.beam) {
+    h2_beam_abort(task->input.beam);
+    if (!task->worker_done) {
         h2_beam_abort(task->output.beam);
     }
     if (task->c) {