On Tue, 27 Feb 2001, Greg Stein wrote:
> Euh... I don't think we want another ring.
>
> A simpler idea is to have the apr_bucket_pool structure contain a pointer to
> an apr_bucket_heap structure. At cleanup time, you do the following:
...
> So what you get here is a single allocation of a heap bucket. Then, you
> lazily repoint all other buckets to the new heap bucket.
I came up with a really cool idea that's an even greater simplification of
this. It's coded up and ready to go, but I figured I'd give people a
chance to comment before I commit.
Basically, instead of having the pool bucket contain a pointer to a heap
bucket and having to fool with manipulating reference counts and keeping
track of when to destroy the old apr_bucket_pool struct, the
apr_bucket_pool *contains* the apr_bucket_heap struct that it will become
as its first element.
Since the apr_bucket_heap has an apr_bucket_refcount as ITS first element,
the pool and heap bucket share an apr_bucket_refcount (ie, no more
twiddling with reference counts).
All you have to do in the cleanup routine is malloc/memcpy out of the pool
and onto the heap and set p->data to NULL, which is the flag to later
pool_read() calls that they should morph their buckets. All that entails
is changing b->type to &apr_bucket_type_heap... even the data pointer is
already correct because the apr_bucket_heap is the first element of
apr_bucket_pool.
After the morph, heap_read() and heap_destroy() take right over where
pool_read() and pool_destroy() left off. When the last reference is
heap_destroy'ed, heap_destroy() free's the b->data. As far as it knows,
b->data is just an apr_bucket_heap, but it unwittingly cleans up the
entire apr_bucket_pool struct for us since the b->data pointer never
changed.
=-)
Patch attached for comments.
--Cliff
Index: buckets/apr_buckets_pool.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_buckets_pool.c,v
retrieving revision 1.14
diff -u -d -r1.14 apr_buckets_pool.c
--- buckets/apr_buckets_pool.c 2001/02/28 17:52:43 1.14
+++ buckets/apr_buckets_pool.c 2001/02/28 19:46:48
@@ -57,71 +57,87 @@
static apr_status_t pool_bucket_cleanup(void *data)
{
- apr_bucket_pool *h = data;
- apr_bucket *b = h->b;
- apr_off_t start = b->start;
- apr_off_t length = b->length;
+ apr_bucket_pool *p = data;
+ apr_bucket_heap *h = &p->heap; /* &p->heap == data */
- b = apr_bucket_heap_make(b, h->base, b->length, 1, NULL);
- b->start = start;
- b->length = length;
/*
- * XXX: this is fubar... only the *first* apr_bucket to reference
- * the bucket is changed to a heap bucket... ALL references must
- * be changed or at least "notified" in some way. Fix is in the
- * works. --jcw
+ * If the pool gets cleaned up, we have to copy the data out
+ * of the pool and onto the heap. But the apr_bucket's that
+ * point to this pool bucket need to be notified such that
+ * they can morph themselves into a true heap bucket the next
+ * time they try to read. To avoid having to manipulate
+ * reference counts and b->data pointers, the apr_bucket_pool
+ * actually _contains_ an apr_bucket_heap as its first element,
+ * so the two share their apr_bucket_refcount member and you
+ * can typecast a pool bucket struct to make it look like a
+ * regular old heap bucket struct.
*/
+ h->base = malloc(h->alloc_len); /* XXX: check for failure? */
+ memcpy(h->base, p->base, h->alloc_len);
+ p->base = NULL;
+
return APR_SUCCESS;
}
static apr_status_t pool_read(apr_bucket *b, const char **str,
apr_size_t *len, apr_read_type_e block)
{
- apr_bucket_pool *h = b->data;
+ apr_bucket_pool *p = b->data;
+ const char *base = p->base;
- *str = h->base + b->start;
+ if (base == NULL) {
+ /* pool has been cleaned up... morph to a heap bucket */
+ apr_bucket_heap *h = &p->heap;
+ b->type = &apr_bucket_type_heap;
+ base = h->base;
+ }
+ *str = base + b->start;
*len = b->length;
return APR_SUCCESS;
}
static void pool_destroy(void *data)
{
- apr_bucket_pool *h = data;
+ apr_bucket_pool *p = data;
- apr_pool_cleanup_kill(h->p, data, pool_bucket_cleanup);
if (apr_bucket_shared_destroy(data)) {
- free(h);
+ apr_pool_cleanup_kill(p->pool, p, pool_bucket_cleanup);
+ free(p);
}
}
APU_DECLARE(apr_bucket *) apr_bucket_pool_make(apr_bucket *b,
- const char *buf, apr_size_t length, apr_pool_t *p)
+ const char *buf, apr_size_t length, apr_pool_t *pool)
{
- apr_bucket_pool *h;
+ apr_bucket_pool *p;
+ apr_bucket_heap *h;
- h = malloc(sizeof(*h));
- if (h == NULL) {
+ p = malloc(sizeof(*p));
+ if (p == NULL) {
return NULL;
}
/* XXX: we lose the const qualifier here which indicates
* there's something screwy with the API...
*/
- h->base = (char *) buf;
- h->p = p;
+ p->base = (char *) buf;
+ p->pool = pool;
+ h = &p->heap;
+ h->alloc_len = length; /* pre-initialize heap bucket member */
+ h->base = NULL;
- b = apr_bucket_shared_make(b, h, 0, length);
+ b = apr_bucket_shared_make(b, p, 0, length);
b->type = &apr_bucket_type_pool;
- h->b = b; /* XXX: see comment in pool_bucket_cleanup() */
- apr_pool_cleanup_register(h->p, b->data, pool_bucket_cleanup,
apr_pool_cleanup_null);
+ apr_pool_cleanup_register(p->pool, p, pool_bucket_cleanup,
+ apr_pool_cleanup_null);
return b;
}
APU_DECLARE(apr_bucket *) apr_bucket_pool_create(
- const char *buf, apr_size_t length, apr_pool_t *p)
+ const char *buf, apr_size_t length, apr_pool_t *pool)
{
- apr_bucket_do_create(apr_bucket_pool_make(b, buf, length, p));
+ apr_bucket_do_create(apr_bucket_pool_make(b, buf, length, pool));
}
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_pool = {
Index: include/apr_buckets.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
retrieving revision 1.86
diff -u -d -r1.86 apr_buckets.h
--- include/apr_buckets.h 2001/02/28 17:52:46 1.86
+++ include/apr_buckets.h 2001/02/28 19:47:01
@@ -499,41 +499,41 @@
/* ***** Reference-counted bucket types ***** */
-typedef struct apr_bucket_pool apr_bucket_pool;
+typedef struct apr_bucket_heap apr_bucket_heap;
/**
- * A bucket referring to data allocated from a pool
+ * A bucket referring to data allocated off the heap.
*/
-struct apr_bucket_pool {
+struct apr_bucket_heap {
/** Number of buckets using this memory */
apr_bucket_refcount refcount;
/** The start of the data actually allocated. This should never be
* modified, it is only used to free the bucket.
*/
- const char *base;
- /** The pool the data was allocated from */
- apr_pool_t *p;
- /** This is a hack, because we call apr_destroy_bucket with the ->data
- * pointer, so the pool cleanup needs to be registered with that pointer,
- * but the whole point of the cleanup is to convert the bucket to another
- * type. To do that conversion, we need a pointer to the bucket itself.
- * This gives us a pointer to the original bucket.
- */
- apr_bucket *b;
+ char *base;
+ /** how much memory was allocated */
+ size_t alloc_len;
};
-typedef struct apr_bucket_heap apr_bucket_heap;
+typedef struct apr_bucket_pool apr_bucket_pool;
/**
- * A bucket referring to data allocated off the heap.
+ * A bucket referring to data allocated from a pool
*/
-struct apr_bucket_heap {
- /** Number of buckets using this memory */
- apr_bucket_refcount refcount;
+struct apr_bucket_pool {
+ /** The pool bucket must be able to be easily morphed to a heap
+ * bucket if the pool gets cleaned up before all references are
+ * destroyed. To avoid having to do many extra refcount
+ * manipulations and b->data manipulations, the apr_bucket_pool
+ * struct actually *contains* the apr_bucket_heap struct that it
+ * will become as its first element. The two share their
+ * apr_bucket_refcount members.
+ */
+ apr_bucket_heap heap;
/** The start of the data actually allocated. This should never be
* modified, it is only used to free the bucket.
*/
- char *base;
- /** how much memory was allocated */
- size_t alloc_len;
+ const char *base;
+ /** The pool the data was allocated from */
+ apr_pool_t *pool;
};
#if APR_HAS_MMAP
@@ -1128,25 +1128,25 @@
* Create a bucket referring to memory allocated from a pool.
*
* @param buf The buffer to insert into the bucket
- * @param p The pool the memory was allocated from
+ * @param pool The pool the memory was allocated from
* @return The new bucket, or NULL if allocation failed
- * @deffunc apr_bucket *apr_bucket_pool_create(const char *buf, apr_size_t
*length, apr_pool_t *p)
+ * @deffunc apr_bucket *apr_bucket_pool_create(const char *buf, apr_size_t
*length, apr_pool_t *pool)
*/
APU_DECLARE(apr_bucket *)
apr_bucket_pool_create(const char *buf, apr_size_t length,
- apr_pool_t *p);
+ apr_pool_t *pool);
/**
* Make the bucket passed in a bucket refer to pool data
* @param b The bucket to make into a pool bucket
* @param buf The buffer to insert into the bucket
- * @param p The pool the memory was allocated from
+ * @param pool The pool the memory was allocated from
* @return The new bucket, or NULL if allocation failed
- * @deffunc apr_bucket *apr_bucket_pool_make(apr_bucket *b, const char *buf,
apr_size_t *length, apr_pool_t *p)
+ * @deffunc apr_bucket *apr_bucket_pool_make(apr_bucket *b, const char *buf,
apr_size_t *length, apr_pool_t *pool)
*/
APU_DECLARE(apr_bucket *)
apr_bucket_pool_make(apr_bucket *b, const char *buf,
- apr_size_t length, apr_pool_t *p);
+ apr_size_t length, apr_pool_t *pool);
#if APR_HAS_MMAP
/**
