[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

[[[
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));
+
+ 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));
+ **((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,