On Fri, Nov 28, 2008 at 11:03 PM, Hyrum K. Wright
<hyrum_wright_at_mail.utexas.edu> wrote:
> Dave,
> I've used the new caching infrastructure to implement a
> revision->pack-file-offset cache on the fsfs-pack branch. I've *think* that
> I've implemented everything correctly, but for some reason it isn't working as
> expected. Commenting out the call to svn_cache__get() removes the problems.
> (You can see this effect by running fs-pack-test on the branch.)
>
> On the very likely chance that the bug is something obvious, and is just my
> (ab)use of the caching APIs, could you take a look at the attached patch let me
> know if there are any glaring deficiencies?
>
> Thanks,
> -Hyrum
>
> [[[
> On the fsfs-pack branch:
> Cache revision offsets in the pack file, which prevents repeated opening and
> reading of the corresponding manifest file. I believe this change may also
> create performance parity with non-packed repositories, in terms of number
> of files opened and seeks performed, in the common case.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (get_packed_offset): Set and get the cache of revision->offset mappings.
>
> * subversion/libsvn_fs_fs/fs.h
> (fs_fs_data_t): Add the packed offset cache.
>
> * subversio/libsvn_fs_fs/caching.c
> (offset_serialize, offset_deserialize, dup_pack_offset): New.
> (svn_fs_fs__initialize_caches): Initialize the offset cache.
> ]]]
>
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 34464)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -1608,8 +1608,15 @@ get_packed_offset(apr_off_t *rev_offset,
> apr_file_t *manifest_file;
> char buf[PACK_MANIFEST_ENTRY_LEN + 1];
> apr_off_t rev_index_offset;
> + svn_boolean_t is_cached;
> apr_size_t len;
>
> + SVN_ERR(svn_cache__get((void **) &rev_offset, &is_cached,
> + ffd->packed_offset_cache, &rev, pool));
About to go to bed, so I won't try this out tonight, but here's my
guess (and if this doesn't work I'll get an fsfs-pack build going
tomorrow): You're passing in the address of rev_offset here, and then
here...
> +
> + if (is_cached)
> + return SVN_NO_ERROR;
> +
> /* Open the manifest file. */
> SVN_ERR(svn_io_file_open(&manifest_file, path_rev_packed(fs, rev, "manifest",
> pool),
> @@ -1628,7 +1635,7 @@ get_packed_offset(apr_off_t *rev_offset,
>
> *rev_offset = apr_atoi64(buf);
>
> - return SVN_NO_ERROR;
> + return svn_cache__set(ffd->packed_offset_cache, &rev, rev_offset, pool);
> }
>
> /* Open the revision file for revision REV in filesystem FS and store
> Index: subversion/libsvn_fs_fs/fs.h
> ===================================================================
> --- subversion/libsvn_fs_fs/fs.h (revision 34458)
> +++ subversion/libsvn_fs_fs/fs.h (working copy)
> @@ -223,6 +223,10 @@ typedef struct
> rep key to svn_string_t. */
> svn_cache__t *fulltext_cache;
>
> + /* Pack manifest cache; maps revision numbers to offsets in their respective
> + pack files. */
> + svn_cache__t *packed_offset_cache;
> +
> /* Data shared between all svn_fs_t objects for a given filesystem. */
> fs_fs_shared_data_t *shared;
>
> Index: subversion/libsvn_fs_fs/caching.c
> ===================================================================
> --- subversion/libsvn_fs_fs/caching.c (revision 34449)
> +++ subversion/libsvn_fs_fs/caching.c (working copy)
> @@ -104,6 +104,45 @@ dup_dir_listing(void **out,
> return SVN_NO_ERROR;
> }
>
> +
> +/** Caching packed rev offsets. **/
> +/* Implements svn_cache__serialize_func_t */
> +static svn_error_t *
> +offset_serialize(char **data,
> + apr_size_t *data_len,
> + void *in,
> + apr_pool_t *pool)
> +{
> + *data = apr_off_t_toa(pool, *((apr_off_t *) in));
> + *data_len = strlen(*data);
> + return SVN_NO_ERROR;
> +}
> +
> +/* Implements svn_cache__deserialize_func_t */
> +static svn_error_t *
> +offset_deserialize(void **out,
> + const char *data,
> + apr_size_t data_len,
> + apr_pool_t *pool)
> +{
> + *out = apr_palloc(pool, sizeof (apr_off_t));
> + apr_strtoff((apr_off_t *)*out, data, NULL, 0);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Implements svn_cache__dup_func_t */
> +static svn_error_t *
> +dup_pack_offset(void **out,
> + void *in,
> + apr_pool_t *pool)
> +{
> + *out = apr_palloc(pool, sizeof(apr_off_t));
You're changing *out, which is to say the contents of the variable
rev_offset (once you unpack a few layers of indirection, and assuming
we're in non-memcache mode). Which means that you're modifying the
arg-variable rev_offset, but not anything it points to, so this won't
escape from the function.
If instead you had a local apr_off_t *cached_rev_offset, and you
passed &cached_rev_offset to svn_cache__get, and then if it's found
you do a *rev_offset = *cached_rev_offset, then I think it should
work.
(Alternatively, you might be able to just store apr_off_t as the
values in the cache instead of pointers... I think in theory this is
illegal (casting a void * to and from an apr_off_t) but would work in
practice; somebody who's a better C lawyer than I might know the
answer. And speaking of C lawyerness, I think it isn't necessarily
legal to cast **'s around like you are:
http://c-faq.com/ptrs/genericpp.html If you look at the other
functions in caching.c, you'll note that through intermediate
variables and such I manage to avoid doing that.)
--dave
> + **((apr_off_t **) out) = *((apr_off_t *) in);
> + return SVN_NO_ERROR;
> +}
> +
> +
> /* Return a memcache in *MEMCACHE_P for FS if it's configured to use
> memcached, or NULL otherwise. Also, sets *FAIL_STOP to a boolean
> indicating whether cache errors should be returned to the caller or
> @@ -218,7 +257,26 @@ svn_fs_fs__initialize_caches(svn_fs_t *fs,
> SVN_ERR(svn_cache__set_error_handler(ffd->dir_cache,
> warn_on_cache_errors, fs, pool));
>
> + /* Only 16 bytes per entry (a revision number + the corresponding offset). */
> if (memcache)
> + SVN_ERR(svn_cache__create_memcache(&(ffd->packed_offset_cache),
> + memcache,
> + offset_serialize,
> + offset_deserialize,
> + sizeof(svn_revnum_t),
> + apr_pstrcat(pool, prefix, "PACK-OFFSET",
> + NULL),
> + fs->pool));
> + else
> + SVN_ERR(svn_cache__create_inprocess(&(ffd->packed_offset_cache),
> + dup_pack_offset, sizeof(svn_revnum_t),
> + 16, 1024, FALSE, fs->pool));
> +
> + if (! no_handler)
> + SVN_ERR(svn_cache__set_error_handler(ffd->packed_offset_cache,
> + warn_on_cache_errors, fs, pool));
> +
> + if (memcache)
> {
> SVN_ERR(svn_cache__create_memcache(&(ffd->fulltext_cache),
> memcache,
>
>
--
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
Received on 2008-11-29 07:44:45 CET