From 3b156a19e8e7346404ed88a46cd9cb6acd046b55 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Tue, 19 Feb 2019 11:51:27 +0000 Subject: [PATCH] mod_cache_socache: avoid pool to heap reallocation. Below some threshold, the previous code tried free (sub-)pooled memory ASAP by moving small buffers (< capacity / 2) to a heap bucket. But this is not really an optimization because first it requires at some point to allocate more than the configured capacity, and second since this happens during response handling the pool is about to be destroyed soon anymay. This commit simply keeps the data in the subpool and uses a pool bucket for the output brigade to take care of the lifetime until it's consumed (or not). git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1853874 13f79535-47bb-0310-9956-ffa450edef68 --- modules/cache/mod_cache_socache.c | 57 ++++++++++++++----------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/modules/cache/mod_cache_socache.c b/modules/cache/mod_cache_socache.c index 4920dc232c..d5bba99d3e 100644 --- a/modules/cache/mod_cache_socache.c +++ b/modules/cache/mod_cache_socache.c @@ -429,6 +429,14 @@ static int create_entity(cache_handle_t *h, request_rec *r, const char *key, return OK; } +static apr_status_t sobj_body_pre_cleanup(void *baton) +{ + cache_socache_object_t *sobj = baton; + apr_brigade_cleanup(sobj->body); + sobj->body = NULL; + return APR_SUCCESS; +} + static int open_entity(cache_handle_t *h, request_rec *r, const char *key) { cache_socache_dir_conf *dconf = @@ -648,36 +656,25 @@ static int open_entity(cache_handle_t *h, request_rec *r, const char *key) } /* Retrieve the body if we have one */ - sobj->body = apr_brigade_create(r->pool, r->connection->bucket_alloc); len = buffer_len - slider; - - /* - * Optimisation: if the body is small, we want to make a - * copy of the body and free the temporary pool, as we - * don't want large blocks of unused memory hanging around - * to the end of the response. In contrast, if the body is - * large, we would rather leave the body where it is in the - * temporary pool, and save ourselves the copy. - */ - if (len * 2 > dconf->max) { + if (len > 0) { apr_bucket *e; - - /* large - use the brigade as is, we're done */ - e = apr_bucket_immortal_create((const char *) sobj->buffer + slider, - len, r->connection->bucket_alloc); - + /* Create the body brigade later concatenated to the output filters' + * brigade by recall_body(). Since sobj->buffer (the data) point to + * sobj->pool (a subpool of r->pool), be safe by using a pool bucket + * which can morph to heap if sobj->pool is destroyed while the bucket + * is still alive. But if sobj->pool gets destroyed while the bucket is + * still in sobj->body (i.e. recall_body() was never called), we don't + * need to morph to something just about to be freed, so a pre_cleanup + * will take care of cleaning up sobj->body before this happens (and is + * a noop otherwise). + */ + sobj->body = apr_brigade_create(sobj->pool, r->connection->bucket_alloc); + apr_pool_pre_cleanup_register(sobj->pool, sobj, sobj_body_pre_cleanup); + e = apr_bucket_pool_create((const char *) sobj->buffer + slider, len, + sobj->pool, r->connection->bucket_alloc); APR_BRIGADE_INSERT_TAIL(sobj->body, e); } - else { - - /* small - make a copy of the data... */ - apr_brigade_write(sobj->body, NULL, NULL, (const char *) sobj->buffer - + slider, len); - - /* ...and get rid of the large memory buffer */ - apr_pool_destroy(sobj->pool); - sobj->pool = NULL; - } /* make the configuration stick */ h->cache_obj = obj; @@ -766,13 +763,9 @@ static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb) { cache_socache_object_t *sobj = (cache_socache_object_t*) h->cache_obj->vobj; - apr_bucket *e; - e = APR_BRIGADE_FIRST(sobj->body); - - if (e != APR_BRIGADE_SENTINEL(sobj->body)) { - APR_BUCKET_REMOVE(e); - APR_BRIGADE_INSERT_TAIL(bb, e); + if (sobj->body) { + APR_BRIGADE_CONCAT(bb, sobj->body); } return APR_SUCCESS; -- 2.40.0