Re: [PATCH] Trace editor should free dir_baton when it's done.

Karl Fogel <kfogel@newton.ch.collab.net> writes:

> I have only a few concerns below. Actually, the same concern three
> times. With the changes below, I'd say go ahead and commit it. (I
> assume it already passes "make check" for you, of course -- I didn't
> build the patch myself, just looked at it.)

Yes, I reran it with fixes you suggested and it passed all
tests (modulo I need to comment out new $EDITOR code to run
client tests).

> Thanks for the patch, Yoshiki!

You're welcome. But more simple and important patch I
attached to issuezilla seems to go unnoticed so I'll send it
to here, too. (I didn't know plain/text attachment wasn't
sent as is to bug@subversion.)

> `rb' should be created in `subpool', not `eb->pool', right? That way
> it fulfills the claim made by the comment in the middle of
> decrement_dir_ref_count().

Yes. Thanks for careful review. I found another bug in my
patch. File button should be allocated from directory's
pool, not from edit_baton's. I was wondering why memory
consumption doesn't change much and this was the reason.
Could you review this new patch? This passes make check and
I believe it is bug free.

Thom May <thom@planetarytramp.net> writes:

> > >(add_directory): Create subpool. Initialize refcount. Increment parent's
> > > refcount.
> > >(open_directory): Ditto.
> > >(close_directory): Call decrement_dir_ref_count.
> > >(clsoe_file): Ditto.
> ^^^^
> as an unfeasibly tiny niggle, I guess this typo should be fixed before the
> commit...

Thanks.

Implement freeing directory button using reference counting.

* trace-commit.c

(struct dir_baton): New member, subpool and ref_count.
(decrement_dir_ref_count): New function, copied from
  libsvn_ra_local/commit_editor.
(open_root): Create subpool corresponding the directory. Initialize refcount.
(add_directory): Create subpool. Initialize refcount. Increment parent's
  refcount.
(open_directory): Ditto.
(close_directory): Call decrement_dir_ref_count.
(close_file): Ditto.
(add_file): Increase parent's refcount. Use parent directory's pool.
(open_file): Ditto.

Index: ./subversion/clients/cmdline/trace-commit.c
===================================================================
--- ./subversion/clients/cmdline/trace-commit.c
+++ ./subversion/clients/cmdline/trace-commit.c Fri Jan 25 22:04:28 2002
@@ -51,6 +51,8 @@
   svn_stringbuf_t *path;
   svn_boolean_t added;
   svn_boolean_t prop_changed;
+ apr_pool_t *subpool;
+ int ref_count;
 };
 
 
@@ -66,14 +68,44 @@
 
 
 static svn_error_t *
+decrement_dir_ref_count (struct dir_baton *db)
+{
+ if (db == NULL)
+ return SVN_NO_ERROR;
+
+ db->ref_count--;
+
+ /* Check to see if *any* child batons still depend on this
+ directory's pool. */
+ if (db->ref_count == 0)
+ {
+ struct dir_baton *dbparent = db->parent_dir_baton;
+
+ /* Destroy all memory used by this baton, including the baton
+ itself! */
+ svn_pool_destroy (db->subpool);
+
+ /* Tell your parent that you're gone. */
+ SVN_ERR (decrement_dir_ref_count (dbparent));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
 open_root (void *edit_baton, svn_revnum_t base_revision, void **root_baton)
 {
+ apr_pool_t *subpool;
   struct edit_baton *eb = edit_baton;
- struct dir_baton *rb = apr_pcalloc (eb->pool, sizeof (*rb));
+ struct dir_baton *rb;
 
+ subpool = svn_pool_create (eb->pool);;
+ rb = apr_pcalloc (subpool, sizeof (*rb));
   rb->edit_baton = eb;
   rb->parent_dir_baton = NULL;
   rb->path = eb->initial_path;
+ rb->subpool = subpool;
+ rb->ref_count = 1;
 
   *root_baton = rb;
 
@@ -102,16 +134,21 @@
                svn_revnum_t copyfrom_revision,
                void **child_baton)
 {
+ apr_pool_t *subpool;
   struct dir_baton *parent_d = parent_baton;
- struct dir_baton *child_d
- = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_d));
+ struct dir_baton *child_d;
 
+ subpool = svn_pool_create (parent_d->edit_baton->pool);
+ child_d = apr_pcalloc (subpool, sizeof (*child_d));
   child_d->edit_baton = parent_d->edit_baton;
   child_d->parent_dir_baton = parent_d;
   child_d->path = svn_stringbuf_dup (parent_d->path,
                                      child_d->edit_baton->pool);
   svn_path_add_component (child_d->path, name);
   child_d->added = TRUE;
+ child_d->subpool = subpool;
+ child_d->ref_count = 1;
+ parent_d->ref_count++;
 
   printf ("Adding %s\n", child_d->path->data);
   *child_baton = child_d;
@@ -126,15 +163,20 @@
                 svn_revnum_t base_revision,
                 void **child_baton)
 {
+ apr_pool_t *subpool;
   struct dir_baton *parent_d = parent_baton;
- struct dir_baton *child_d
- = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_d));
+ struct dir_baton *child_d;
 
+ subpool = svn_pool_create (parent_d->edit_baton->pool);
+ child_d = apr_pcalloc (subpool, sizeof (*child_d));
   child_d->edit_baton = parent_d->edit_baton;
   child_d->parent_dir_baton = parent_d;
   child_d->path = svn_stringbuf_dup (parent_d->path,
                                      child_d->edit_baton->pool);
   svn_path_add_component (child_d->path, name);
+ child_d->subpool = subpool;
+ child_d->ref_count = 1;
+ parent_d->ref_count++;
 
   *child_baton = child_d;
 
@@ -153,6 +195,7 @@
   if (db->prop_changed)
     printf ("Sending %s\n", db->path->data);
 
+ decrement_dir_ref_count (db);
   return SVN_NO_ERROR;
 }
 
@@ -168,7 +211,8 @@
             fb->path->data);
   else
     printf ("Sending %s\n", fb->path->data);
-
+
+ decrement_dir_ref_count (fb->parent_dir_baton);
   return SVN_NO_ERROR;
 }
 
@@ -208,7 +252,7 @@
 {
   struct dir_baton *parent_d = parent_baton;
   struct file_baton *child_fb
- = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_fb));
+ = apr_pcalloc (parent_d->subpool, sizeof (*child_fb));
 
   child_fb->parent_dir_baton = parent_d;
   child_fb->path = svn_stringbuf_dup (parent_d->path,
@@ -218,6 +262,8 @@
   child_fb->binary = FALSE;
   *file_baton = child_fb;
 
+ parent_d->ref_count++;
+
   return SVN_NO_ERROR;
 }
 
@@ -230,7 +276,7 @@
 {
   struct dir_baton *parent_d = parent_baton;
   struct file_baton *child_fb
- = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_fb));
+ = apr_pcalloc (parent_d->subpool, sizeof (*child_fb));
 
   child_fb->parent_dir_baton = parent_d;
   child_fb->path = svn_stringbuf_dup (parent_d->path,
@@ -238,6 +284,8 @@
   svn_path_add_component (child_fb->path, name);
 
   *file_baton = child_fb;
+
+ parent_d->ref_count++;
 
   return SVN_NO_ERROR;
 }

-- 
Yoshiki Hayashi
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org