You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2002/05/22 17:15:42 UTC
[PATCH] Pools space-time tradeoff
[crossposted to APR and SVN dev lists.]
Hi, all. I've been working with Sander to resolve some pool problems.
** Here's the problem:
We have two use-cases in Subversion now that literally run out of
memory; the application footprint grows without bound until the OS
kills things. Here they are:
1. 'svn checkout file:///repository -r HEAD'
2. 'svnadmin dump repository > dumpfile'
These use-cases, it turns out, are nearly identical. They both simply
walk over a repository tree and push data at a vtable, which in turn
writes the data to disk (either as a working copy, or as a dumpfile).
The pools are controlled by our filesystem-walker, and we're being
*extremely* careful about controlling pool lifetimes and clearing subpools
in loops. (Gstein has pounded these concepts into our heads. :-))
[Note: 'svn checkout http://...' works fine, because svn is doing an
independent GET request for each file... which means a separate
pool created and destroyed by httpd for each item.]
It turns out that the real issue here is that our current pool
implementation always grows forever, until the entire pool hierarchy
is destroyed when the main() process exits. I believe that when Mike
Pilato stat'ed our pools, we were actually only using a few
megs... but we were still creating a memory footprint of 200M+ !
** Here's the solution:
Sander whipped out two of his pool patches that have been previously
ignored up till now. He combined them into a single patch. Here's
what they do:
1. Keep each pool's list of 'old' active blocks sorted by size. If
a request comes in that won't fit in the current active block,
then try to alloc the request in the largest 'old' active block
(instead of alloc'ing a completely new active block).
Effectively, we get some memory re-use now.
2. When a pool is destroyed or cleared, normally all of its
inactive blocks are given back to a 'global free list' in the
pool hierarchy. We now have a threshold size that can be set:
if the global-free-list ever grows larger than the threshold,
then the 'extra' inactive blocks are actually free()d. We've
set the maximum free-list size at 4M for now.
Anyway, I've included the patches below for people to look at.
Apparently Sander posted these before, but nobody had a reason to
care. Well, now Subversion *really* needs these features.
I've tested this patch on our two use cases: the memory footprint for
'svn checkout' bounces up and down, but eventually plateaus around
25M. The footprint for 'svnadmin dump' also bounces around, but
plateaus around 17M. A huge difference!
Sander is worried about possible pool-performance hits from these
changes, so he's hoping folks might be able to do some timing stats.
(Personally, via my sophisticated use of the unix 'time' command, I
find no noticeable difference.)
The patch affects two APR files (apr_allocator.h, apr_pools.c), and
one svn file (svn_error.c)... assuming svn users want to take
advantage of the new pool features. I've posted the patch below.
Assuming nobody objects, Sander plans to apply these changes in a day
or two. We just want to give people time to look them over.
Index: include/apr_allocator.h
===================================================================
RCS file: /home/cvs/apr/include/apr_allocator.h,v
retrieving revision 1.4
diff -u -r1.4 apr_allocator.h
--- include/apr_allocator.h 9 Apr 2002 06:56:55 -0000 1.4
+++ include/apr_allocator.h 22 May 2002 16:11:50 -0000
@@ -83,7 +83,9 @@
struct apr_memnode_t {
apr_memnode_t *next;
+ apr_memnode_t **ref;
apr_uint32_t index;
+ apr_uint32_t free_index;
char *first_avail;
char *endp;
};
@@ -150,6 +152,16 @@
* @param allocator The allocator to get the owner from
*/
APR_DECLARE(apr_pool_t *) apr_allocator_get_owner(apr_allocator_t *allocator);
+
+
+/**
+ * Set the current threshold at which the allocator should start
+ * giving blocks back to the system.
+ * @param allocator The allocator the set the threshold on
+ * @param size The threshold. 0 == unlimited.
+ */
+APR_DECLARE(void) apr_allocator_set_max_free(apr_allocator_t *allocator,
+ apr_size_t size);
#include "apr_thread_mutex.h"
Index: memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v
retrieving revision 1.167
diff -u -r1.167 apr_pools.c
--- memory/unix/apr_pools.c 5 May 2002 22:24:41 -0000 1.167
+++ memory/unix/apr_pools.c 22 May 2002 16:11:55 -0000
@@ -93,6 +93,8 @@
struct apr_allocator_t {
apr_uint32_t max_index;
+ apr_uint32_t max_free_index;
+ apr_uint32_t current_free_index;
#if APR_HAS_THREADS
apr_thread_mutex_t *mutex;
#endif /* APR_HAS_THREADS */
@@ -166,6 +168,32 @@
return allocator->owner;
}
+APR_DECLARE(void) apr_allocator_set_max_free(apr_allocator_t *allocator,
+ apr_size_t size)
+{
+ apr_uint32_t max_free_index;
+
+#if APR_HAS_THREADS
+ apr_thread_mutex_t *mutex;
+
+ mutex = apr_allocator_get_mutex(allocator);
+ if (mutex != NULL)
+ apr_thread_mutex_lock(mutex);
+#endif /* APR_HAS_THREADS */
+
+ max_free_index = APR_ALIGN(size, BOUNDARY_SIZE) >> BOUNDARY_INDEX;
+ allocator->current_free_index += max_free_index;
+ allocator->current_free_index -= allocator->max_free_index;
+ allocator->max_free_index = max_free_index;
+ if (allocator->current_free_index > max_free_index)
+ allocator->current_free_index = max_free_index;
+
+#if APR_HAS_THREADS
+ if (mutex != NULL)
+ apr_thread_mutex_unlock(mutex);
+#endif
+}
+
APR_INLINE
APR_DECLARE(apr_memnode_t *) apr_allocator_alloc(apr_allocator_t *allocator,
apr_size_t size)
@@ -228,6 +256,10 @@
allocator->max_index = max_index;
}
+ allocator->current_free_index += node->index;
+ if (allocator->current_free_index > allocator->max_free_index)
+ allocator->current_free_index = allocator->max_free_index;
+
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_unlock(allocator->mutex);
@@ -264,6 +296,10 @@
if (node) {
*ref = node->next;
+ allocator->current_free_index += node->index;
+ if (allocator->current_free_index > allocator->max_free_index)
+ allocator->current_free_index = allocator->max_free_index;
+
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_unlock(allocator->mutex);
@@ -299,8 +335,9 @@
APR_DECLARE(void) apr_allocator_free(apr_allocator_t *allocator,
apr_memnode_t *node)
{
- apr_memnode_t *next;
+ apr_memnode_t *next, *freelist = NULL;
apr_uint32_t index, max_index;
+ apr_uint32_t max_free_index, current_free_index;
#if APR_HAS_THREADS
if (allocator->mutex)
@@ -308,6 +345,8 @@
#endif /* APR_HAS_THREADS */
max_index = allocator->max_index;
+ max_free_index = allocator->max_free_index;
+ current_free_index = allocator->current_free_index;
/* Walk the list of submitted nodes and free them one by one,
* shoving them in the right 'size' buckets as we go.
@@ -316,7 +355,11 @@
next = node->next;
index = node->index;
- if (index < MAX_INDEX) {
+ if (max_free_index != 0 && index > current_free_index) {
+ node->next = freelist;
+ freelist = node;
+ }
+ else if (index < MAX_INDEX) {
/* Add the node to the appropiate 'size' bucket. Adjust
* the max_index when appropiate.
*/
@@ -325,6 +368,7 @@
max_index = index;
}
allocator->free[index] = node;
+ current_free_index -= index;
}
else {
/* This node is too large to keep in a specific size bucket,
@@ -332,15 +376,23 @@
*/
node->next = allocator->free[0];
allocator->free[0] = node;
+ current_free_index -= index;
}
} while ((node = next) != NULL);
allocator->max_index = max_index;
+ allocator->current_free_index = current_free_index;
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_unlock(allocator->mutex);
#endif /* APR_HAS_THREADS */
+
+ while (freelist != NULL) {
+ node = freelist;
+ freelist = node->next;
+ free(node);
+ }
}
@@ -544,6 +596,7 @@
apr_memnode_t *active, *node;
void *mem;
char *endp;
+ apr_uint32_t free_index;
size = APR_ALIGN_DEFAULT(size);
active = pool->active;
@@ -557,17 +610,52 @@
return mem;
}
- if ((node = apr_allocator_alloc(pool->allocator, size)) == NULL) {
- if (pool->abort_fn)
- pool->abort_fn(APR_ENOMEM);
-
- return NULL;
+ node = active->next;
+ endp = node->first_avail + size;
+ if (endp < node->endp) {
+ *node->ref = node->next;
+ node->next->ref = node->ref;
+ }
+ else {
+ if ((node = apr_allocator_alloc(pool->allocator, size)) == NULL) {
+ if (pool->abort_fn)
+ pool->abort_fn(APR_ENOMEM);
+ }
+ endp = node->first_avail + size;
}
- active->next = pool->active = node;
+ node->free_index = 0;
mem = node->first_avail;
- node->first_avail += size;
+ node->first_avail = endp;
+
+ node->ref = active->ref;
+ *node->ref = node;
+ node->next = active;
+ active->ref = &node->next;
+
+ pool->active = node;
+
+ free_index = (APR_ALIGN(active->endp - active->first_avail + 1,
+ BOUNDARY_SIZE) - BOUNDARY_SIZE) >> BOUNDARY_INDEX;
+
+ active->free_index = free_index;
+ node = active->next;
+ if (free_index >= node->free_index)
+ return mem;
+
+ do {
+ node = node->next;
+ }
+ while (free_index < node->free_index);
+
+ *active->ref = active->next;
+ active->next->ref = active->ref;
+
+ active->ref = node->ref;
+ *active->ref = active;
+ active->next = node;
+ node->ref = &active->next;
return mem;
}
@@ -625,11 +713,13 @@
active = pool->active = pool->self;
active->first_avail = pool->self_first_avail;
- if (active->next == NULL)
+ if (active->next == active)
return;
+ *active->ref = NULL;
apr_allocator_free(pool->allocator, active->next);
- active->next = NULL;
+ active->next = active;
+ active->ref = &active->next;
}
APR_DECLARE(void) apr_pool_destroy(apr_pool_t *pool)
@@ -672,6 +762,7 @@
*/
allocator = pool->allocator;
active = pool->self;
+ *active->ref = NULL;
#if APR_HAS_THREADS
if (apr_allocator_get_owner(allocator) == pool) {
@@ -724,6 +815,9 @@
return APR_ENOMEM;
}
+ node->next = node;
+ node->ref = &node->next;
+
pool = (apr_pool_t *)node->first_avail;
node->first_avail = pool->self_first_avail = (char *)pool + SIZEOF_POOL_T;
@@ -791,7 +885,7 @@
struct psprintf_data {
apr_vformatter_buff_t vbuff;
apr_memnode_t *node;
- apr_allocator_t *allocator;
+ apr_pool_t *pool;
apr_byte_t got_a_new_node;
apr_memnode_t *free;
};
@@ -800,29 +894,70 @@
{
struct psprintf_data *ps = (struct psprintf_data *)vbuff;
apr_memnode_t *node, *active;
- apr_size_t cur_len;
+ apr_size_t cur_len, size;
char *strp;
- apr_allocator_t *allocator;
+ apr_pool_t *pool;
+ apr_uint32_t free_index;
- allocator = ps->allocator;
- node = ps->node;
+ pool = ps->pool;
+ active = ps->node;
strp = ps->vbuff.curpos;
- cur_len = strp - node->first_avail;
+ cur_len = strp - active->first_avail;
+ size = cur_len << 1;
- if ((active = apr_allocator_alloc(allocator, cur_len << 1)) == NULL)
- return -1;
+ node = active->next;
+ if (!ps->got_a_new_node && node->first_avail + size < node->endp) {
+ *node->ref = node->next;
+ node->next->ref = node->ref;
+
+ node->ref = active->ref;
+ *node->ref = node;
+ node->next = active;
+ active->ref = &node->next;
+
+ node->free_index = 0;
+
+ pool->active = node;
+
+ free_index = (APR_ALIGN(active->endp - active->first_avail + 1,
+ BOUNDARY_SIZE) - BOUNDARY_SIZE) >> BOUNDARY_INDEX;
+
+ active->free_index = free_index;
+ node = active->next;
+ if (free_index < node->free_index) {
+ do {
+ node = node->next;
+ }
+ while (free_index < node->free_index);
+
+ *active->ref = active->next;
+ active->next->ref = active->ref;
- memcpy(active->first_avail, node->first_avail, cur_len);
+ active->ref = node->ref;
+ *active->ref = active;
+ active->next = node;
+ node->ref = &active->next;
+ }
+
+ node = pool->active;
+ }
+ else {
+ if ((node = apr_allocator_alloc (pool->allocator, size)) == NULL)
+ return -1;
+
+ if (ps->got_a_new_node) {
+ active->next = ps->free;
+ ps->free = node;
+ }
- if (ps->got_a_new_node) {
- node->next = ps->free;
- ps->free = node;
+ ps->got_a_new_node = 1;
}
- ps->node = active;
- ps->vbuff.curpos = active->first_avail + cur_len;
- ps->vbuff.endpos = active->endp - 1; /* Save a byte for NUL terminator */
- ps->got_a_new_node = 1;
+ memcpy(node->first_avail, active->first_avail, cur_len);
+
+ ps->node = node;
+ ps->vbuff.curpos = node->first_avail + cur_len;
+ ps->vbuff.endpos = node->endp - 1; /* Save a byte for NUL terminator */
return 0;
}
@@ -832,10 +967,11 @@
struct psprintf_data ps;
char *strp;
apr_size_t size;
- apr_memnode_t *active;
+ apr_memnode_t *active, *node;
+ apr_uint32_t free_index;
ps.node = active = pool->active;
- ps.allocator = pool->allocator;
+ ps.pool = pool;
ps.vbuff.curpos = ps.node->first_avail;
/* Save a byte for the NUL terminator */
@@ -858,15 +994,48 @@
strp = ps.node->first_avail;
ps.node->first_avail += size;
+ if (ps.free)
+ apr_allocator_free(pool->allocator, ps.free);
+
/*
* Link the node in if it's a new one
*/
- if (ps.got_a_new_node) {
- active->next = pool->active = ps.node;
+ if (!ps.got_a_new_node)
+ return strp;
+
+ active = pool->active;
+ node = ps.node;
+
+ node->free_index = 0;
+
+ node->ref = active->ref;
+ *node->ref = node;
+ node->next = active;
+ active->ref = &node->next;
+
+ pool->active = node;
+
+ free_index = (APR_ALIGN(active->endp - active->first_avail + 1,
+ BOUNDARY_SIZE) - BOUNDARY_SIZE) >> BOUNDARY_INDEX;
+
+ active->free_index = free_index;
+ node = active->next;
+
+ if (free_index >= node->free_index)
+ return strp;
+
+ do {
+ node = node->next;
}
+ while (free_index < node->free_index);
+
+ *active->ref = active->next;
+ active->next->ref = active->ref;
- if (ps.free)
- apr_allocator_free(ps.allocator, ps.free);
+ active->ref = node->ref;
+ *active->ref = active;
+ active->next = node;
+ node->ref = &active->next;
return strp;
}
Index: ./subversion/libsvn_subr/svn_error.c
===================================================================
--- ./subversion/libsvn_subr/svn_error.c
+++ ./subversion/libsvn_subr/svn_error.c Wed May 22 10:39:46 2002
@@ -25,6 +25,7 @@
#include "apr_pools.h"
#include "apr_strings.h"
#include "apr_hash.h"
+#include "apr_allocator.h"
#include "svn_pools.h"
#include "svn_error.h"
@@ -423,18 +424,46 @@
SVN_POOL_FUNC_DEFINE(apr_pool_t *, svn_pool_create)
{
apr_pool_t *ret_pool;
+ apr_allocator_t *allocator = NULL;
+ apr_status_t apr_err;
+
+ /* For the top level pool we want a seperate allocator */
+ if (pool == NULL)
+ {
+ apr_err = apr_allocator_create (&allocator);
+ if (apr_err)
+ abort_on_pool_failure (apr_err);
+
+ /* Use magic number for testing, 2MB */
+ apr_allocator_set_max_free (allocator, 4096 * 1024);
+ }
#if !APR_POOL_DEBUG
- apr_pool_create_ex (&ret_pool, pool, abort_on_pool_failure, NULL);
+ apr_pool_create_ex (&ret_pool, pool, abort_on_pool_failure, allocator);
#else /* APR_POOL_DEBUG */
apr_pool_create_ex_debug (&ret_pool, pool, abort_on_pool_failure,
- NULL, file_line);
+ allocator, file_line);
#endif /* APR_POOL_DEBUG */
/* If there is no parent, then initialize ret_pool as the "top". */
if (pool == NULL)
{
- apr_status_t apr_err = svn_error_init_pool (ret_pool);
+#if APR_HAS_THREADS
+ {
+ apr_thread_mutex_t *mutex;
+
+ apr_err = apr_thread_mutex_create (&mutex, APR_THREAD_MUTEX_DEFAULT,
+ ret_pool);
+ if (apr_err)
+ abort_on_pool_failure (apr_err);
+
+ apr_allocator_set_mutex (allocator, mutex);
+ }
+#endif /* APR_HAS_THREADS */
+
+ apr_allocator_set_owner (allocator, ret_pool);
+
+ apr_err = svn_error_init_pool (ret_pool);
if (apr_err)
abort_on_pool_failure (apr_err);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Pools space-time tradeoff
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Greg Hudson [mailto:ghudson@MIT.EDU]
>
> I don't have any problems with this patch, but... isn't APR's pool
> system doing way too much work? If I were implementing it, I would
just
> malloc() a new block for each apr_palloc(), chain it into a list, and
> free() all those blocks when the pool is cleared or destroyed.
>
> Presumably the extra work is for some theoretical or actual
performance
> gain, since Apache is very performance-sensitive. Have people
actually
> measured a performance benefit from the current code over the dumb
> implementation?
>
> (Apologies for, most likely, treading on well-trammeled ground.)
The performance gain by not having the pools free memory is well
measured and understood. The thing is that most (if not all) long-lived
processes eventually will reach a steady-state with regard to how much
memory they need. By not calling free() from within the pools code, we
allow ourselves to reach this steady-state, and never again call
malloc() while processing a request. This means that the cost of the
original malloc() can be spread over EVERY request that the child
process handles, which is a serious performance boost.
Ryan
RE: [PATCH] Pools space-time tradeoff
Posted by Ryan Bloom <rb...@covalent.net>.
> From: Greg Hudson [mailto:ghudson@MIT.EDU]
>
> I don't have any problems with this patch, but... isn't APR's pool
> system doing way too much work? If I were implementing it, I would
just
> malloc() a new block for each apr_palloc(), chain it into a list, and
> free() all those blocks when the pool is cleared or destroyed.
>
> Presumably the extra work is for some theoretical or actual
performance
> gain, since Apache is very performance-sensitive. Have people
actually
> measured a performance benefit from the current code over the dumb
> implementation?
>
> (Apologies for, most likely, treading on well-trammeled ground.)
The performance gain by not having the pools free memory is well
measured and understood. The thing is that most (if not all) long-lived
processes eventually will reach a steady-state with regard to how much
memory they need. By not calling free() from within the pools code, we
allow ourselves to reach this steady-state, and never again call
malloc() while processing a request. This means that the cost of the
original malloc() can be spread over EVERY request that the child
process handles, which is a serious performance boost.
Ryan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pools space-time tradeoff
Posted by Greg Hudson <gh...@MIT.EDU>.
I don't have any problems with this patch, but... isn't APR's pool
system doing way too much work? If I were implementing it, I would just
malloc() a new block for each apr_palloc(), chain it into a list, and
free() all those blocks when the pool is cleared or destroyed.
Presumably the extra work is for some theoretical or actual performance
gain, since Apache is very performance-sensitive. Have people actually
measured a performance benefit from the current code over the dumb
implementation?
(Apologies for, most likely, treading on well-trammeled ground.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Pools space-time tradeoff
Posted by Sander Striker <st...@apache.org>.
> From: Brian Pane [mailto:bpane@pacbell.net]
> Sent: 26 May 2002 02:03
> On Sat, 2002-05-25 at 13:08, Brian Pane wrote:
>> I'll run httpd benchmarks using this patch and the current pool code.
>> If all goes well, I'll post results later today.
>
> Good news:
>
> The benchmark tests are finished, and I don't see any loss of
> httpd performance with Sander's patch.
>
> The test case:
> httpd-2.0.37-dev on Solaris 8/Sparc
> worker MPM, running on a server with 8 CPUs
> no libmtmalloc
> load generator sending 50 concurrent requests for a 0-byte file
> over 100Mb/s switched LAN
>
> The point of this test setup was to amplify the effect of any
> extra mallocs that the patch might require.
>
> Results:
>
> Configuration Requests/sec CPU util Load avg
> ------------- ------------ -------- --------
>
> 2.0.37 baseline 1206 72% 7.3
>
> plus Sander's patch 1213 71% 7.3
>
>
> The difference in throughput and CPU utilization is within the
> range of normal measurement error for this test setup, so I'd
> say that the httpd performance is identical with and without
> the patch.
Thanks for the benchmark test Brian!
I've committed both patches (which were tested as one in the above) to
APR. The Subversion patch is in rev 2017, so remember to update your
APR when you update Subversion.
Cliff, could you consider including these in 2.0.37? It would be
nice if a Subversion server could be built from a release tarball ;).
Thanks,
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Pools space-time tradeoff
Posted by Sander Striker <st...@apache.org>.
> From: Brian Pane [mailto:bpane@pacbell.net]
> Sent: 26 May 2002 02:03
> On Sat, 2002-05-25 at 13:08, Brian Pane wrote:
>> I'll run httpd benchmarks using this patch and the current pool code.
>> If all goes well, I'll post results later today.
>
> Good news:
>
> The benchmark tests are finished, and I don't see any loss of
> httpd performance with Sander's patch.
>
> The test case:
> httpd-2.0.37-dev on Solaris 8/Sparc
> worker MPM, running on a server with 8 CPUs
> no libmtmalloc
> load generator sending 50 concurrent requests for a 0-byte file
> over 100Mb/s switched LAN
>
> The point of this test setup was to amplify the effect of any
> extra mallocs that the patch might require.
>
> Results:
>
> Configuration Requests/sec CPU util Load avg
> ------------- ------------ -------- --------
>
> 2.0.37 baseline 1206 72% 7.3
>
> plus Sander's patch 1213 71% 7.3
>
>
> The difference in throughput and CPU utilization is within the
> range of normal measurement error for this test setup, so I'd
> say that the httpd performance is identical with and without
> the patch.
Thanks for the benchmark test Brian!
I've committed both patches (which were tested as one in the above) to
APR. The Subversion patch is in rev 2017, so remember to update your
APR when you update Subversion.
Cliff, could you consider including these in 2.0.37? It would be
nice if a Subversion server could be built from a release tarball ;).
Thanks,
Sander
Re: [PATCH] Pools space-time tradeoff
Posted by Brian Pane <bp...@pacbell.net>.
On Sat, 2002-05-25 at 13:08, Brian Pane wrote:
> I'll run httpd benchmarks using this patch and the current pool code.
> If all goes well, I'll post results later today.
Good news:
The benchmark tests are finished, and I don't see any loss of
httpd performance with Sander's patch.
The test case:
httpd-2.0.37-dev on Solaris 8/Sparc
worker MPM, running on a server with 8 CPUs
no libmtmalloc
load generator sending 50 concurrent requests for a 0-byte file
over 100Mb/s switched LAN
The point of this test setup was to amplify the effect of any
extra mallocs that the patch might require.
Results:
Configuration Requests/sec CPU util Load avg
------------- ------------ -------- --------
2.0.37 baseline 1206 72% 7.3
plus Sander's patch 1213 71% 7.3
The difference in throughput and CPU utilization is within the
range of normal measurement error for this test setup, so I'd
say that the httpd performance is identical with and without
the patch.
--Brian
Re: [PATCH] Pools space-time tradeoff
Posted by Brian Pane <bp...@pacbell.net>.
On Sat, 2002-05-25 at 13:08, Brian Pane wrote:
> I'll run httpd benchmarks using this patch and the current pool code.
> If all goes well, I'll post results later today.
Good news:
The benchmark tests are finished, and I don't see any loss of
httpd performance with Sander's patch.
The test case:
httpd-2.0.37-dev on Solaris 8/Sparc
worker MPM, running on a server with 8 CPUs
no libmtmalloc
load generator sending 50 concurrent requests for a 0-byte file
over 100Mb/s switched LAN
The point of this test setup was to amplify the effect of any
extra mallocs that the patch might require.
Results:
Configuration Requests/sec CPU util Load avg
------------- ------------ -------- --------
2.0.37 baseline 1206 72% 7.3
plus Sander's patch 1213 71% 7.3
The difference in throughput and CPU utilization is within the
range of normal measurement error for this test setup, so I'd
say that the httpd performance is identical with and without
the patch.
--Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pools space-time tradeoff
Posted by Brian Pane <bp...@pacbell.net>.
On Wed, 2002-05-22 at 10:15, Ben Collins-Sussman wrote:
>
> [crossposted to APR and SVN dev lists.]
>
> Hi, all. I've been working with Sander to resolve some pool problems.
...
> Anyway, I've included the patches below for people to look at.
> Apparently Sander posted these before, but nobody had a reason to
> care. Well, now Subversion *really* needs these features.
>
> I've tested this patch on our two use cases: the memory footprint for
> 'svn checkout' bounces up and down, but eventually plateaus around
> 25M. The footprint for 'svnadmin dump' also bounces around, but
> plateaus around 17M. A huge difference!
>
> Sander is worried about possible pool-performance hits from these
> changes, so he's hoping folks might be able to do some timing stats.
> (Personally, via my sophisticated use of the unix 'time' command, I
> find no noticeable difference.)
I'll run httpd benchmarks using this patch and the current pool code.
If all goes well, I'll post results later today.
--Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pools space-time tradeoff
Posted by Greg Hudson <gh...@MIT.EDU>.
I don't have any problems with this patch, but... isn't APR's pool
system doing way too much work? If I were implementing it, I would just
malloc() a new block for each apr_palloc(), chain it into a list, and
free() all those blocks when the pool is cleared or destroyed.
Presumably the extra work is for some theoretical or actual performance
gain, since Apache is very performance-sensitive. Have people actually
measured a performance benefit from the current code over the dumb
implementation?
(Apologies for, most likely, treading on well-trammeled ground.)
Re: [PATCH] Pools space-time tradeoff
Posted by Brian Pane <bp...@pacbell.net>.
On Wed, 2002-05-22 at 10:15, Ben Collins-Sussman wrote:
>
> [crossposted to APR and SVN dev lists.]
>
> Hi, all. I've been working with Sander to resolve some pool problems.
...
> Anyway, I've included the patches below for people to look at.
> Apparently Sander posted these before, but nobody had a reason to
> care. Well, now Subversion *really* needs these features.
>
> I've tested this patch on our two use cases: the memory footprint for
> 'svn checkout' bounces up and down, but eventually plateaus around
> 25M. The footprint for 'svnadmin dump' also bounces around, but
> plateaus around 17M. A huge difference!
>
> Sander is worried about possible pool-performance hits from these
> changes, so he's hoping folks might be able to do some timing stats.
> (Personally, via my sophisticated use of the unix 'time' command, I
> find no noticeable difference.)
I'll run httpd benchmarks using this patch and the current pool code.
If all goes well, I'll post results later today.
--Brian
Re: [PATCH] Pools space-time tradeoff
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Why do pools even have a separate "active" block vs a list of old
"inactive blocks"? Especially now if we're going to start reusing
some of the inactive blocks anyway...
What about this:
- When palloc() requests some mem, see if there's any block with
enough unused room to handle it. If so, allocate in that block.
Else, find or malloc a new block that is at least large enough to
hold the request, maybe doubling it against future allocations.
It may or may not be worthwhile to store the blocks sorted by
available unused space.
- When a pool is cleared/destroyed, walk its blocks marking
everything as unused. If that leaves a huge amount of unused
room (more than BLAH bytes), then free() some or all of it,
otherwise just give it back to the top level unused-block list.
This way, a pool just has a list of blocks. None is any more active
than another, and there's no special case, just normal code flow.
I'm aware that this is a loose description, but hope it's enough to
give the gist of the idea.
Apologies if all this has been gone over before. I didn't participate
in the original pool design discussions; I'm just basing these
comments on the way it seems to work today + the proposed changes.
-Karl
"Sander Striker" <st...@apache.org> writes:
> > 1. Keep each pool's list of 'old' active blocks sorted by size. If
> > a request comes in that won't fit in the current active block,
> > then try to alloc the request in the largest 'old' active block
> > (instead of alloc'ing a completely new active block).
> > Effectively, we get some memory re-use now.
> >
> > 2. When a pool is destroyed or cleared, normally all of its
> > inactive blocks are given back to a 'global free list' in the
> > pool hierarchy. We now have a threshold size that can be set:
> > if the global-free-list ever grows larger than the threshold,
> > then the 'extra' inactive blocks are actually free()d. We've
> > set the maximum free-list size at 4M for now.
> >
> > Anyway, I've included the patches below for people to look at.
> > Apparently Sander posted these before, but nobody had a reason to
> > care. Well, now Subversion *really* needs these features.
>
> Well, I need to clarify that the high free patch has come up before
> and a small group of people wanted it included (ianh and brianp iirc).
> I didn't commit it since the demand was too low and I was +0 myself.
> Seeing the impact it has on subversion and the fact that the developer
> has control over the threshold (which defaults to unlimited, so no
> surprises there), I've decided to go ahead and commit this change
> within a day or two unless major objections are raised.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Pools space-time tradeoff
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Why do pools even have a separate "active" block vs a list of old
"inactive blocks"? Especially now if we're going to start reusing
some of the inactive blocks anyway...
What about this:
- When palloc() requests some mem, see if there's any block with
enough unused room to handle it. If so, allocate in that block.
Else, find or malloc a new block that is at least large enough to
hold the request, maybe doubling it against future allocations.
It may or may not be worthwhile to store the blocks sorted by
available unused space.
- When a pool is cleared/destroyed, walk its blocks marking
everything as unused. If that leaves a huge amount of unused
room (more than BLAH bytes), then free() some or all of it,
otherwise just give it back to the top level unused-block list.
This way, a pool just has a list of blocks. None is any more active
than another, and there's no special case, just normal code flow.
I'm aware that this is a loose description, but hope it's enough to
give the gist of the idea.
Apologies if all this has been gone over before. I didn't participate
in the original pool design discussions; I'm just basing these
comments on the way it seems to work today + the proposed changes.
-Karl
"Sander Striker" <st...@apache.org> writes:
> > 1. Keep each pool's list of 'old' active blocks sorted by size. If
> > a request comes in that won't fit in the current active block,
> > then try to alloc the request in the largest 'old' active block
> > (instead of alloc'ing a completely new active block).
> > Effectively, we get some memory re-use now.
> >
> > 2. When a pool is destroyed or cleared, normally all of its
> > inactive blocks are given back to a 'global free list' in the
> > pool hierarchy. We now have a threshold size that can be set:
> > if the global-free-list ever grows larger than the threshold,
> > then the 'extra' inactive blocks are actually free()d. We've
> > set the maximum free-list size at 4M for now.
> >
> > Anyway, I've included the patches below for people to look at.
> > Apparently Sander posted these before, but nobody had a reason to
> > care. Well, now Subversion *really* needs these features.
>
> Well, I need to clarify that the high free patch has come up before
> and a small group of people wanted it included (ianh and brianp iirc).
> I didn't commit it since the demand was too low and I was +0 myself.
> Seeing the impact it has on subversion and the fact that the developer
> has control over the threshold (which defaults to unlimited, so no
> surprises there), I've decided to go ahead and commit this change
> within a day or two unless major objections are raised.
RE: [PATCH] Pools space-time tradeoff
Posted by Sander Striker <st...@apache.org>.
> From: sussman@collab.net [mailto:sussman@collab.net]
> Sent: 22 May 2002 19:16
> [crossposted to APR and SVN dev lists.]
[...]
> ** Here's the solution:
>
> Sander whipped out two of his pool patches that have been previously
> ignored up till now. He combined them into a single patch. Here's
> what they do:
>
> 1. Keep each pool's list of 'old' active blocks sorted by size. If
> a request comes in that won't fit in the current active block,
> then try to alloc the request in the largest 'old' active block
> (instead of alloc'ing a completely new active block).
> Effectively, we get some memory re-use now.
>
> 2. When a pool is destroyed or cleared, normally all of its
> inactive blocks are given back to a 'global free list' in the
> pool hierarchy. We now have a threshold size that can be set:
> if the global-free-list ever grows larger than the threshold,
> then the 'extra' inactive blocks are actually free()d. We've
> set the maximum free-list size at 4M for now.
>
> Anyway, I've included the patches below for people to look at.
> Apparently Sander posted these before, but nobody had a reason to
> care. Well, now Subversion *really* needs these features.
Well, I need to clarify that the high free patch has come up before
and a small group of people wanted it included (ianh and brianp iirc).
I didn't commit it since the demand was too low and I was +0 myself.
Seeing the impact it has on subversion and the fact that the developer
has control over the threshold (which defaults to unlimited, so no
surprises there), I've decided to go ahead and commit this change
within a day or two unless major objections are raised.
> I've tested this patch on our two use cases: the memory footprint for
> 'svn checkout' bounces up and down, but eventually plateaus around
> 25M. The footprint for 'svnadmin dump' also bounces around, but
> plateaus around 17M. A huge difference!
>
> Sander is worried about possible pool-performance hits from these
> changes, so he's hoping folks might be able to do some timing stats.
> (Personally, via my sophisticated use of the unix 'time' command, I
> find no noticeable difference.)
Ian, Brian, or anyone else doing thorough benchmarks, could you please
run some benchmarks on httpd with and without the patch?
Thanks,
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Pools space-time tradeoff
Posted by Sander Striker <st...@apache.org>.
> From: sussman@collab.net [mailto:sussman@collab.net]
> Sent: 22 May 2002 19:16
> [crossposted to APR and SVN dev lists.]
[...]
> ** Here's the solution:
>
> Sander whipped out two of his pool patches that have been previously
> ignored up till now. He combined them into a single patch. Here's
> what they do:
>
> 1. Keep each pool's list of 'old' active blocks sorted by size. If
> a request comes in that won't fit in the current active block,
> then try to alloc the request in the largest 'old' active block
> (instead of alloc'ing a completely new active block).
> Effectively, we get some memory re-use now.
>
> 2. When a pool is destroyed or cleared, normally all of its
> inactive blocks are given back to a 'global free list' in the
> pool hierarchy. We now have a threshold size that can be set:
> if the global-free-list ever grows larger than the threshold,
> then the 'extra' inactive blocks are actually free()d. We've
> set the maximum free-list size at 4M for now.
>
> Anyway, I've included the patches below for people to look at.
> Apparently Sander posted these before, but nobody had a reason to
> care. Well, now Subversion *really* needs these features.
Well, I need to clarify that the high free patch has come up before
and a small group of people wanted it included (ianh and brianp iirc).
I didn't commit it since the demand was too low and I was +0 myself.
Seeing the impact it has on subversion and the fact that the developer
has control over the threshold (which defaults to unlimited, so no
surprises there), I've decided to go ahead and commit this change
within a day or two unless major objections are raised.
> I've tested this patch on our two use cases: the memory footprint for
> 'svn checkout' bounces up and down, but eventually plateaus around
> 25M. The footprint for 'svnadmin dump' also bounces around, but
> plateaus around 17M. A huge difference!
>
> Sander is worried about possible pool-performance hits from these
> changes, so he's hoping folks might be able to do some timing stats.
> (Personally, via my sophisticated use of the unix 'time' command, I
> find no noticeable difference.)
Ian, Brian, or anyone else doing thorough benchmarks, could you please
run some benchmarks on httpd with and without the patch?
Thanks,
Sander