On 08/02/2010 01:51 PM, stef...@apache.org wrote:
Author: stefan2
Date: Mon Aug  2 20:51:35 2010
New Revision: 981684

URL: http://svn.apache.org/viewvc?rev=981684&view=rev
Log:
Bring the membuffer cache to its first use for the full text cache.
Also, provide functions to get / set the FSFS cache configuration
although not all of it is supported, yet.


Stefan,

I appreciate you working on this, I'm looking forward to using these improvements in production, which is why I'm closely looking at them.

+/* Access to the cache settings as a process-wide singleton. */
+static svn_fs_fs__cache_config_t *
+internal_get_cache_config(void)
+{
+  static svn_fs_fs__cache_config_t settings =
+    {
+      /* default configuration:
+       */
+      0x8000000,   /* 128 MB for caches */
+      16,          /* up to 16 files kept open */
+      FALSE,       /* don't cache fulltexts */
+      FALSE,       /* don't cache text deltas */
+      FALSE        /* assume multi-threaded operation */
+    };
+
+  return&settings;
+}


Why not make "settings" static at file scope instead of inside this function? What does this function provide for us?



+/* Access the process-global (singleton) membuffer cache. The first call
+ * will automatically allocate the cache using the current cache config.
+ * NULL will be returned if the desired cache size is 0.
+ */
+static svn_membuffer_t *
+get_global_membuffer_cache(void)
+{
+  static svn_membuffer_t *cache = NULL;
+
+  apr_uint64_t cache_size = svn_fs_fs__get_cache_config()->cache_size;
+  if (!cache&&  cache_size)

s/cache&&/cache &&/

+    {
+      /* auto-allocate cache*/
+      apr_allocator_t *allocator = NULL;
+      apr_pool_t *pool = NULL;
+
+      if (apr_allocator_create(&allocator))
+        return NULL;
+
+      /* ensure that a we free partially allocated data if we run OOM


s/a we/we/


+       * before the cache is complete.
+       */
+      apr_allocator_max_free_set(allocator, 1);
+      pool = svn_pool_create_ex(NULL, allocator);
+
+      svn_cache__membuffer_cache_create
+          (&cache,
+           cache_size,
+           cache_size / 16,
+           ! svn_fs_fs__get_cache_config()->single_threaded,
+           pool);
+    }

So the allocator is associated with this pool forever then? So it cannot be destroyed after

-    ffd->fulltext_cache = NULL;
+    {
+      ffd->fulltext_cache = NULL;
+    }
+
+  if (ffd->fulltext_cache&&  ! no_handler)

s/fulltext_cache&&  ! no_handler/fulltext_cache && ! no_handler

+    SVN_ERR(svn_cache__set_error_handler(ffd->fulltext_cache,
+            warn_on_cache_errors, fs, pool));

    return SVN_NO_ERROR;
  }

Modified: subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h
URL: 
http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h?rev=981684&r1=981683&r2=981684&view=diff
==============================================================================
--- subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h (original)
+++ subversion/branches/performance/subversion/libsvn_fs_fs/fs_fs.h Mon Aug  2 
20:51:35 2010
@@ -523,6 +523,39 @@ svn_fs_fs__get_node_origin(const svn_fs_
  svn_error_t *
  svn_fs_fs__initialize_caches(svn_fs_t *fs, apr_pool_t *pool);

+/* FSFS cache settings. It controls what caches, in what size and
+   how they will be created. The settomgs apply for the whole process.

s/settomgs/settings/

+ */
+typedef struct svn_fs_fs__cache_config_t
+{
+  /* total cache size in bytes. May be 0, resulting in no cache being used */
+  apr_uint64_t cache_size;
+
+  /* maximum number of files kept open */
+  apr_size_t file_handle_count;
+
+  /* shall fulltexts be cached? */
+  svn_boolean_t cache_fulltexts;
+
+  /* shall text deltas be cached? */
+  svn_boolean_t cache_txdeltas;
+
+  /* is this a guaranteed single-threaded application? */
+  svn_boolean_t single_threaded;
+} svn_fs_fs__cache_config_t;
+
+/* Get the current FSFS cache configuration. If it has not been set,
+   yet, this function will return the default settings.


s/yet,//


+ */
+const svn_fs_fs__cache_config_t *
+svn_fs_fs__get_cache_config(void);
+
+/* Set the FSFS cache configuration. Please note that it may not change
+   the actual configuration *in use*. Therefore, call it before reading
+   data from any FSFS repo and call it only once.

Should also document that this is not multi-threaded safe and does not protect against races, just as you do at the functions implementation.

+ */
+void
+svn_fs_fs__set_cache_config(svn_fs_fs__cache_config_t *settings);

Instead of

  svn_fs_fs__cache_config_t *settings

make it const:

  const svn_fs_fs__cache_config_t *settings

Regards,
Blair

Reply via email to