You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2014/03/27 00:22:23 UTC

svn commit: r1582117 - in /subversion/trunk/subversion: include/private/svn_object_pool.h include/private/svn_repos_private.h libsvn_repos/authz_pool.c libsvn_repos/config_pool.c libsvn_subr/object_pool.c tests/libsvn_repos/repos-test.c

Author: stefan2
Date: Wed Mar 26 23:22:22 2014
New Revision: 1582117

URL: http://svn.apache.org/r1582117
Log:
Simplify svn_object_pool__t code:  Support shared objects only and use
standard APR memory pool handling.

We simply provide ref-counted objects allocated through from a sub-pool
of some user-provided memory pool.  References returned will no longer
survive the cleanup of that user-provided container pool.  Unused objects
may still be kept but the specifics are now an implementation detail.

* subversion/include/private/svn_object_pool.h
  (svn_object_pool__create): Non-sharable objects are no longer supported
                             and caching policies are hidden now.
  (svn_object_pool__pool): Replaced by ...
  (svn_object_pool__new_wrapper_pool): ... this one; directly returning a
                                       new sub-pool that may be used to
                                       hold a new object instance.
  (svn_object_pool__count): Update docstring.

* subversion/libsvn_subr/object_pool.c
  (object_ref_t): Since all objects can be shared, there is no need to
                  chain multiple instances.
  (svn_object_pool__t): We only keep two atomic counters for bookkeeping.
                        There is only a standard pool now and no user-
                        defined caching parameters.
  (destroy_object_pool): Drop as no specific destruction code is needed.
  (object_pool_cleanup): Do standard cleanup now and only verify correct
                         reference lifetimes.
  (root_object_pool_cleanup): No longer needed with standard pool handling.
  (remove_unused_objects): Update removal logic.  Also, we don't need to
                           the APR hash as it's implementation ensures that
                           adding & removing entries does not make memory
                           consumption to go up.
  (exit_on_error): No longer needed with standard pool cleanup.
  (object_ref_cleanup): Re-implement.  Still keep the actual object around
                        but notify the container that there are unused
                        objects that can be cleaned up at its discretion.
  (add_object_ref): Update only the counters that we still have.
  (insert): Adapt to simplified model and keep it consistent even in case
            of an error.  Implement caching / lazy object cleanup here.
  (svn_object_pool__create): Update to simplified data model and standard
                             memory pool usage.
  (svn_object_pool__pool): Replace by ... 
  (svn_object_pool__new_wrapper_pool): ... this.
  (svn_object_pool__count): Adapt to struct changes.

* subversion/include/private/svn_repos_private.h
  (svn_repos__config_pool_create,
   svn_repos__config_pool_get,
   svn_repos__authz_pool_create,
   svn_repos__authz_pool_get): Update docstrings to reflect the changed
                               object / pool lifetime restrictions.

* subversion/libsvn_repos/authz_pool.c
  (svn_repos__authz_pool_create): Adapt to changed API and put the container
                                  into the POOL provided by the user.
  (svn_repos__authz_pool_get): Adapt to changed API.

* subversion/libsvn_repos/config_pool.c
  (auto_parse): Ditto
  (svn_repos__config_pool_create): Same.  Also, put the container into the
                                   POOL provided by the user.

* subversion/tests/libsvn_repos/repos-test.c
  (test_config_pool): References must not live longer than the container
                      anymore.  Thus, remove this part of the test case.

Modified:
    subversion/trunk/subversion/include/private/svn_object_pool.h
    subversion/trunk/subversion/include/private/svn_repos_private.h
    subversion/trunk/subversion/libsvn_repos/authz_pool.c
    subversion/trunk/subversion/libsvn_repos/config_pool.c
    subversion/trunk/subversion/libsvn_subr/object_pool.c
    subversion/trunk/subversion/tests/libsvn_repos/repos-test.c

Modified: subversion/trunk/subversion/include/private/svn_object_pool.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_object_pool.h?rev=1582117&r1=1582116&r2=1582117&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_object_pool.h (original)
+++ subversion/trunk/subversion/include/private/svn_object_pool.h Wed Mar 26 23:22:22 2014
@@ -90,16 +90,8 @@ typedef svn_error_t * (* svn_object_pool
  * one (or both) may be NULL and the default implementation assumes that
  * wrapper == object and updating is a no-op.
  *
- * Unused objects won't be cleaned up immediately to speed up repeated
- * lookup / release sequences.  The implementation will try to keep the
- * number of unused objects between MIN_UNUSED and MAX_UNUSED.  However,
- * there is no guarantee that it will remain in that range.
- *
- * If SHARE_OBJECTS is TRUE, #svn_object_pool__lookup will return multiple
- * references to the same object for the same key.  Otherwise, all used
- * references for the same key will be to independent instances.  If
- * THREAD_SAFE is not set, neither the object pool nor the object references
- * returned from it may be accessed from multiple threads.
+ * If THREAD_SAFE is not set, neither the object pool nor the object
+ * references returned from it may be accessed from multiple threads.
  *
  * It is not legal to call any API on the object pool after POOL got
  * cleared or destroyed.  However, existing object references handed out
@@ -110,24 +102,20 @@ svn_error_t *
 svn_object_pool__create(svn_object_pool__t **object_pool,
                         svn_object_pool__getter_t getter,
                         svn_object_pool__setter_t setter,
-                        apr_size_t min_unused,
-                        apr_size_t max_unused,
-                        svn_boolean_t share_objects,
                         svn_boolean_t thread_safe,
                         apr_pool_t *pool);
 
 /* Return the root pool containing the OBJECT_POOL and all sub-structures.
  */
 apr_pool_t *
-svn_object_pool__pool(svn_object_pool__t *object_pool);
+svn_object_pool__new_wrapper_pool(svn_object_pool__t *object_pool);
 
 /* Return the mutex used to serialize all OBJECT_POOL access.
  */
 svn_mutex__t *
 svn_object_pool__mutex(svn_object_pool__t *object_pool);
 
-/* Return the total number of object instances in OBJECT_POOL, including
- * unused ones.
+/* Return the number of object instances (used or unused) in OBJECT_POOL.
  */
 unsigned
 svn_object_pool__count(svn_object_pool__t *object_pool);

Modified: subversion/trunk/subversion/include/private/svn_repos_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_repos_private.h?rev=1582117&r1=1582116&r2=1582117&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_repos_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_repos_private.h Wed Mar 26 23:22:22 2014
@@ -162,11 +162,11 @@ svn_repos__retrieve_config(svn_config_t 
  */
 typedef struct svn_repos__config_pool_t svn_repos__config_pool_t;
 
-/* Create a new configuration pool object with a minimum lifetime determined
- * by POOL and return it in *CONFIG_POOL.  References to any configuration
- * in the *CONFIG_POOL will keep the latter alive beyond POOL cleanup.
+/* Create a new configuration pool object with a lifetime determined by
+ * POOL and return it in *CONFIG_POOL.
+ * 
  * The THREAD_SAFE flag indicates whether the pool actually needs to be
- * thread-safe.
+ * thread-safe and POOL must be also be thread-safe if this flag is set.
  */
 svn_error_t *
 svn_repos__config_pool_create(svn_repos__config_pool_t **config_pool,
@@ -188,7 +188,9 @@ svn_repos__config_pool_create(svn_repos_
  * instead of creating a new repo instance.  Note that this might not
  * return the latest content.
  *
- * POOL determines the minimum lifetime of *CFG.
+ * POOL determines the minimum lifetime of *CFG (may remain cached after
+ * release) but must not exceed the lifetime of the pool provided to
+ * #svn_repos__config_pool_create.
  */
 svn_error_t *
 svn_repos__config_pool_get(svn_config_t **cfg,
@@ -215,14 +217,13 @@ svn_repos__config_pool_get(svn_config_t 
  */
 typedef struct svn_repos__authz_pool_t svn_repos__authz_pool_t;
 
-/* Create a new authorization pool object with a minimum lifetime determined
- * by POOL and return it in *AUTHZ_POOL.  CONFIG_POOL will be the common
- * source for the configuration data underlying the authz objects.
+/* Create a new authorization pool object with a lifetime determined by
+ * POOL and return it in *AUTHZ_POOL.  CONFIG_POOL will be the common
+ * source for the configuration data underlying the authz objects and must
+ * remain valid at least until POOL cleanup.
+ *
  * The THREAD_SAFE flag indicates whether the pool actually needs to be
- * thread-safe.
- * 
- * References to any authorization object in the *AUTHZ_POOL will keep the
- * latter alive beyond POOL cleanup.
+ * thread-safe and POOL must be also be thread-safe if this flag is set.
  */
 svn_error_t *
 svn_repos__authz_pool_create(svn_repos__authz_pool_t **authz_pool,
@@ -244,7 +245,9 @@ svn_repos__authz_pool_create(svn_repos__
  * instead of creating a new repo instance.  Note that this might not
  * return the latest content.
  *
- * POOL determines the minimum lifetime of *AUTHZ_P.
+ * POOL determines the minimum lifetime of *AUTHZ_P (may remain cached
+ * after release) but must not exceed the lifetime of the pool provided to
+ * svn_repos__authz_pool_create.
  */
 svn_error_t *
 svn_repos__authz_pool_get(svn_authz_t **authz_p,

Modified: subversion/trunk/subversion/libsvn_repos/authz_pool.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/authz_pool.c?rev=1582117&r1=1582116&r2=1582117&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/authz_pool.c (original)
+++ subversion/trunk/subversion/libsvn_repos/authz_pool.c Wed Mar 26 23:22:22 2014
@@ -130,11 +130,10 @@ svn_repos__authz_pool_create(svn_repos__
   svn_object_pool__t *object_pool;
 
   /* there is no setter as we don't need to update existing authz */
-  SVN_ERR(svn_object_pool__create(&object_pool, getter, NULL,
-                                  4, APR_UINT32_MAX, TRUE, thread_safe,
+  SVN_ERR(svn_object_pool__create(&object_pool, getter, NULL, thread_safe,
                                   pool));
 
-  result = apr_pcalloc(svn_object_pool__pool(object_pool), sizeof(*result));
+  result = apr_pcalloc(pool, sizeof(*result));
   result->object_pool = object_pool;
   result->config_pool = config_pool;
 
@@ -152,7 +151,7 @@ svn_repos__authz_pool_get(svn_authz_t **
                           apr_pool_t *pool)
 {
   apr_pool_t *authz_ref_pool
-    = svn_pool_create(svn_object_pool__pool(authz_pool->object_pool));
+    = svn_object_pool__new_wrapper_pool(authz_pool->object_pool);
   authz_object_t *authz_ref
     = apr_pcalloc(authz_ref_pool, sizeof(*authz_ref));
   svn_boolean_t have_all_keys;

Modified: subversion/trunk/subversion/libsvn_repos/config_pool.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/config_pool.c?rev=1582117&r1=1582116&r2=1582117&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/config_pool.c (original)
+++ subversion/trunk/subversion/libsvn_repos/config_pool.c Wed Mar 26 23:22:22 2014
@@ -212,7 +212,7 @@ auto_parse(svn_config_t **cfg,
     return SVN_NO_ERROR;
 
   /* create a pool for the new config object and parse the data into it  */
-  cfg_pool = svn_pool_create(svn_object_pool__pool(config_pool->object_pool));
+  cfg_pool = svn_object_pool__new_wrapper_pool(config_pool->object_pool);
 
   config_object = apr_pcalloc(cfg_pool, sizeof(*config_object));
 
@@ -439,19 +439,16 @@ svn_repos__config_pool_create(svn_repos_
 {
   svn_repos__config_pool_t *result;
   svn_object_pool__t *object_pool;
-  apr_pool_t *root_pool;
 
   SVN_ERR(svn_object_pool__create(&object_pool, getter, setter,
-                                  4, APR_UINT32_MAX, TRUE, thread_safe,
-                                  pool));
-  root_pool = svn_object_pool__pool(object_pool);
+                                  thread_safe, pool));
 
   /* construct the config pool in our private ROOT_POOL to survive POOL
    * cleanup and to prevent threading issues with the allocator */
-  result = apr_pcalloc(root_pool, sizeof(*result));
+  result = apr_pcalloc(pool, sizeof(*result));
 
   result->object_pool = object_pool;
-  result->in_repo_hash_pool = svn_pool_create(root_pool);
+  result->in_repo_hash_pool = svn_pool_create(pool);
   result->in_repo_configs = svn_hash__make(result->in_repo_hash_pool);
 
   *config_pool = result;

Modified: subversion/trunk/subversion/libsvn_subr/object_pool.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/object_pool.c?rev=1582117&r1=1582116&r2=1582117&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/object_pool.c (original)
+++ subversion/trunk/subversion/libsvn_subr/object_pool.c Wed Mar 26 23:22:22 2014
@@ -57,9 +57,6 @@ typedef struct object_ref_t
 
   /* Number of references to this data struct */
   volatile svn_atomic_t ref_count;
-
-  /* next struct in the hash bucket (same KEY) */
-  struct object_ref_t *next;
 } object_ref_t;
 
 
@@ -70,11 +67,6 @@ struct svn_object_pool__t
   /* serialization object for all non-atomic data in this struct */
   svn_mutex__t *mutex;
 
-  /* set to TRUE when pool passed to svn_object_pool__create() gets cleaned
-   * up.  When set, the last object reference released must also destroy
-   * this object pool object. */
-  volatile svn_atomic_t ready_for_cleanup;
-
   /* object_ref_t.KEY -> object_ref_t* mapping.
    *
    * In shared object mode, there is at most one such entry per key and it
@@ -83,172 +75,68 @@ struct svn_object_pool__t
    * instances for the key. */
   apr_hash_t *objects;
 
-  /* if TRUE, we operate in shared mode and in exclusive mode otherwise.
-   * This must not change over the lifetime. */
-  svn_boolean_t share_objects;
+  /* same as objects->count but allows for non-sync'ed access */
+  volatile svn_atomic_t object_count;
 
-  /* number of entries in CONFIGS with a reference count > 0 */
-  volatile svn_atomic_t used_count;
+  /* Number of entries in OBJECTS with a reference count 0.
+     Due to races, this may be *temporarily* off by one or more.
+     Hence we must not strictly depend on it. */
   volatile svn_atomic_t unused_count;
 
-  /* try to keep UNUSED_COUNT within this range */
-  apr_size_t min_unused;
-  apr_size_t max_unused;
-
   /* the root pool owning this structure */
-  apr_pool_t *root_pool;
-
-  /* this pool determines the minimum lifetime of this container.
-   * We use this to unregister cleanup routines (see below). */
-  apr_pool_t *owning_pool;
+  apr_pool_t *pool;
   
-  /* allocate the OBJECTS index here */
-  apr_pool_t *objects_hash_pool;
-
   /* extractor and updater for the user object wrappers */
   svn_object_pool__getter_t getter;
   svn_object_pool__setter_t setter;
 };
 
 
-/* Destructor function for the whole OBJECT_POOL.
- */
-static apr_status_t
-destroy_object_pool(svn_object_pool__t *object_pool)
-{
-  svn_mutex__lock(object_pool->mutex);
-
-  /* there should be no outstanding references to any object in this pool */
-  assert(svn_atomic_read(&object_pool->used_count) == 0);
-
-  /* make future attempts to access this pool cause definitive segfaults */
-  object_pool->objects = NULL;
-
-  /* This is the actual point of destruction. */
-  /* Destroying the pool will also release the lock. */
-  svn_pool_destroy(object_pool->root_pool);
-
-  return APR_SUCCESS;
-}
-
-/* Forward-declaration */
-static apr_status_t
-root_object_pool_cleanup(void *baton);
-
-/* Pool cleanup function for the whole object pool.  Actual destruction
- * will be deferred until no configurations are left in use.
+/* Pool cleanup function for the whole object pool.
  */
 static apr_status_t
 object_pool_cleanup(void *baton)
 {
   svn_object_pool__t *object_pool = baton;
 
-  /* disable the alternative cleanup */
-  apr_pool_cleanup_kill(object_pool->root_pool, baton,
-                        root_object_pool_cleanup);
-
-  /* from now on, anyone is allowed to destroy the OBJECT_POOL */
-  svn_atomic_set(&object_pool->ready_for_cleanup, TRUE);
-
-  /* are there no more external references and can we grab the cleanup flag? */
-  if (   svn_atomic_read(&object_pool->used_count) == 0
-      && svn_atomic_cas(&object_pool->ready_for_cleanup, FALSE, TRUE) == TRUE)
-    {
-      /* Attempts to get a configuration from a pool whose cleanup has
-       * already started is illegal.
-       * So, used_count must not increase again.
-       */
-      destroy_object_pool(object_pool);
-    }
+  /* all entries must have been released up by now */
+  SVN_ERR_ASSERT_NO_RETURN(   object_pool->object_count
+                           == object_pool->unused_count);
 
   return APR_SUCCESS;
 }
 
-/* This will be called when the root pool gets destroyed before the actual
- * owner pool.  This may happen if the owner pool is the global root pool. 
- * In that case, all the relevant cleanup has either already been done or
- * is default-scheduled.
- */
-static apr_status_t
-root_object_pool_cleanup(void *baton)
-{
-  svn_object_pool__t *object_pool = baton;
- 
-  /* disable the full-fledged cleanup code */
-  apr_pool_cleanup_kill(object_pool->owning_pool, baton,
-                        object_pool_cleanup);
-  
-  return APR_SUCCESS;
-}
-
-/* Re-allocate OBJECTS in OBJECT_POOL and remove all unused objects to
- * minimize memory consumption.
+/* Remove entries from OBJECTS in OBJECT_POOL that have a ref-count of 0.
  *
  * Requires external serialization on OBJECT_POOL.
  */
 static void
 remove_unused_objects(svn_object_pool__t *object_pool)
 {
-  apr_pool_t *new_pool = svn_pool_create(object_pool->root_pool);
-  apr_hash_t *new_hash = svn_hash__make(new_pool);
+  apr_pool_t *subpool = svn_pool_create(object_pool->pool);
 
   /* process all hash buckets */
   apr_hash_index_t *hi;
-  for (hi = apr_hash_first(object_pool->objects_hash_pool,
-                           object_pool->objects);
+  for (hi = apr_hash_first(subpool, object_pool->objects);
        hi != NULL;
        hi = apr_hash_next(hi))
     {
-      object_ref_t *previous = NULL;
       object_ref_t *object_ref = svn__apr_hash_index_val(hi);
-      object_ref_t *next;
 
-      /* follow the chain of object_ref_ts in this bucket */
-      for (; object_ref; object_ref = next)
+      /* note that we won't hand out new references while access
+         to the hash is serialized */
+      if (svn_atomic_read(&object_ref->ref_count) == 0)
         {
-          next = object_ref->next;
-          if (object_ref->ref_count == 0)
-            {
-              svn_atomic_dec(&object_pool->unused_count);
-              svn_pool_destroy(object_ref->pool);
-            }
-          else
-            {
-              object_ref->next = previous;
-              apr_hash_set(new_hash, object_ref->key.data,
-                           object_ref->key.size, object_ref);
-              previous = object_ref;
-            }
-        }
+          apr_hash_set(object_pool->objects, object_ref->key.data,
+                       object_ref->key.size, NULL);
+          svn_atomic_dec(&object_pool->object_count);
+          svn_atomic_dec(&object_pool->unused_count);
 
+          svn_pool_destroy(object_ref->pool);
+        }
     }
 
-  /* swap out the old container for the new one */
-  svn_pool_destroy(object_pool->objects_hash_pool);
-  object_pool->objects = new_hash;
-  object_pool->objects_hash_pool = new_pool;
-}
-
-/* If ERR is not SVN_NO_ERROR, handle it and terminate the application.
- *
- * Please make this generic if necessary instead of duplicating this code.
- */
-static void
-exit_on_error(const char *file, int line, svn_error_t *err)
-{
-  if (err)
-    {
-      char buffer[1024];
-
-      /* The svn_error_clear() is to make static analyzers happy.
-         svn_error__malfunction() will never return */
-      svn_error_clear(
-            svn_error__malfunction(FALSE /* can_return */, file, line,
-                                   svn_err_best_message(err, buffer,
-                                                        sizeof(buffer))
-                                   ));
-      abort(); /* Only reached by broken malfunction handlers */
-    }
+  svn_pool_destroy(subpool);
 }
 
 /* Cleanup function called when an object_ref_t gets released.
@@ -259,64 +147,17 @@ object_ref_cleanup(void *baton)
   object_ref_t *object = baton;
   svn_object_pool__t *object_pool = object->object_pool;
 
-  /* if we don't share objects and we are not allowed to hold on to
-   * unused object, delete them immediately. */
-  if (!object_pool->share_objects && object_pool->max_unused == 0)
-    {
-       /* there must only be the one references we are releasing right now */
-      assert(object->ref_count == 1);
-      svn_pool_destroy(object->pool);
-
-      /* see below for a more info on this final cleanup check */
-      if (   svn_atomic_dec(&object_pool->used_count) == 0
-          && svn_atomic_cas(&object_pool->ready_for_cleanup, FALSE, TRUE)
-             == TRUE)
-        {
-          destroy_object_pool(object_pool);
-        }
-
-     return APR_SUCCESS;
-    }
-
-  /* begin critical section */
-  exit_on_error(__FILE__, __LINE__,
-                svn_error_trace(svn_mutex__lock(object_pool->mutex)));
-
-  /* put back into "available" container */
-  if (!object_pool->share_objects)
-    {
-      svn_membuf_t *key = &object->key;
-
-      object->next = apr_hash_get(object_pool->objects, key->data, key->size);
-      apr_hash_set(object_pool->objects, key->data, key->size, object);
-    }
-
-  /* Release unused configurations if there are relatively frequent. */
-  if (   object_pool->unused_count > object_pool->max_unused
-      ||   object_pool->used_count * 2 + object_pool->min_unused
-         < object_pool->unused_count)
-    {
-      remove_unused_objects(object_pool);
-    }
-
-  /* end critical section */
-  exit_on_error(__FILE__, __LINE__,
-                svn_error_trace(svn_mutex__unlock(object_pool->mutex, NULL)));
+  /* If we released the last reference to object, there is one more
+     unused entry.
 
-  /* Maintain reference counters and handle object cleanup */
+     Note that unused_count does not need to be always exact but only
+     needs to become exact *eventually* (we use it to check whether we
+     should remove unused objects every now and then).  I.e. it must
+     never drift off / get stuck but always reflect the true value once
+     all threads left the racy sections.
+   */
   if (svn_atomic_dec(&object->ref_count) == 0)
-    {
-      svn_atomic_inc(&object_pool->unused_count);
-      if (   svn_atomic_dec(&object_pool->used_count) == 0
-          && svn_atomic_cas(&object_pool->ready_for_cleanup, FALSE, TRUE)
-             == TRUE)
-        {
-          /* There cannot be any future references to a config in this pool.
-           * So, we are the last one and need to finally clean it up.
-           */
-          destroy_object_pool(object_pool);
-        }
-    }
+    svn_atomic_inc(&object_pool->unused_count);
 
   return APR_SUCCESS;
 }
@@ -330,20 +171,10 @@ static void
 add_object_ref(object_ref_t *object_ref,
               apr_pool_t *pool)
 {
-  /* in exclusive mode, we only keep unused items in our hash */
-  if (!object_ref->object_pool->share_objects)
-    {
-      apr_hash_set(object_ref->object_pool->objects, object_ref->key.data,
-                   object_ref->key.size, object_ref->next);
-      object_ref->next = NULL;
-    }
-
-  /* update ref counter and global usage counters */
+  /* Update ref counter. 
+     Note that this is racy with object_ref_cleanup; see comment there. */
   if (svn_atomic_inc(&object_ref->ref_count) == 0)
-    {
-      svn_atomic_inc(&object_ref->object_pool->used_count);
-      svn_atomic_dec(&object_ref->object_pool->unused_count);
-    }
+    svn_atomic_dec(&object_ref->object_pool->unused_count);
 
   /* make sure the reference gets released automatically */
   apr_pool_cleanup_register(pool, object_ref, object_ref_cleanup,
@@ -392,7 +223,7 @@ insert(void **object,
 {
   object_ref_t *object_ref
     = apr_hash_get(object_pool->objects, key->data, key->size);
-  if (object_ref && object_pool->share_objects)
+  if (object_ref)
     {
       /* entry already exists (e.g. race condition) */
       svn_error_t *err = object_pool->setter(&object_ref->wrapper,
@@ -406,8 +237,14 @@ insert(void **object,
            * available ones.
            */
           apr_hash_set(object_pool->objects, key->data, key->size, NULL);
+          svn_atomic_dec(&object_pool->object_count);
+
+          /* for the unlikely case that the object got created _and_
+           * already released since we last checked: */
+          if (svn_atomic_read(&object_ref->ref_count) == 0)
+            svn_atomic_dec(&object_pool->unused_count);
 
-          /* cleanup the new data as well because it's now safe to use
+          /* cleanup the new data as well because it's not safe to use
            * either.
            */
           svn_pool_destroy(wrapper_pool);
@@ -428,8 +265,6 @@ insert(void **object,
       object_ref->object_pool = object_pool;
       object_ref->wrapper = wrapper;
       object_ref->pool = wrapper_pool;
-      object_ref->next = apr_hash_get(object_pool->objects, key->data,
-                                      key->size);
 
       svn_membuf__create(&object_ref->key, key->size, wrapper_pool);
       object_ref->key.size = key->size;
@@ -437,6 +272,7 @@ insert(void **object,
 
       apr_hash_set(object_pool->objects, object_ref->key.data,
                    object_ref->key.size, object_ref);
+      svn_atomic_inc(&object_pool->object_count);
 
       /* the new entry is *not* in use yet.
        * add_object_ref will update counters again. 
@@ -448,6 +284,11 @@ insert(void **object,
   *object = object_pool->getter(object_ref->wrapper, baton, result_pool);
   add_object_ref(object_ref, result_pool);
 
+  /* limit memory usage */
+  if (svn_atomic_read(&object_pool->unused_count) * 2
+      > apr_hash_count(object_pool->objects) + 2)
+    remove_unused_objects(object_pool);
+
   return SVN_NO_ERROR;
 }
 
@@ -479,43 +320,21 @@ svn_error_t *
 svn_object_pool__create(svn_object_pool__t **object_pool,
                         svn_object_pool__getter_t getter,
                         svn_object_pool__setter_t setter,
-                        apr_size_t min_unused,
-                        apr_size_t max_unused,
-                        svn_boolean_t share_objects,
                         svn_boolean_t thread_safe,
                         apr_pool_t *pool)
 {
   svn_object_pool__t *result;
 
-  /* our allocator may need to be thread-safe */
-  apr_pool_t *root_pool
-    = apr_allocator_owner_get(svn_pool_create_allocator(thread_safe));
-
-  /* paranoia limiter code */
-  if (max_unused > APR_UINT32_MAX)
-    max_unused = APR_UINT32_MAX;
-  if (min_unused > APR_UINT32_MAX)
-    min_unused = APR_UINT32_MAX;
-
-  if (max_unused < min_unused)
-    max_unused = min_unused;
-
   /* construct the object pool in our private ROOT_POOL to survive POOL
    * cleanup and to prevent threading issues with the allocator
    */
-  result = apr_pcalloc(root_pool, sizeof(*result));
-  SVN_ERR(svn_mutex__init(&result->mutex, thread_safe, root_pool));
+  result = apr_pcalloc(pool, sizeof(*result));
+  SVN_ERR(svn_mutex__init(&result->mutex, thread_safe, pool));
 
-  result->root_pool = root_pool;
-  result->owning_pool = pool;
-  result->objects_hash_pool = svn_pool_create(root_pool);
-  result->objects = svn_hash__make(result->objects_hash_pool);
-  result->ready_for_cleanup = FALSE;
-  result->share_objects = share_objects;
+  result->pool = pool;
+  result->objects = svn_hash__make(result->pool);
   result->getter = getter ? getter : default_getter;
   result->setter = setter ? setter : default_setter;
-  result->min_unused = min_unused;
-  result->max_unused = max_unused;
 
   /* make sure we clean up nicely.
    * We need two cleanup functions of which exactly one will be run
@@ -526,17 +345,15 @@ svn_object_pool__create(svn_object_pool_
    */
   apr_pool_cleanup_register(pool, result, object_pool_cleanup,
                             apr_pool_cleanup_null);
-  apr_pool_cleanup_register(root_pool, result, root_object_pool_cleanup,
-                            apr_pool_cleanup_null);
   
   *object_pool = result;
   return SVN_NO_ERROR;
 }
 
 apr_pool_t *
-svn_object_pool__pool(svn_object_pool__t *object_pool)
+svn_object_pool__new_wrapper_pool(svn_object_pool__t *object_pool)
 {
-  return object_pool->root_pool;
+  return svn_pool_create(object_pool->pool);
 }
 
 svn_mutex__t *
@@ -548,7 +365,7 @@ svn_object_pool__mutex(svn_object_pool__
 unsigned
 svn_object_pool__count(svn_object_pool__t *object_pool)
 {
-  return object_pool->used_count + object_pool->unused_count;
+  return svn_atomic_read(&object_pool->object_count);
 }
 
 svn_error_t *

Modified: subversion/trunk/subversion/tests/libsvn_repos/repos-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_repos/repos-test.c?rev=1582117&r1=1582116&r2=1582117&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_repos/repos-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_repos/repos-test.c Wed Mar 26 23:22:22 2014
@@ -3327,7 +3327,6 @@ test_config_pool(const svn_test_opts_t *
   svn_config_t *cfg;
   apr_hash_t *sections1, *sections2;
   int i;
-  svn_boolean_t bvalue;
   svn_fs_txn_t *txn;
   svn_fs_root_t *root, *rev_root;
   svn_revnum_t rev;
@@ -3515,20 +3514,6 @@ test_config_pool(const svn_test_opts_t *
   svn_error_clear(err);
   svn_pool_clear(subpool);
 
-  /* as long as we keep a reference to a config, clearing the config pool
-     should not invalidate that reference */
-  SVN_ERR(svn_repos__config_pool_get(&cfg, NULL, config_pool,
-                                     svn_dirent_join(wrk_dir,
-                                                     "config-pool-test1.cfg",
-                                                     pool),
-                                     TRUE, TRUE, NULL, pool));
-  svn_pool_clear(config_pool_pool);
-  for (i = 0; i < 64000; ++i)
-    apr_pcalloc(config_pool_pool, 80);
-
-  SVN_ERR(svn_config_get_bool(cfg, &bvalue, "booleans", "true3", FALSE));
-  SVN_TEST_ASSERT(bvalue);
-
   return SVN_NO_ERROR;
 }