You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2011/04/17 16:48:34 UTC
svn commit: r1094150 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.h fs_fs.c fs_fs.h tree.c
Author: stefan2
Date: Sun Apr 17 14:48:33 2011
New Revision: 1094150
URL: http://svn.apache.org/viewvc?rev=1094150&view=rev
Log:
Support concurrent transactions on FSFS: reset txn-local caches upon
closing the txn (i.e. don't wait until pool cleanup) and support concurrency
by simply disabling these caches permanently for the respective session
once a concurrency has been detected.
* subversion/libsvn_fs_fs/fs.h
(fs_fs_data_t): add concurrent_transactions member
(svn_fs_fs__reset_txn_caches): declare new private API function
* subversion/libsvn_fs_fs/caching.c
(txn_cleanup_baton_t): new utility struct
(remove_txn_cache): reset txn-local only if they match the ones to clean up
(init_txn_callbacks): new destructor init utility
(svn_fs_fs__initialize_txn_caches): gracefully support multiple concurrent txns
in the *same* FSFS session by permanently disabling txn-local caches
(svn_fs_fs__reset_txn_caches): implement new private API function
* subversion/libsvn_fs_fs/fs_fs.c
(purge_shared_txn_body): reset txn-local caches immediately at the end
of the given txn
* subversion/libsvn_fs_fs/tree.c
(svn_fs_fs__commit_txn): dito
Modified:
subversion/trunk/subversion/libsvn_fs_fs/caching.c
subversion/trunk/subversion/libsvn_fs_fs/fs.h
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
subversion/trunk/subversion/libsvn_fs_fs/tree.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1094150&r1=1094149&r2=1094150&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Sun Apr 17 14:48:33 2011
@@ -331,17 +331,51 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
return SVN_NO_ERROR;
}
+/* Baton to be used for the remove_txn_cache() pool cleanup function, */
+struct txn_cleanup_baton_t
+{
+ /* the cache to reset */
+ svn_cache__t *txn_cache;
+
+ /* the position where to reset it */
+ svn_cache__t **to_reset;
+};
+
/* APR pool cleanup handler that will reset the cache pointer given in
BATON_VOID. */
static apr_status_t
remove_txn_cache(void *baton_void)
{
- svn_cache__t **cache_p = baton_void;
- *cache_p = NULL;
+ struct txn_cleanup_baton_t *baton = baton_void;
+
+ /* be careful not to hurt performance by resetting newer txn's caches */
+ if (*baton->to_reset == baton->txn_cache)
+ *baton->to_reset = NULL;
return APR_SUCCESS;
}
+static svn_error_t *
+init_txn_callbacks(svn_cache__t **cache,
+ apr_pool_t *pool)
+{
+ if (cache != NULL)
+ {
+ struct txn_cleanup_baton_t *baton;
+
+ baton = apr_palloc(pool, sizeof(*baton));
+ baton->txn_cache = *cache;
+ baton->to_reset = cache;
+
+ apr_pool_cleanup_register(pool,
+ baton,
+ remove_txn_cache,
+ apr_pool_cleanup_null);
+ }
+
+ return SVN_NO_ERROR;
+}
+
svn_error_t *
svn_fs_fs__initialize_txn_caches(svn_fs_t *fs,
const char *txn_id,
@@ -361,9 +395,15 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
":", svn_uuid_generate(pool), ":",
(char *)NULL);
- /* There must be no concurrent transactions in progress for the same
- FSFS access / session object. Maybe, you forgot to clean POOL. */
- SVN_ERR_ASSERT(ffd->txn_dir_cache == NULL);
+ /* We don't support caching for concurrent transactions in the SAME
+ * FSFS session. Maybe, you forgot to clean POOL. */
+ if (ffd->txn_dir_cache != NULL || ffd->concurrent_transactions)
+ {
+ ffd->txn_dir_cache = NULL;
+ ffd->concurrent_transactions = TRUE;
+
+ return SVN_NO_ERROR;
+ }
/* create a txn-local directory cache */
if (svn_fs__get_global_membuffer_cache())
@@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
pool));
/* reset the transaction-specific cache if the pool gets cleaned up. */
- apr_pool_cleanup_register(pool,
- &(ffd->txn_dir_cache),
- remove_txn_cache,
- apr_pool_cleanup_null);
+ init_txn_callbacks(&(ffd->txn_dir_cache), pool);
return SVN_NO_ERROR;
}
+
+void
+svn_fs_fs__reset_txn_caches(svn_fs_t *fs)
+{
+ /* we can always just reset the caches. This may degrade performance but
+ * can never cause in incorrect behavior. */
+
+ fs_fs_data_t *ffd = fs->fsap_data;
+ ffd->txn_dir_cache = NULL;
+}
\ No newline at end of file
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.h?rev=1094150&r1=1094149&r2=1094150&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs.h Sun Apr 17 14:48:33 2011
@@ -248,6 +248,9 @@ typedef struct fs_fs_data_t
/* Cache for node_revision_t objects; the key is (revision, id offset) */
svn_cache__t *node_revision_cache;
+ /* If set, there are or have been more than one concurrent transaction */
+ svn_boolean_t concurrent_transactions;
+
/* Tempoary cache for changed directories yet to be committed; maps from
unparsed FS ID to ###x. NULL outside transactions. */
svn_cache__t *txn_dir_cache;
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1094150&r1=1094149&r2=1094150&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sun Apr 17 14:48:33 2011
@@ -853,6 +853,8 @@ purge_shared_txn_body(svn_fs_t *fs, cons
const char *txn_id = baton;
free_shared_txn(fs, txn_id);
+ svn_fs_fs__reset_txn_caches(fs);
+
return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h?rev=1094150&r1=1094149&r2=1094150&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h Sun Apr 17 14:48:33 2011
@@ -517,6 +517,11 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
const char *txn_id,
apr_pool_t *pool);
+/* Resets the svn_cache__t structures local to the current transaction in FS.
+ Calling it more than once per txn or from outside any txn is allowed. */
+void
+svn_fs_fs__reset_txn_caches(svn_fs_t *fs);
+
/* Possibly pack the repository at PATH. This just take full shards, and
combines all the revision files into a single one, with a manifest header.
Use optional CANCEL_FUNC/CANCEL_BATON for cancellation support.
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1094150&r1=1094149&r2=1094150&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Sun Apr 17 14:48:33 2011
@@ -1728,6 +1728,9 @@ svn_fs_fs__commit_txn(const char **confl
}
cleanup:
+
+ svn_fs_fs__reset_txn_caches(fs);
+
svn_pool_destroy(iterpool);
return svn_error_return(err);
}
Re: svn commit: r1094150 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.h fs_fs.c fs_fs.h tree.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 16.05.2011 12:37, Daniel Shahaf wrote:
> Stefan Fuhrmann wrote on Sun, May 15, 2011 at 18:17:22 +0200:
>> On 14.05.2011 03:54, Daniel Shahaf wrote:
>>> stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
>>>> Author: stefan2
>>>> Date: Sun Apr 17 14:48:33 2011
>>>> New Revision: 1094150
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1094150&view=rev
>>>> Log:
>>>> Support concurrent transactions on FSFS: reset txn-local caches upon
>>>> closing the txn (i.e. don't wait until pool cleanup) and support concurrency
>>>> by simply disabling these caches permanently for the respective session
>>>> once a concurrency has been detected.
>>>>
>>>> + struct txn_cleanup_baton_t *baton;
>>>> +
>>>> + baton = apr_palloc(pool, sizeof(*baton));
>>>> + baton->txn_cache = *cache;
>>>> + baton->to_reset = cache;
>>>> +
>>>> + apr_pool_cleanup_register(pool,
>>>> + baton,
>>>> + remove_txn_cache,
>>>> + apr_pool_cleanup_null);
>>>> + }
>>>> +
>>>> + return SVN_NO_ERROR;
>>>> +}
>>>> +
>>>> svn_error_t *
>>>> svn_fs_fs__initialize_txn_caches(svn_fs_t *fs,
>>>> const char *txn_id,
>>>> @@ -361,9 +395,15 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
>>>> ":", svn_uuid_generate(pool), ":",
>>>> (char *)NULL);
>>>>
>>>> - /* There must be no concurrent transactions in progress for the same
>>>> - FSFS access / session object. Maybe, you forgot to clean POOL. */
>>>> - SVN_ERR_ASSERT(ffd->txn_dir_cache == NULL);
>>>> + /* We don't support caching for concurrent transactions in the SAME
>>>> + * FSFS session. Maybe, you forgot to clean POOL. */
>>>> + if (ffd->txn_dir_cache != NULL || ffd->concurrent_transactions)
>>>> + {
>>>> + ffd->txn_dir_cache = NULL;
>>>> + ffd->concurrent_transactions = TRUE;
>>>> +
>>>> + return SVN_NO_ERROR;
>>>> + }
>>>>
>>> Why is this safe?
>>>
>>> Is it because svn_fs_fs__initialize_txn_caches() is called from all the
>>> relevant codepaths? (presumably the relevant codepaths are those that
>>> create txns)
>> Correct. The assumption that is being made here is that either
>>
>> * transactions won't use txn-local caches without previously
>> calling svn_fs_fs__initialize_txn_caches, or
>> * no two transactions are being run concurrently within the
>> same FSFS session.
>>
> Okay. There is only one code path that initializes txn's, so I'm not
> too worried about documenting this centrally...
>
> But: the call in make_txn_root() to svn_fs_fs__initialize_txn_caches()
> is missing an SVN_ERR() wrapper. Could you look into these please?
>
Done in r1104332.
-- Stefan^2.
Re: svn commit: r1094150 - in
/subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
fs_fs.h tree.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Sun, May 15, 2011 at 18:17:22 +0200:
> On 14.05.2011 03:54, Daniel Shahaf wrote:
>> stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
>>> Author: stefan2
>>> Date: Sun Apr 17 14:48:33 2011
>>> New Revision: 1094150
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1094150&view=rev
>>> Log:
>>> Support concurrent transactions on FSFS: reset txn-local caches upon
>>> closing the txn (i.e. don't wait until pool cleanup) and support concurrency
>>> by simply disabling these caches permanently for the respective session
>>> once a concurrency has been detected.
>>>
>>> + struct txn_cleanup_baton_t *baton;
>>> +
>>> + baton = apr_palloc(pool, sizeof(*baton));
>>> + baton->txn_cache = *cache;
>>> + baton->to_reset = cache;
>>> +
>>> + apr_pool_cleanup_register(pool,
>>> + baton,
>>> + remove_txn_cache,
>>> + apr_pool_cleanup_null);
>>> + }
>>> +
>>> + return SVN_NO_ERROR;
>>> +}
>>> +
>>> svn_error_t *
>>> svn_fs_fs__initialize_txn_caches(svn_fs_t *fs,
>>> const char *txn_id,
>>> @@ -361,9 +395,15 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
>>> ":", svn_uuid_generate(pool), ":",
>>> (char *)NULL);
>>>
>>> - /* There must be no concurrent transactions in progress for the same
>>> - FSFS access / session object. Maybe, you forgot to clean POOL. */
>>> - SVN_ERR_ASSERT(ffd->txn_dir_cache == NULL);
>>> + /* We don't support caching for concurrent transactions in the SAME
>>> + * FSFS session. Maybe, you forgot to clean POOL. */
>>> + if (ffd->txn_dir_cache != NULL || ffd->concurrent_transactions)
>>> + {
>>> + ffd->txn_dir_cache = NULL;
>>> + ffd->concurrent_transactions = TRUE;
>>> +
>>> + return SVN_NO_ERROR;
>>> + }
>>>
>> Why is this safe?
>>
>> Is it because svn_fs_fs__initialize_txn_caches() is called from all the
>> relevant codepaths? (presumably the relevant codepaths are those that
>> create txns)
> Correct. The assumption that is being made here is that either
>
> * transactions won't use txn-local caches without previously
> calling svn_fs_fs__initialize_txn_caches, or
> * no two transactions are being run concurrently within the
> same FSFS session.
>
Okay. There is only one code path that initializes txn's, so I'm not
too worried about documenting this centrally...
But: the call in make_txn_root() to svn_fs_fs__initialize_txn_caches()
is missing an SVN_ERR() wrapper. Could you look into these please?
(I've fixed *all* instances in the code a while back using vim+gcc ---
I'd be happy not to see more instances added :-))
> Otherwise, one txn might read from a different txn's caches.
> I say caches here for generality. Currently, there is at most
> one such cache.
>>> /* create a txn-local directory cache */
>>> if (svn_fs__get_global_membuffer_cache())
>>> @@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
>>> pool));
>>>
>>> /* reset the transaction-specific cache if the pool gets cleaned up. */
>>> - apr_pool_cleanup_register(pool,
>>> -&(ffd->txn_dir_cache),
>>> - remove_txn_cache,
>>> - apr_pool_cleanup_null);
>>> + init_txn_callbacks(&(ffd->txn_dir_cache), pool);
>>>
>>> return SVN_NO_ERROR;
>>> }
>>> +
>>> +void
>>> +svn_fs_fs__reset_txn_caches(svn_fs_t *fs)
>>> +{
>>> + /* we can always just reset the caches. This may degrade performance but
>>> + * can never cause in incorrect behavior. */
>>> +
>>> + fs_fs_data_t *ffd = fs->fsap_data;
>>> + ffd->txn_dir_cache = NULL;
>>> +}
>>> \ No newline at end of file
> Thanks for all the review!
>
Thanks for the fixes :)
> -- Stefan^2.
>
Re: svn commit: r1094150 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.h fs_fs.c fs_fs.h tree.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 14.05.2011 03:54, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
>> Author: stefan2
>> Date: Sun Apr 17 14:48:33 2011
>> New Revision: 1094150
>>
>> URL: http://svn.apache.org/viewvc?rev=1094150&view=rev
>> Log:
>> Support concurrent transactions on FSFS: reset txn-local caches upon
>> closing the txn (i.e. don't wait until pool cleanup) and support concurrency
>> by simply disabling these caches permanently for the respective session
>> once a concurrency has been detected.
>>
>> * subversion/libsvn_fs_fs/fs.h
>> (fs_fs_data_t): add concurrent_transactions member
>> (svn_fs_fs__reset_txn_caches): declare new private API function
>>
>> * subversion/libsvn_fs_fs/caching.c
>> (txn_cleanup_baton_t): new utility struct
>> (remove_txn_cache): reset txn-local only if they match the ones to clean up
>> (init_txn_callbacks): new destructor init utility
>> (svn_fs_fs__initialize_txn_caches): gracefully support multiple concurrent txns
>> in the *same* FSFS session by permanently disabling txn-local caches
>> (svn_fs_fs__reset_txn_caches): implement new private API function
>>
>> * subversion/libsvn_fs_fs/fs_fs.c
>> (purge_shared_txn_body): reset txn-local caches immediately at the end
>> of the given txn
>> * subversion/libsvn_fs_fs/tree.c
>> (svn_fs_fs__commit_txn): dito
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_fs_fs/caching.c
>> subversion/trunk/subversion/libsvn_fs_fs/fs.h
>> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
>> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>>
>> Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1094150&r1=1094149&r2=1094150&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Sun Apr 17 14:48:33 2011
>> @@ -331,17 +331,51 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
>> return SVN_NO_ERROR;
>> }
>>
>> +/* Baton to be used for the remove_txn_cache() pool cleanup function, */
>> +struct txn_cleanup_baton_t
>> +{
>> + /* the cache to reset */
>> + svn_cache__t *txn_cache;
>> +
>> + /* the position where to reset it */
>> + svn_cache__t **to_reset;
>> +};
>> +
>> /* APR pool cleanup handler that will reset the cache pointer given in
>> BATON_VOID. */
>> static apr_status_t
>> remove_txn_cache(void *baton_void)
>> {
>> - svn_cache__t **cache_p = baton_void;
>> - *cache_p = NULL;
>> + struct txn_cleanup_baton_t *baton = baton_void;
>> +
>> + /* be careful not to hurt performance by resetting newer txn's caches */
>> + if (*baton->to_reset == baton->txn_cache)
>> + *baton->to_reset = NULL;
>>
> If I understand correctly, this line actually has the same effect as
> svn_fs_fs__reset_txn_caches() --- namely, it zeroes out the
> FFD->txn_dir_cache member?
>
> Perhaps you could add a comment to save future readers a bit of
> mental double-pointer arithmetic?
r1103419 should make that a little more obvious.
>> return APR_SUCCESS;
>> }
>>
>> +static svn_error_t *
>> +init_txn_callbacks(svn_cache__t **cache,
>> + apr_pool_t *pool)
>> +{
>> + if (cache != NULL)
>> + {
> This condition will always be true. Did you mean 'if (*cache)' ?
Oops. Fixed in r1103422.
>> + struct txn_cleanup_baton_t *baton;
>> +
>> + baton = apr_palloc(pool, sizeof(*baton));
>> + baton->txn_cache = *cache;
>> + baton->to_reset = cache;
>> +
>> + apr_pool_cleanup_register(pool,
>> + baton,
>> + remove_txn_cache,
>> + apr_pool_cleanup_null);
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> svn_error_t *
>> svn_fs_fs__initialize_txn_caches(svn_fs_t *fs,
>> const char *txn_id,
>> @@ -361,9 +395,15 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
>> ":", svn_uuid_generate(pool), ":",
>> (char *)NULL);
>>
>> - /* There must be no concurrent transactions in progress for the same
>> - FSFS access / session object. Maybe, you forgot to clean POOL. */
>> - SVN_ERR_ASSERT(ffd->txn_dir_cache == NULL);
>> + /* We don't support caching for concurrent transactions in the SAME
>> + * FSFS session. Maybe, you forgot to clean POOL. */
>> + if (ffd->txn_dir_cache != NULL || ffd->concurrent_transactions)
>> + {
>> + ffd->txn_dir_cache = NULL;
>> + ffd->concurrent_transactions = TRUE;
>> +
>> + return SVN_NO_ERROR;
>> + }
>>
> Why is this safe?
>
> Is it because svn_fs_fs__initialize_txn_caches() is called from all the
> relevant codepaths? (presumably the relevant codepaths are those that
> create txns)
Correct. The assumption that is being made here is that either
* transactions won't use txn-local caches without previously
calling svn_fs_fs__initialize_txn_caches, or
* no two transactions are being run concurrently within the
same FSFS session.
Otherwise, one txn might read from a different txn's caches.
I say caches here for generality. Currently, there is at most
one such cache.
>> /* create a txn-local directory cache */
>> if (svn_fs__get_global_membuffer_cache())
>> @@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
>> pool));
>>
>> /* reset the transaction-specific cache if the pool gets cleaned up. */
>> - apr_pool_cleanup_register(pool,
>> -&(ffd->txn_dir_cache),
>> - remove_txn_cache,
>> - apr_pool_cleanup_null);
>> + init_txn_callbacks(&(ffd->txn_dir_cache), pool);
>>
>> return SVN_NO_ERROR;
>> }
>> +
>> +void
>> +svn_fs_fs__reset_txn_caches(svn_fs_t *fs)
>> +{
>> + /* we can always just reset the caches. This may degrade performance but
>> + * can never cause in incorrect behavior. */
>> +
>> + fs_fs_data_t *ffd = fs->fsap_data;
>> + ffd->txn_dir_cache = NULL;
>> +}
>> \ No newline at end of file
Thanks for all the review!
-- Stefan^2.
Re: svn commit: r1094150 - in
/subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
fs_fs.h tree.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
> Author: stefan2
> Date: Sun Apr 17 14:48:33 2011
> New Revision: 1094150
>
> URL: http://svn.apache.org/viewvc?rev=1094150&view=rev
> Log:
> Support concurrent transactions on FSFS: reset txn-local caches upon
> closing the txn (i.e. don't wait until pool cleanup) and support concurrency
> by simply disabling these caches permanently for the respective session
> once a concurrency has been detected.
>
> * subversion/libsvn_fs_fs/fs.h
> (fs_fs_data_t): add concurrent_transactions member
> (svn_fs_fs__reset_txn_caches): declare new private API function
>
> * subversion/libsvn_fs_fs/caching.c
> (txn_cleanup_baton_t): new utility struct
> (remove_txn_cache): reset txn-local only if they match the ones to clean up
> (init_txn_callbacks): new destructor init utility
> (svn_fs_fs__initialize_txn_caches): gracefully support multiple concurrent txns
> in the *same* FSFS session by permanently disabling txn-local caches
> (svn_fs_fs__reset_txn_caches): implement new private API function
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (purge_shared_txn_body): reset txn-local caches immediately at the end
> of the given txn
> * subversion/libsvn_fs_fs/tree.c
> (svn_fs_fs__commit_txn): dito
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/caching.c
> subversion/trunk/subversion/libsvn_fs_fs/fs.h
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1094150&r1=1094149&r2=1094150&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Sun Apr 17 14:48:33 2011
> @@ -331,17 +331,51 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> return SVN_NO_ERROR;
> }
>
> +/* Baton to be used for the remove_txn_cache() pool cleanup function, */
> +struct txn_cleanup_baton_t
> +{
> + /* the cache to reset */
> + svn_cache__t *txn_cache;
> +
> + /* the position where to reset it */
> + svn_cache__t **to_reset;
> +};
> +
> /* APR pool cleanup handler that will reset the cache pointer given in
> BATON_VOID. */
> static apr_status_t
> remove_txn_cache(void *baton_void)
> {
> - svn_cache__t **cache_p = baton_void;
> - *cache_p = NULL;
> + struct txn_cleanup_baton_t *baton = baton_void;
> +
> + /* be careful not to hurt performance by resetting newer txn's caches */
> + if (*baton->to_reset == baton->txn_cache)
> + *baton->to_reset = NULL;
>
If I understand correctly, this line actually has the same effect as
svn_fs_fs__reset_txn_caches() --- namely, it zeroes out the
FFD->txn_dir_cache member?
Perhaps you could add a comment to save future readers a bit of
mental double-pointer arithmetic?
> return APR_SUCCESS;
> }
>
> +static svn_error_t *
> +init_txn_callbacks(svn_cache__t **cache,
> + apr_pool_t *pool)
> +{
> + if (cache != NULL)
> + {
This condition will always be true. Did you mean 'if (*cache)' ?
> + struct txn_cleanup_baton_t *baton;
> +
> + baton = apr_palloc(pool, sizeof(*baton));
> + baton->txn_cache = *cache;
> + baton->to_reset = cache;
> +
> + apr_pool_cleanup_register(pool,
> + baton,
> + remove_txn_cache,
> + apr_pool_cleanup_null);
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_fs_fs__initialize_txn_caches(svn_fs_t *fs,
> const char *txn_id,
> @@ -361,9 +395,15 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
> ":", svn_uuid_generate(pool), ":",
> (char *)NULL);
>
> - /* There must be no concurrent transactions in progress for the same
> - FSFS access / session object. Maybe, you forgot to clean POOL. */
> - SVN_ERR_ASSERT(ffd->txn_dir_cache == NULL);
> + /* We don't support caching for concurrent transactions in the SAME
> + * FSFS session. Maybe, you forgot to clean POOL. */
> + if (ffd->txn_dir_cache != NULL || ffd->concurrent_transactions)
> + {
> + ffd->txn_dir_cache = NULL;
> + ffd->concurrent_transactions = TRUE;
> +
> + return SVN_NO_ERROR;
> + }
>
Why is this safe?
Is it because svn_fs_fs__initialize_txn_caches() is called from all the
relevant codepaths? (presumably the relevant codepaths are those that
create txns)
> /* create a txn-local directory cache */
> if (svn_fs__get_global_membuffer_cache())
> @@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
> pool));
>
> /* reset the transaction-specific cache if the pool gets cleaned up. */
> - apr_pool_cleanup_register(pool,
> - &(ffd->txn_dir_cache),
> - remove_txn_cache,
> - apr_pool_cleanup_null);
> + init_txn_callbacks(&(ffd->txn_dir_cache), pool);
>
> return SVN_NO_ERROR;
> }
> +
> +void
> +svn_fs_fs__reset_txn_caches(svn_fs_t *fs)
> +{
> + /* we can always just reset the caches. This may degrade performance but
> + * can never cause in incorrect behavior. */
> +
> + fs_fs_data_t *ffd = fs->fsap_data;
> + ffd->txn_dir_cache = NULL;
> +}
> \ No newline at end of file
Re: svn commit: r1094150 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.h fs_fs.c fs_fs.h tree.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 14.05.2011 03:45, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
>> +static svn_error_t *
>> +init_txn_callbacks(svn_cache__t **cache,
>> + apr_pool_t *pool)
>> +{
> ...
>> @@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
>> pool));
>>
>> /* reset the transaction-specific cache if the pool gets cleaned up. */
>> - apr_pool_cleanup_register(pool,
>> -&(ffd->txn_dir_cache),
>> - remove_txn_cache,
>> - apr_pool_cleanup_null);
>> + init_txn_callbacks(&(ffd->txn_dir_cache), pool);
> Error leak.
Thanks for the feedback.
In r1103413, I decided that init_txn_callbacks
should simply return void.
-- Stefan^2.
Re: svn commit: r1094150 - in
/subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
fs_fs.h tree.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
> +static svn_error_t *
> +init_txn_callbacks(svn_cache__t **cache,
> + apr_pool_t *pool)
> +{
...
> @@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
> pool));
>
> /* reset the transaction-specific cache if the pool gets cleaned up. */
> - apr_pool_cleanup_register(pool,
> - &(ffd->txn_dir_cache),
> - remove_txn_cache,
> - apr_pool_cleanup_null);
> + init_txn_callbacks(&(ffd->txn_dir_cache), pool);
Error leak.
Re: svn commit: r1094150 - in
/subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
fs_fs.h tree.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
> Author: stefan2
> Date: Sun Apr 17 14:48:33 2011
> New Revision: 1094150
>
> URL: http://svn.apache.org/viewvc?rev=1094150&view=rev
> Log:
> Support concurrent transactions on FSFS: reset txn-local caches upon
> closing the txn (i.e. don't wait until pool cleanup) and support concurrency
> by simply disabling these caches permanently for the respective session
> once a concurrency has been detected.
>
> * subversion/libsvn_fs_fs/fs.h
> (fs_fs_data_t): add concurrent_transactions member
> (svn_fs_fs__reset_txn_caches): declare new private API function
>
> * subversion/libsvn_fs_fs/caching.c
> (txn_cleanup_baton_t): new utility struct
> (remove_txn_cache): reset txn-local only if they match the ones to clean up
> (init_txn_callbacks): new destructor init utility
> (svn_fs_fs__initialize_txn_caches): gracefully support multiple concurrent txns
> in the *same* FSFS session by permanently disabling txn-local caches
> (svn_fs_fs__reset_txn_caches): implement new private API function
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (purge_shared_txn_body): reset txn-local caches immediately at the end
> of the given txn
> * subversion/libsvn_fs_fs/tree.c
> (svn_fs_fs__commit_txn): dito
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/caching.c
> subversion/trunk/subversion/libsvn_fs_fs/fs.h
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1094150&r1=1094149&r2=1094150&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Sun Apr 17 14:48:33 2011
> @@ -331,17 +331,51 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
> return SVN_NO_ERROR;
> }
>
> +/* Baton to be used for the remove_txn_cache() pool cleanup function, */
> +struct txn_cleanup_baton_t
> +{
> + /* the cache to reset */
> + svn_cache__t *txn_cache;
> +
> + /* the position where to reset it */
> + svn_cache__t **to_reset;
> +};
> +
> /* APR pool cleanup handler that will reset the cache pointer given in
> BATON_VOID. */
> static apr_status_t
> remove_txn_cache(void *baton_void)
> {
> - svn_cache__t **cache_p = baton_void;
> - *cache_p = NULL;
> + struct txn_cleanup_baton_t *baton = baton_void;
> +
> + /* be careful not to hurt performance by resetting newer txn's caches */
> + if (*baton->to_reset == baton->txn_cache)
> + *baton->to_reset = NULL;
>
If I understand correctly, this line actually has the same effect as
svn_fs_fs__reset_txn_caches() --- namely, it zeroes out the
FFD->txn_dir_cache member?
Perhaps you could add a comment to save future readers a bit of
mental double-pointer arithmetic?
> return APR_SUCCESS;
> }
>
> +static svn_error_t *
> +init_txn_callbacks(svn_cache__t **cache,
> + apr_pool_t *pool)
> +{
> + if (cache != NULL)
> + {
This condition will always be true. Did you mean 'if (*cache)' ?
> + struct txn_cleanup_baton_t *baton;
> +
> + baton = apr_palloc(pool, sizeof(*baton));
> + baton->txn_cache = *cache;
> + baton->to_reset = cache;
> +
> + apr_pool_cleanup_register(pool,
> + baton,
> + remove_txn_cache,
> + apr_pool_cleanup_null);
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_fs_fs__initialize_txn_caches(svn_fs_t *fs,
> const char *txn_id,
> @@ -361,9 +395,15 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
> ":", svn_uuid_generate(pool), ":",
> (char *)NULL);
>
> - /* There must be no concurrent transactions in progress for the same
> - FSFS access / session object. Maybe, you forgot to clean POOL. */
> - SVN_ERR_ASSERT(ffd->txn_dir_cache == NULL);
> + /* We don't support caching for concurrent transactions in the SAME
> + * FSFS session. Maybe, you forgot to clean POOL. */
> + if (ffd->txn_dir_cache != NULL || ffd->concurrent_transactions)
> + {
> + ffd->txn_dir_cache = NULL;
> + ffd->concurrent_transactions = TRUE;
> +
> + return SVN_NO_ERROR;
> + }
>
Why is this safe?
Is it because svn_fs_fs__initialize_txn_caches() is called from all the
relevant codepaths? (presumably the relevant codepaths are those that
create txns)
> /* create a txn-local directory cache */
> if (svn_fs__get_global_membuffer_cache())
> @@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
> pool));
>
> /* reset the transaction-specific cache if the pool gets cleaned up. */
> - apr_pool_cleanup_register(pool,
> - &(ffd->txn_dir_cache),
> - remove_txn_cache,
> - apr_pool_cleanup_null);
> + init_txn_callbacks(&(ffd->txn_dir_cache), pool);
>
> return SVN_NO_ERROR;
> }
> +
> +void
> +svn_fs_fs__reset_txn_caches(svn_fs_t *fs)
> +{
> + /* we can always just reset the caches. This may degrade performance but
> + * can never cause in incorrect behavior. */
> +
> + fs_fs_data_t *ffd = fs->fsap_data;
> + ffd->txn_dir_cache = NULL;
> +}
> \ No newline at end of file
Re: svn commit: r1094150 - in
/subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.h fs_fs.c
fs_fs.h tree.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@apache.org wrote on Sun, Apr 17, 2011 at 14:48:34 -0000:
> +static svn_error_t *
> +init_txn_callbacks(svn_cache__t **cache,
> + apr_pool_t *pool)
> +{
...
> @@ -386,10 +426,17 @@ svn_fs_fs__initialize_txn_caches(svn_fs_
> pool));
>
> /* reset the transaction-specific cache if the pool gets cleaned up. */
> - apr_pool_cleanup_register(pool,
> - &(ffd->txn_dir_cache),
> - remove_txn_cache,
> - apr_pool_cleanup_null);
> + init_txn_callbacks(&(ffd->txn_dir_cache), pool);
Error leak.