You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/03/26 06:31:39 UTC
Re: svn commit: r1081097 - in /subversion/trunk/subversion:
include/private/svn_cache.h libsvn_fs_fs/caching.c libsvn_fs_fs/tree.c
libsvn_subr/cache-inprocess.c tests/libsvn_subr/cache-test.c
stefan2@apache.org wrote on Sun, Mar 13, 2011 at 12:40:50 -0000:
> Author: stefan2
> Date: Sun Mar 13 12:40:49 2011
> New Revision: 1081097
>
> URL: http://svn.apache.org/viewvc?rev=1081097&view=rev
> Log:
> Prepare for the introduction of a generic cache statistics API:
> make cache-inprocess caches identifiable.
>
> * subversion/include/private/svn_cache.h
> (svn_cache__create_inprocess): add id parameter
> * subversion/libsvn_subr/cache-inprocess.c
> (inprocess_cache_t): add id member
> (svn_cache__create_inprocess): store id parameter
> * subversion/libsvn_fs_fs/caching.c
> (init_callbacks): new function; factored out from the init function
> (svn_fs_fs__initialize_caches): provide IDs / prefixes to all cache types,
> call new init sub-function
> * subversion/libsvn_fs_fs/tree.c
> (make_txn_root): adapt to internal API change
> * subversion/tests/libsvn_subr/cache-test.c
> (test_inprocess_cache_basic): dito
>
> Modified:
> subversion/trunk/subversion/include/private/svn_cache.h
> subversion/trunk/subversion/libsvn_fs_fs/caching.c
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
> subversion/trunk/subversion/libsvn_subr/cache-inprocess.c
> subversion/trunk/subversion/tests/libsvn_subr/cache-test.c
>
> Modified: subversion/trunk/subversion/include/private/svn_cache.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_cache.h?rev=1081097&r1=1081096&r2=1081097&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/private/svn_cache.h (original)
> +++ subversion/trunk/subversion/include/private/svn_cache.h Sun Mar 13 12:40:49 2011
> @@ -136,6 +136,7 @@ svn_cache__create_inprocess(svn_cache__t
> apr_int64_t pages,
> apr_int64_t items_per_page,
> svn_boolean_t thread_safe,
> + const char *id,
Need to document the ID parameter please...
> apr_pool_t *pool);
> /**
> * Creates a new cache in @a *cache_p, communicating to a memcached
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1081097&r1=1081096&r2=1081097&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Sun Mar 13 12:40:49 2011
> @@ -64,6 +64,24 @@ warn_on_cache_errors(svn_error_t *err,
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +init_callbacks(svn_cache__t *cache,
> + svn_fs_t *fs,
> + svn_boolean_t no_handler,
> + apr_pool_t *pool)
> +{
> + if (cache != NULL)
> + {
> + if (! no_handler)
> + SVN_ERR(svn_cache__set_error_handler(cache,
> + warn_on_cache_errors,
> + fs,
> + pool));
> +
> + }
> +
> + return SVN_NO_ERROR;
> +}
>
> svn_error_t *
> svn_fs_fs__initialize_caches(svn_fs_t *fs,
> @@ -101,11 +119,12 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> svn_fs_fs__serialize_id,
> svn_fs_fs__deserialize_id,
> sizeof(svn_revnum_t),
> - 1, 100, FALSE, fs->pool));
> - if (! no_handler)
> - SVN_ERR(svn_cache__set_error_handler(ffd->rev_root_id_cache,
> - warn_on_cache_errors, fs, pool));
> + 1, 100, FALSE,
> + apr_pstrcat(pool, prefix, "RRI",
> + (char *)NULL),
> + fs->pool));
>
> + SVN_ERR(init_callbacks(ffd->rev_root_id_cache, fs, no_handler, pool));
>
> /* Rough estimate: revision DAG nodes have size around 320 bytes, so
> * let's put 16 on a page. */
> @@ -123,11 +142,12 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> svn_fs_fs__dag_serialize,
> svn_fs_fs__dag_deserialize,
> APR_HASH_KEY_STRING,
> - 1024, 16, FALSE, fs->pool));
> - if (! no_handler)
> - SVN_ERR(svn_cache__set_error_handler(ffd->rev_node_cache,
> - warn_on_cache_errors, fs, pool));
> + 1024, 16, FALSE,
> + apr_pstrcat(pool, prefix, "DAG",
> + (char *)NULL),
> + fs->pool));
>
> + SVN_ERR(init_callbacks(ffd->rev_node_cache, fs, no_handler, pool));
>
> /* Very rough estimate: 1K per directory. */
> if (svn_fs__get_global_membuffer_cache())
> @@ -144,11 +164,12 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> svn_fs_fs__serialize_dir_entries,
> svn_fs_fs__deserialize_dir_entries,
> APR_HASH_KEY_STRING,
> - 1024, 8, FALSE, fs->pool));
> + 1024, 8, FALSE,
> + apr_pstrcat(pool, prefix, "DIR",
> + (char *)NULL),
> + fs->pool));
>
> - if (! no_handler)
> - SVN_ERR(svn_cache__set_error_handler(ffd->dir_cache,
> - warn_on_cache_errors, fs, pool));
> + SVN_ERR(init_callbacks(ffd->dir_cache, fs, no_handler, pool));
>
> /* Only 16 bytes per entry (a revision number + the corresponding offset).
> Since we want ~8k pages, that means 512 entries per page. */
> @@ -166,11 +187,12 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> svn_fs_fs__serialize_manifest,
> svn_fs_fs__deserialize_manifest,
> sizeof(svn_revnum_t),
> - 32, 1, FALSE, fs->pool));
> + 32, 1, FALSE,
> + apr_pstrcat(pool, prefix, "PACK-MANIFEST",
> + (char *)NULL),
> + fs->pool));
>
> - if (! no_handler)
> - SVN_ERR(svn_cache__set_error_handler(ffd->packed_offset_cache,
> - warn_on_cache_errors, fs, pool));
> + SVN_ERR(init_callbacks(ffd->packed_offset_cache, fs, no_handler, pool));
>
> /* initialize fulltext cache as configured */
> if (memcache)
> @@ -201,9 +223,7 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> ffd->fulltext_cache = NULL;
> }
>
> - if (ffd->fulltext_cache && ! no_handler)
> - SVN_ERR(svn_cache__set_error_handler(ffd->fulltext_cache,
> - warn_on_cache_errors, fs, pool));
> + SVN_ERR(init_callbacks(ffd->fulltext_cache, fs, no_handler, pool));
>
> /* initialize txdelta window cache, if that has been enabled */
> if (svn_fs__get_global_membuffer_cache() &&
> @@ -223,9 +243,7 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> ffd->txdelta_window_cache = NULL;
> }
>
> - if (ffd->txdelta_window_cache && ! no_handler)
> - SVN_ERR(svn_cache__set_error_handler(ffd->txdelta_window_cache,
> - warn_on_cache_errors, fs, pool));
> + SVN_ERR(init_callbacks(ffd->txdelta_window_cache, fs, no_handler, pool));
>
> /* initialize node revision cache, if caching has been enabled */
> if (svn_fs__get_global_membuffer_cache())
> @@ -246,9 +264,7 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> ffd->node_revision_cache = NULL;
> }
>
> - if (ffd->node_revision_cache && ! no_handler)
> - SVN_ERR(svn_cache__set_error_handler(ffd->node_revision_cache,
> - warn_on_cache_errors, fs, pool));
> + SVN_ERR(init_callbacks(ffd->node_revision_cache, fs, no_handler, pool));
>
> return SVN_NO_ERROR;
> }
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1081097&r1=1081096&r2=1081097&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Mar 13 12:40:49 2011
> @@ -3916,7 +3916,9 @@ make_txn_root(svn_fs_root_t **root_p,
> svn_fs_fs__dag_serialize,
> svn_fs_fs__dag_deserialize,
> APR_HASH_KEY_STRING,
> - 32, 20, FALSE, root->pool));
> + 32, 20, FALSE,
> + apr_pstrcat(pool, txn, ":TXN", (char *)NULL),
This doesn't use namespacing...
So, beyond the theoretical problem, doesn't it mean that a single
application that creates two repositories and begins a txn against each
repository will mix the two txn's caches? (since txn names are
predictable)
IOW: shouldn't this use the PREFIX argument that caching.c uses?
> + root->pool));
>
> root->fsap_data = frd;
>
>
> Modified: subversion/trunk/subversion/tests/libsvn_subr/cache-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/cache-test.c?rev=1081097&r1=1081096&r2=1081097&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_subr/cache-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_subr/cache-test.c Sun Mar 13 12:40:49 2011
> @@ -136,6 +136,7 @@ test_inprocess_cache_basic(apr_pool_t *p
> 1,
> 1,
> TRUE,
> + "",
> pool));
>
Same problem... (I imagine we might want to use "" in the cache
infrastructure internally some day, for example)
> return basic_cache_test(cache, TRUE, pool);
>
>
Re: svn commit: r1081097 - in /subversion/trunk/subversion:
include/private/svn_cache.h libsvn_fs_fs/caching.c libsvn_fs_fs/tree.c
libsvn_subr/cache-inprocess.c tests/libsvn_subr/cache-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sun, Apr 03, 2011 at 20:00:28 +0200:
> On 26.03.2011 06:31, Daniel Shahaf wrote:
> >stefan2@apache.org wrote on Sun, Mar 13, 2011 at 12:40:50 -0000:
> >>URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1081097&r1=1081096&r2=1081097&view=diff
> >>==============================================================================
> >>--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> >>+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Mar 13 12:40:49 2011
> >>@@ -3916,7 +3916,9 @@ make_txn_root(svn_fs_root_t **root_p,
> >> svn_fs_fs__dag_serialize,
> >> svn_fs_fs__dag_deserialize,
> >> APR_HASH_KEY_STRING,
> >>- 32, 20, FALSE, root->pool));
> >>+ 32, 20, FALSE,
> >>+ apr_pstrcat(pool, txn, ":TXN", (char *)NULL),
> >This doesn't use namespacing...
> >
> Name spaces are only required for membuffer caches.
> This is a good ol' inprocess cache that will not share
> cached information with other cache instances.
>
If I understand correctly, in other words it's because membuffer caches
are a singleton but "inprocess" caches are not --- and therefore, the ID
needs to be unique only within the scope of this cache, rather than
globally.
Thanks for the explanation,
Daniel
> >So, beyond the theoretical problem, doesn't it mean that a single
> >application that creates two repositories and begins a txn against each
> >repository will mix the two txn's caches? (since txn names are
> >predictable)
> >
> >IOW: shouldn't this use the PREFIX argument that caching.c uses?
> Unless somebody reads the statistics info for this
> cache instance, the id won't be used at all. If someone
> should call svn_cache__get_info on that info for
> profiling etc., the transaction name plus the ":TXN"
> marker should be sufficient to identify the data
> in a log, for instance.
> >>+ root->pool));
> >>
> >> root->fsap_data = frd;
> >>
> >>
> >>Modified: subversion/trunk/subversion/tests/libsvn_subr/cache-test.c
> >>URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/cache-test.c?rev=1081097&r1=1081096&r2=1081097&view=diff
> >>==============================================================================
> >>--- subversion/trunk/subversion/tests/libsvn_subr/cache-test.c (original)
> >>+++ subversion/trunk/subversion/tests/libsvn_subr/cache-test.c Sun Mar 13 12:40:49 2011
> >>@@ -136,6 +136,7 @@ test_inprocess_cache_basic(apr_pool_t *p
> >> 1,
> >> 1,
> >> TRUE,
> >>+ "",
> >> pool));
> >>
> >Same problem... (I imagine we might want to use "" in the cache
> >infrastructure internally some day, for example)
> This is also a cache_inprocess instance. So, we don't need
> a proper ID / prefix here for proper function.
>
> -- Stefan^2.
>
Re: svn commit: r1081097 - in /subversion/trunk/subversion: include/private/svn_cache.h
libsvn_fs_fs/caching.c libsvn_fs_fs/tree.c libsvn_subr/cache-inprocess.c
tests/libsvn_subr/cache-test.c
Posted by Stefan Fuhrmann <eq...@web.de>.
On 26.03.2011 06:31, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Sun, Mar 13, 2011 at 12:40:50 -0000:
>> Author: stefan2
>> Date: Sun Mar 13 12:40:49 2011
>> New Revision: 1081097
>>
>> URL: http://svn.apache.org/viewvc?rev=1081097&view=rev
>> Log:
>> Prepare for the introduction of a generic cache statistics API:
>> make cache-inprocess caches identifiable.
>>
>> * subversion/include/private/svn_cache.h
>> (svn_cache__create_inprocess): add id parameter
>> * subversion/libsvn_subr/cache-inprocess.c
>> (inprocess_cache_t): add id member
>> (svn_cache__create_inprocess): store id parameter
>> * subversion/libsvn_fs_fs/caching.c
>> (init_callbacks): new function; factored out from the init function
>> (svn_fs_fs__initialize_caches): provide IDs / prefixes to all cache types,
>> call new init sub-function
>> * subversion/libsvn_fs_fs/tree.c
>> (make_txn_root): adapt to internal API change
>> * subversion/tests/libsvn_subr/cache-test.c
>> (test_inprocess_cache_basic): dito
>>
>> Modified:
>> subversion/trunk/subversion/include/private/svn_cache.h
>> subversion/trunk/subversion/libsvn_fs_fs/caching.c
>> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>> subversion/trunk/subversion/libsvn_subr/cache-inprocess.c
>> subversion/trunk/subversion/tests/libsvn_subr/cache-test.c
>>
>> Modified: subversion/trunk/subversion/include/private/svn_cache.h
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_cache.h?rev=1081097&r1=1081096&r2=1081097&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/include/private/svn_cache.h (original)
>> +++ subversion/trunk/subversion/include/private/svn_cache.h Sun Mar 13 12:40:49 2011
>> @@ -136,6 +136,7 @@ svn_cache__create_inprocess(svn_cache__t
>> apr_int64_t pages,
>> apr_int64_t items_per_page,
>> svn_boolean_t thread_safe,
>> + const char *id,
> Need to document the ID parameter please...
Thanks for finding that!
Fixed in r1088351.
>
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1081097&r1=1081096&r2=1081097&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Mar 13 12:40:49 2011
>> @@ -3916,7 +3916,9 @@ make_txn_root(svn_fs_root_t **root_p,
>> svn_fs_fs__dag_serialize,
>> svn_fs_fs__dag_deserialize,
>> APR_HASH_KEY_STRING,
>> - 32, 20, FALSE, root->pool));
>> + 32, 20, FALSE,
>> + apr_pstrcat(pool, txn, ":TXN", (char *)NULL),
> This doesn't use namespacing...
>
Name spaces are only required for membuffer caches.
This is a good ol' inprocess cache that will not share
cached information with other cache instances.
> So, beyond the theoretical problem, doesn't it mean that a single
> application that creates two repositories and begins a txn against each
> repository will mix the two txn's caches? (since txn names are
> predictable)
>
> IOW: shouldn't this use the PREFIX argument that caching.c uses?
Unless somebody reads the statistics info for this
cache instance, the id won't be used at all. If someone
should call svn_cache__get_info on that info for
profiling etc., the transaction name plus the ":TXN"
marker should be sufficient to identify the data
in a log, for instance.
>> + root->pool));
>>
>> root->fsap_data = frd;
>>
>>
>> Modified: subversion/trunk/subversion/tests/libsvn_subr/cache-test.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/cache-test.c?rev=1081097&r1=1081096&r2=1081097&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/tests/libsvn_subr/cache-test.c (original)
>> +++ subversion/trunk/subversion/tests/libsvn_subr/cache-test.c Sun Mar 13 12:40:49 2011
>> @@ -136,6 +136,7 @@ test_inprocess_cache_basic(apr_pool_t *p
>> 1,
>> 1,
>> TRUE,
>> + "",
>> pool));
>>
> Same problem... (I imagine we might want to use "" in the cache
> infrastructure internally some day, for example)
This is also a cache_inprocess instance. So, we don't need
a proper ID / prefix here for proper function.
-- Stefan^2.