You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@mail.utexas.edu> on 2008/11/29 04:03:58 UTC
[PATCH] Pack revision offset caching
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
Re: [PATCH] Pack revision offset caching
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
David Glasser wrote:
> On Fri, Nov 28, 2008 at 11:03 PM, Hyrum K. Wright
> <hy...@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.)
It looks like this was the problem. I've committed the updated patch in r34473.
Thanks for the review!
-Hyrum
Re: [PATCH] Pack revision offset caching
Posted by David Glasser <gl...@davidglasser.net>.
On Fri, Nov 28, 2008 at 11:03 PM, Hyrum K. Wright
<hy...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org