Re: svn commit: rev 7166 - trunk/subversion/libsvn_fs

On Tue, Sep 23, 2003 at 05:17:57PM -0500, cmpilato@tigris.org wrote:
> Author: cmpilato
> Date: Tue Sep 23 17:17:56 2003
> New Revision: 7166
>
> Modified:
> trunk/subversion/libsvn_fs/tree.c
> Log:
> Caching done (a little more closely to) right (I hope). Reviews, please?

Heh. Saw your original commit and was going to suggest the looping index
thing. I see you already got that feedback :-)

That said: I think the per-node pool is a poor idea, though I'm
hard-pressed to come up with a fix offhand. I *would* suggest documenting
why you're doing that since it is not a standard pattern anywhere in SVN
(that is: per-object pools). I'm guessing it is because you're
(essentially) putting an unbounded amount of data into fs_root->pool.
Since a caller is using the iteration-pool concept on a *different* pool,
that means you're "exiting" from the standard memory usage bounding
mechanism (the iterpool), and (thus) you need a way to manually provide a
bound. Correct?

I believe that pool creation is not nearly as cheap as a simple alloc.
Sander can correct me here, though. I've got a small tweak to improve the
pool handling a little bit:

>...
> +++ trunk/subversion/libsvn_fs/tree.c Tue Sep 23 17:17:56 2003
>...
> +static void
> +add_to_dag_node_cache (svn_fs_root_t *root,
> + const char *path,
> + dag_node_t *node)
> +{
> + const char *cache_path;
> + apr_pool_t *cache_pool = svn_pool_create (root->pool);

Change to just declare the pool.

> + struct dag_node_cache_t *cache_item;
> +
> + /* We're adding a new cache item. First, see if we have room for it
> + (otherwise, make some room). */
> + if (apr_hash_count (root->node_cache) == SVN_FS_NODE_CACHE_MAX_KEYS)
> + {
> + /* No room. Expire the oldest thing. */
> + cache_path = root->node_cache_keys[root->node_cache_idx];
> + cache_item = apr_hash_get (root->node_cache, cache_path,
> + APR_HASH_KEY_STRING);
> + apr_hash_set (root->node_cache, cache_path, APR_HASH_KEY_STRING, NULL);
> + svn_pool_destroy (cache_item->pool);
> + }

Adjust the above destroy to:

    cache_pool = cache_item->pool;
    svn_pool_clear (cache_pool);
  }
  else
    cache_pool = svn_pool_create (root->pool);

The net result is that you'll create N pools max, then start reusing them.

All that said, I'm not sure that it is relevant to do fine-tuned
optimizations around this code (i.e. we could avoid some hash lookups,
some strlen calls, etc). The first-order improvement was to avoid BDB
calls, which the cache has done. The overall time is probably still driven
by other BDB calls, and futher optimization here would generally make the
code a bit harder to read for little gain.

I would suggest, however, you omit the memset() and the zero assignment
from tree.c::make_root(). You already have an apr_pcalloc() there, so the
zeroing is completely redundant.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org