Re: svn_cache review

r30148, r31049, and r30150 address your concerns from this thread,
except for the const void * one, which I'm looking into now.

--dave

On Tue, Mar 25, 2008 at 5:08 PM, Eric Gillespie <epg_at_google.com> wrote:
> > Index: subversion/libsvn_fs_fs/tree.c
> > ===================================================================
>
> > /* Invalidate cache entries for PATH and any of its children. */
> > -static void
> > +static svn_error_t *
> > dag_node_cache_invalidate(svn_fs_root_t *root,
> > - const char *path)
> > + const char *path,
> > + apr_pool_t *pool)
> > {
> > - fs_txn_root_data_t *frd;
> > - apr_size_t len = strlen(path);
> > - const char *key;
> > - dag_node_cache_t *item;
> > + struct fdic_baton b;
> > + svn_cache_t *cache;
> > + int i;
> >
> > + b.path = path;
> > + b.pool = svn_pool_create(pool);
> > + b.list = apr_array_make(b.pool, 1, sizeof(const char *));
> > +
> > assert(root->is_txn_root);
> > + locate_cache(&cache, NULL, root, NULL, b.pool);
> >
> > - frd = root->fsap_data;
> >
> > - for (item = frd->txn_node_list.next;
> > - item != &frd->txn_node_list;
> > - item = item->next)
> > + SVN_ERR(svn_cache_iter(NULL, cache, find_descendents_in_cache,
> > + &b, b.pool));
> > +
> > + for (i = 0; i < b.list->nelts; i++)
> > {
> > - key = item->key;
> > - if (strncmp(key, path, len) == 0 && (key[len] == '/' || !key[len]))
> > - item->node = NULL;
> > + const char *descendent = APR_ARRAY_IDX(b.list, i, const char *);
> > + SVN_ERR(svn_cache_set(cache, descendent, NULL, b.pool));
>
> Use an iterpool for this loop?
>
> > +make_txn_root(svn_fs_root_t **root_p,
> > + svn_fs_t *fs,
> > const char *txn,
> > svn_revnum_t base_rev,
> > apr_uint32_t flags,
> > @@ -3801,11 +3775,14 @@
> > root->txn_flags = flags;
> > root->rev = base_rev;
> >
> > - frd->txn_node_cache = apr_hash_make(root->pool);
> > - frd->txn_node_list.prev = &frd->txn_node_list;
> > - frd->txn_node_list.next = &frd->txn_node_list;
> > + /* Because this cache actually tries to invalidate elements, keep
> > + the number of elements per page down. */
> > + SVN_ERR(svn_cache_create(&(frd->txn_node_cache),
> > + svn_fs_fs__dag_dup_for_cache, APR_HASH_KEY_STRING,
> > + 32, 20, FALSE, root->pool));
>
> We don't have to go there right now, but it seems unlikely to me
> that one cache size fits all; should we allow admins to tune them?
>
> > Index: subversion/libsvn_fs_fs/fs.c
> > ===================================================================
>
> > @@ -114,6 +122,50 @@
> >
> > ffd->shared = ffsd;
> >
> > + /* Now do it all again for the caches; this key contains the path. */
> > + key = apr_pstrcat(pool, SVN_FSFS_SHARED_CACHES_USERDATA_PREFIX, ffd->uuid,
> > + "/", fs->path, (char *) NULL);
> > + status = apr_pool_userdata_get(&val, key, common_pool);
> > + if (status)
> > + return svn_error_wrap_apr(status, _("Can't fetch FSFS shared caches data"));
> > + ffsc = val;
> > +
> > + if (!ffsc)
> > + {
> > + ffsc = apr_pcalloc(common_pool, sizeof(*ffsc));
> > +
> > + /* Make the cache for revision roots. For the vast majority of
> > + * commands, this is only going to contain a few entries (svnadmin
> > + * dump/verify is an exception here), so to reduce overhead let's
> > + * try to keep it to just one page. I estimate each entry has about
> > + * 72 bytes of overhead (svn_revnum_t key, svn_fs_id_t +
> > + * id_private_t + 3 strings for value, and the cache_entry); the
> > + * default pool size is 8192, so about a hundred should fit
> > + * comfortably. */
> > + SVN_ERR(svn_cache_create(&(ffsc->rev_root_id_cache),
> > + dup_ids, sizeof(svn_revnum_t),
> > + 1, 100, TRUE, common_pool));
> > +
> > + /* Rough estimate: revision DAG nodes have size around 320 bytes, so
> > + * let's put 16 on a page. */
> > + SVN_ERR(svn_cache_create(&(ffsc->rev_node_cache),
> > + svn_fs_fs__dag_dup_for_cache,
> > + APR_HASH_KEY_STRING,
> > + 1024, 16, TRUE, common_pool));
> > +
> > + /* Very rough estimate: 1K per directory. */
> > + SVN_ERR(svn_cache_create(&(ffsc->dir_cache),
> > + dup_dir_listing, APR_HASH_KEY_STRING,
> > + 1024, 8, TRUE, common_pool));
>
> As we discussed in person, allocating these things in a
> process-lifetime pool (common_pool in fs-loader.c) is probably
> not a problem, but worth documenting.
>
> > +/* We have two different "shared between all svn_fs_t objects for the
> > + same filesystem" structures, allocated in the common pool. Why two?
> [...]
>
> Thanks for all the great comments; they really helped.
>
> I had a lot less to say on this round, so I may have missed
> potential problems if I didn't sufficiently understand some
> surrounding context of existing code. It all looked right, but
> it would be nice to have another person as familiar with fsfs as
> you to review this, too.
>

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org