Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@subversion.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1220388 - in
 /subversion/branches/file-handle-cache/subversion:
 include/private/svn_file_handle_cache.h libsvn_fs_fs/fs_fs.c
 libsvn_subr/svn_file_handle_cache.c
Date: Sun, 18 Dec 2011 12:56:16 -0000
To: commits@subversion.apache.org
From: stefan2@apache.org
Message-Id: <20111218125616.A681223888E7@eris.apache.org>

Author: stefan2
Date: Sun Dec 18 12:56:16 2011
New Revision: 1220388

URL: http://svn.apache.org/viewvc?rev=1220388&view=rev
Log:
On file_handle_cache branch: Simplify the file handle cache API.
No longer require cookies, perms and flags. The first will be replaced by a smarter
internal file handle selection logic and the latter will be hard-coded since this cache
is only useful for files with buffered read.

* subversion/include/private/svn_file_handle_cache.h
  (svn_file_handle_cache__open): drop parameters from API declaration
* subversion/libsvn_subr/svn_file_handle_cache.c
  adapt global commentary
  (cache_entry_t): remove now unused element from cache entry
  (internal_file_open): drop the same 3 parameters
  (are_siblings): new utility function
  (svn_file_handle_cache__open_internal): prefer opening a small number
   of file handles for the same file over moving file pointers back and forth
   and reading the same data many times.
  (svn_file_handle_cache__open): adapt pass-through function

* subversion/libsvn_fs_fs/fs_fs.c
  (DEFAULT_FILE_COOKIE, REP_FILE_COOKIE): no longer needed
  (open_pack_or_rev_file, open_and_seek_revision, open_and_seek_transaction):
   remove cookie from signature; adapt API and function calls
  (get_node_revision_body, svn_fs_fs__rev_get_root, svn_fs_fs__paths_changed,
   recover_get_largest_revision, recover_body): adapt API and function calls

Modified:
    subversion/branches/file-handle-cache/subversion/include/private/svn_file_handle_cache.h
    subversion/branches/file-handle-cache/subversion/libsvn_fs_fs/fs_fs.c
    subversion/branches/file-handle-cache/subversion/libsvn_subr/svn_file_handle_cache.c

Modified: subversion/branches/file-handle-cache/subversion/include/private/svn_file_handle_cache.h
URL: http://svn.apache.org/viewvc/subversion/branches/file-handle-cache/subversion/include/private/svn_file_handle_cache.h?rev=1220388&r1=1220387&r2=1220388&view=diff
==============================================================================
--- subversion/branches/file-handle-cache/subversion/include/private/svn_file_handle_cache.h (original)
+++ subversion/branches/file-handle-cache/subversion/include/private/svn_file_handle_cache.h Sun Dec 18 12:56:16 2011
@@ -53,24 +53,15 @@ typedef
 struct svn_file_handle_cache__handle_t svn_file_handle_cache__handle_t;
 
 /**
- * Get an open file handle in @a f, for the file named @a fname with the
- * open flag(s) in @a flag and permissions in @a perm. These parameters
- * are the same as in @ref svn_io_file_open. The file pointer will be
- * moved to the specified @a offset, if it is different from -1.
- *
- * If there are one or more unused matching open file handles, those with
- * the specified @a cookie will be preferred. This is particularly useful
- * if @a offset is -1, i.e. if the file pointer position of the handle
- * returned is undefined.
+ * Get an open buffered read file handle in @a f, for the file named
+ * @a fname. The file pointer will be moved to the specified @a offset,
+ * if it is different from -1.
  */
 svn_error_t *
 svn_file_handle_cache__open(svn_file_handle_cache__handle_t **f,
                             svn_file_handle_cache_t *cache,
                             const char *fname,
-                            apr_int32_t flag,
-                            apr_fileperms_t perm,
                             apr_off_t offset,
-                            int cookie,
                             apr_pool_t *pool);
 
 /**

Modified: subversion/branches/file-handle-cache/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/branches/file-handle-cache/subversion/libsvn_fs_fs/fs_fs.c?rev=1220388&r1=1220387&r2=1220388&view=diff
==============================================================================
--- subversion/branches/file-handle-cache/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/branches/file-handle-cache/subversion/libsvn_fs_fs/fs_fs.c Sun Dec 18 12:56:16 2011
@@ -114,17 +114,6 @@
 #define REP_PLAIN          "PLAIN"
 #define REP_DELTA          "DELTA"
 
-/* Cookies used to classify cached file handle usage */
-/* Used whenever no other specific region of the rev file is being read. */
-#define DEFAULT_FILE_COOKIE 0
-
-/* Used when reading representation data.
- * Since this is often interleaved with other reads, use a separate 
- * cookie (hence a separate file handle) for the reps.  That way, rep
- * access can often be satisfied from the APR read buffer.  The same
- * applies to the meta data because it is not rep data. */
-#define REP_FILE_COOKIE     1
-
 /* Notes:
 
 To avoid opening and closing the rev-files all the time, it would
@@ -1792,8 +1781,7 @@ ensure_revision_exists(svn_fs_t *fs,
    been packed, *FILE will be set to the packed file; otherwise, set *FILE
    to the revision file for REV.  Return SVN_ERR_FS_NO_SUCH_REVISION if the
    file doesn't exist.  Move the file pointer of OFFSET, if the latter is
-   not -1.  Prefer cached file handles that share the same COOKIE (again,
-   if not -1).
+   not -1.
 
    TODO: Consider returning an indication of whether this is a packed rev
          file, so the caller need not rely on is_packed_rev() which in turn
@@ -1806,7 +1794,6 @@ open_pack_or_rev_file(svn_file_handle_ca
                       svn_fs_t *fs,
                       svn_revnum_t rev,
                       apr_off_t offset,
-                      int cookie,
                       apr_pool_t *pool)
 {
   fs_fs_data_t *ffd = fs->fsap_data;
@@ -1823,10 +1810,7 @@ open_pack_or_rev_file(svn_file_handle_ca
         err = svn_file_handle_cache__open(file,
                                           ffd->file_handle_cache,
                                           path,
-                                          APR_READ | APR_BUFFERED,
-                                          APR_OS_DEFAULT,
                                           offset,
-                                          cookie,
                                           pool);
 
       if (err && APR_STATUS_IS_ENOENT(err->apr_err)
@@ -1934,7 +1918,6 @@ open_and_seek_revision(svn_file_handle_c
                        svn_fs_t *fs,
                        svn_revnum_t rev,
                        apr_off_t offset,
-                       int cookie,
                        apr_pool_t *pool)
 {
   /* none of the following requires the file handle */
@@ -1948,7 +1931,7 @@ open_and_seek_revision(svn_file_handle_c
     }
 
   /* So, open the revision file and position the pointer here in one go. */
-  return open_pack_or_rev_file(file, fs, rev, offset, cookie, pool);
+  return open_pack_or_rev_file(file, fs, rev, offset, pool);
 }
 
 /* Open the representation for a node-revision in transaction TXN_ID
@@ -1962,7 +1945,6 @@ open_and_seek_transaction(svn_file_handl
                           svn_fs_t *fs,
                           const char *txn_id,
                           representation_t *rep,
-                          int cookie,
                           apr_pool_t *pool)
 {
   fs_fs_data_t *ffd = fs->fsap_data;
@@ -1971,10 +1953,7 @@ open_and_seek_transaction(svn_file_handl
   return svn_file_handle_cache__open(file,
                                      ffd->file_handle_cache,
                                      path_txn_proto_rev(fs, txn_id, pool),
-                                     APR_READ | APR_BUFFERED, 
-                                     APR_OS_DEFAULT, 
                                      rep->offset,
-                                     cookie,
                                      pool);
 }
 
@@ -1992,10 +1971,9 @@ open_and_seek_representation(svn_file_ha
    * buffer effectiveness. */
   if (! rep->txn_id)
     return open_and_seek_revision(file_p, fs, rep->revision, rep->offset,
-                                  REP_FILE_COOKIE, pool);
+                                  pool);
   else
-    return open_and_seek_transaction(file_p, fs, rep->txn_id, rep, 
-                                     REP_FILE_COOKIE, pool);
+    return open_and_seek_transaction(file_p, fs, rep->txn_id, rep, pool);
 }
 
 /* Parse the description of a representation from STRING and store it
@@ -2218,10 +2196,7 @@ get_node_revision_body(node_revision_t *
       err = svn_file_handle_cache__open(&revision_file,
                                         ffd->file_handle_cache,
                                         path_txn_node_rev(fs, id, pool),
-                                        APR_READ | APR_BUFFERED,
-                                        APR_OS_DEFAULT,
                                         0,
-                                        DEFAULT_FILE_COOKIE,
                                         pool);
     }
   else
@@ -2230,7 +2205,6 @@ get_node_revision_body(node_revision_t *
       err = open_and_seek_revision(&revision_file, fs,
                                    svn_fs_fs__id_rev(id),
                                    svn_fs_fs__id_offset(id),
-                                   DEFAULT_FILE_COOKIE,
                                    pool);
     }
 
@@ -2928,8 +2902,7 @@ svn_fs_fs__rev_get_root(svn_fs_id_t **ro
     return SVN_NO_ERROR;
 
   /* we don't care about the file pointer position */
-  SVN_ERR(open_pack_or_rev_file(&revision_file, fs, rev, -1,
-                                DEFAULT_FILE_COOKIE, pool));
+  SVN_ERR(open_pack_or_rev_file(&revision_file, fs, rev, -1,pool));
   apr_rev_file = svn_file_handle_cache__get_apr_handle(revision_file);
 
   /* here, it will get moved anyways */
@@ -4681,8 +4654,7 @@ svn_fs_fs__paths_changed(apr_hash_t **ch
   SVN_ERR(ensure_revision_exists(fs, rev, pool));
 
   /* we don't care about the file pointer position */
-  SVN_ERR(open_pack_or_rev_file(&revision_file, fs, rev, -1, 
-                                DEFAULT_FILE_COOKIE, pool));
+  SVN_ERR(open_pack_or_rev_file(&revision_file, fs, rev, -1, pool));
   apr_revision_file = svn_file_handle_cache__get_apr_handle(revision_file);
 
   /* here, it will get moved anyways */
@@ -6778,8 +6750,7 @@ recover_get_largest_revision(svn_fs_t *f
 
       /* We don't care about the file pointer position as long as the file 
          itself exists. */
-      err = open_pack_or_rev_file(&file, fs, right, -1, 
-                                  DEFAULT_FILE_COOKIE, iterpool);
+      err = open_pack_or_rev_file(&file, fs, right, -1, iterpool);
       svn_pool_clear(iterpool);
 
       if (err && err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
@@ -6804,8 +6775,7 @@ recover_get_largest_revision(svn_fs_t *f
       svn_file_handle_cache__handle_t *file;
 
       /* Again, ignore the file pointer position. */
-      err = open_pack_or_rev_file(&file, fs, probe, -1, 
-                                  DEFAULT_FILE_COOKIE, iterpool);
+      err = open_pack_or_rev_file(&file, fs, probe, -1, iterpool);
       svn_pool_clear(iterpool);
 
       if (err && err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
@@ -7095,8 +7065,7 @@ recover_body(void *baton, apr_pool_t *po
             SVN_ERR(b->cancel_func(b->cancel_baton));
 
           /* Any file pointer position will do ... */
-          SVN_ERR(open_pack_or_rev_file(&rev_file, fs, rev, -1,
-                                        DEFAULT_FILE_COOKIE, iterpool));
+          SVN_ERR(open_pack_or_rev_file(&rev_file, fs, rev, -1, iterpool));
           apr_rev_file = svn_file_handle_cache__get_apr_handle(rev_file);
 
           /* ... because it gets set here explicitly */

Modified: subversion/branches/file-handle-cache/subversion/libsvn_subr/svn_file_handle_cache.c
URL: http://svn.apache.org/viewvc/subversion/branches/file-handle-cache/subversion/libsvn_subr/svn_file_handle_cache.c?rev=1220388&r1=1220387&r2=1220388&view=diff
==============================================================================
--- subversion/branches/file-handle-cache/subversion/libsvn_subr/svn_file_handle_cache.c (original)
+++ subversion/branches/file-handle-cache/subversion/libsvn_subr/svn_file_handle_cache.c Sun Dec 18 12:56:16 2011
@@ -55,8 +55,7 @@
  * the current file pointer gets (optionally) compared with the location
  * of the next access. If a cached file is found that may already have the
  * desired data in its buffer, that handle will be returned instead of a
- * random one. When the position of the next file access is not known,
- * a cookie may be used to discern files in a similar way.
+ * random one.
  *
  * For read-after-write scenarios, it is imperative to flush the APR file
  * buffer before attempting to read that file. Therefore, all idle handles
@@ -117,7 +116,8 @@ struct entry_list_t
  * so we can reuse the memory by clearing the pool.
  *
  * The cache entry is linked in three lists:
- * - the global list of either used or unused entry (having no file handle)
+ * - the global list of either used or unused entries
+ *   (unused ones are those having no APR file handle)
  * - list of sibblings, i.e. file handles to the same file
  * - global LRU list of idle file handles, i.e. those that are currently
  *   not used by the application
@@ -140,15 +140,6 @@ struct cache_entry_t
   /* the file name. NULL for unused entries */
   const char *name;
 
-  /* file open flag(s). Valid only for used entries. */
-  apr_int32_t flag;
-
-  /* granted file permissions. Valid only for used entries. */
-  apr_fileperms_t perm;
-
-  /* granted file permissions. Valid only for used entries. */
-  int cookie;
-
   /* position of the file pointer. Valid only for idle entries. */
   apr_off_t position;
 
@@ -356,17 +347,13 @@ auto_close_cached_handle(void *entry_voi
   return APR_SUCCESS;
 }
 
-/* Create a new APR-level file handle with the specified file NAME, FLAG
- * and permissions PERM. A COOKIE will be attached to it. The corresponding
- * cache item will be returned in RESULT.
+/* Create a new APR-level file handle with the specified file NAME in CACHE.
+ * The corresponding cache entry will be returned in RESULT.
  */
 static svn_error_t *
 internal_file_open(cache_entry_t **result,
                    svn_file_handle_cache_t *cache,
-                   const char *name,
-                   apr_int32_t flag,
-                   apr_fileperms_t perm,
-                   int cookie)
+                   const char *name)
 {
   cache_entry_t *entry;
   cache_entry_t *sibling;
@@ -395,7 +382,8 @@ internal_file_open(cache_entry_t **resul
     }
 
   /* (try to) open the requested file */
-  SVN_ERR(svn_io_file_open(&entry->file, name, flag, perm, entry->pool));
+  SVN_ERR(svn_io_file_open(&entry->file, name, APR_READ | APR_BUFFERED, 
+                           APR_OS_DEFAULT, entry->pool));
   assert(entry->file);
 
   /* make sure we auto-close cached file handles held by the application
@@ -406,9 +394,6 @@ internal_file_open(cache_entry_t **resul
                             apr_pool_cleanup_null);
 
   /* set file info */
-  entry->flag = flag;
-  entry->perm = perm;
-  entry->cookie = cookie;
   entry->name = apr_pstrdup(entry->pool, name);
   entry->position = 0;
 
@@ -608,6 +593,14 @@ pointer_is_closer(const cache_entry_t *e
   return old_delta > new_delta ? TRUE : FALSE;
 }
 
+/* Returns true if LHS and RHS refer to the same file.
+ */
+static svn_boolean_t
+are_siblings(const cache_entry_t *lhs, const cache_entry_t *rhs)
+{
+  return (lhs == rhs) || !strcmp(lhs->name, rhs->name);
+}
+
 /* Set file pointer of ENTRY->file to OFFSET. As an optimization, make sure
  * that a few hundred bytes before that OFFSET are also pre-fetched as SVN
  * tends to read data "backwards".
@@ -635,63 +628,94 @@ aligned_seek(cache_entry_t *entry, apr_o
  * flag(s) in FLAG and permissions in PERM. These parameters are the same
  * as in svn_io_file_open(). The file pointer will be moved to the specified
  * OFFSET, if it is different from -1.
- *
- * If the COOKIE is not -1, only those file handles will be considerd a match
- * that have been opened with that cookie before. This is particularly useful
- * if OFFSET is -1, i.e. if the file pointer position of the handle returned
- * is undefined.
  */
 static svn_error_t *
 svn_file_handle_cache__open_internal
    (svn_file_handle_cache__handle_t **f,
     svn_file_handle_cache_t *cache,
     const char *fname,
-    apr_int32_t flag,
-    apr_fileperms_t perm,
     apr_off_t offset,
-    int cookie,
     apr_pool_t *pool)
 {
-  svn_error_t *err = SVN_NO_ERROR;
   cache_entry_t *entry;
-  cache_entry_t *next_entry;
+  cache_entry_t *first_entry;
   cache_entry_t *near_entry = NULL;
-  cache_entry_t *cookie_entry = NULL;
+  cache_entry_t *any_entry = NULL;
+  cache_entry_t *last_entry = NULL;
   cache_entry_t *entry_found = NULL;
+ 
+  int idle_entry_count = 0;
 
   /* look through all idle entries for this filename and find suitable ones */
-  for (entry = find_first(cache, fname); entry; entry = next_entry)
+  first_entry = find_first(cache, fname);
+  for ( entry = first_entry
+      ; entry
+      ; entry = get_next_entry (&entry->sibling_link))
     {
-      next_entry = get_next_entry (&entry->sibling_link);
+      last_entry = entry;
+      assert(entry->file != NULL);
+      
       if (! entry->open_handle)
         {
-          /* The entry is idle. Is it suitable? */
-          if (entry->flag == flag && entry->perm == perm)
+          idle_entry_count++;
+          
+          if (! any_entry)
+            any_entry = entry;
+
+          /* is it a particularly close match? */
+          if (pointer_is_closer(entry, offset, near_entry))
+            near_entry = entry;
+        }
+    }
+
+  /* select the most suitable idle file handle */
+  if (near_entry)
+    {
+      /* best option: a file whose buffer propably already contains
+       * the data that we are looking for. */
+      entry_found = near_entry;
+    }
+  else if (any_entry)
+    {
+      /* Re-using an open file is also a good idea.
+       * 
+       * However, it may be better to open packed files a few more times
+       * since later we are likely to read data later close to the current
+       * location. Keep the number open handles / file reasonably low.
+       */      
+      if (   (idle_entry_count >= 4) 
+          || (   (cache->unused_entries.count == 0)
+              /* auto-closing a suitable file doesn't make sense */
+              && (are_siblings(cache->idle_entries.first->item, any_entry))))
+        {
+          /* Ensure that any_entry will be the last one to be re-used in
+           * successive calls: put it at the end of the siblings list for
+           * this file name.
+           */
+          if (last_entry != any_entry)
             {
-              /* we can use this entry, if the cookie matches */
-              if (entry->cookie == cookie && cookie != -1)
-                {
-                  cookie_entry = entry;
-
-                  /* is it a particularly close match? */
-                  if (pointer_is_closer(entry, offset, near_entry))
-                    near_entry = entry;
+              if (any_entry == first_entry)
+                { 
+                  first_entry = get_next_entry(&any_entry->sibling_link);
+                  assert(first_entry->file != NULL);
+                  apr_hash_set(cache->first_by_name,
+                               any_entry->name,
+                               APR_HASH_KEY_STRING,
+                               NULL);
+                  apr_hash_set(cache->first_by_name,
+                               first_entry->name,
+                               APR_HASH_KEY_STRING,
+                               first_entry);
                 }
+                
+              unlink_link(&any_entry->sibling_link);
+              link_link(&any_entry->sibling_link, &last_entry->sibling_link);
             }
-          else
-            {
-              /* the old file handle has been opened with different flags,
-               * e.g. this is "read" after "write" or vice versa. Therefore,
-               * close the underlying APR handle.
-               */
-              internal_close_file(cache, entry);
-            }
+
+           entry_found = any_entry;
         }
     }
-
-  /* the best match we found */
-  entry_found = near_entry ? near_entry : cookie_entry;
-
+    
   if (entry_found)
     {
       /* we can use an idle entry */
@@ -729,20 +753,14 @@ svn_error_t *
 svn_file_handle_cache__open(svn_file_handle_cache__handle_t **f,
                             svn_file_handle_cache_t *cache,
                             const char *fname,
-                            apr_int32_t flag,
-                            apr_fileperms_t perm,
                             apr_off_t offset,
-                            int cookie,
                             apr_pool_t *pool)
 {
   SVN_MUTEX__WITH_LOCK(cache->mutex,
                        svn_file_handle_cache__open_internal(f,
                                                             cache,
                                                             fname,
-                                                            flag,
-                                                            perm,
                                                             offset,
-                                                            cookie,
                                                             pool));
 
   return SVN_NO_ERROR;