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 2010/11/09 22:37:56 UTC
Re: svn commit: r1033040 -
/subversion/branches/performance/subversion/libsvn_fs_util/caching.c
stefan2@apache.org wrote on Tue, Nov 09, 2010 at 15:51:54 -0000:
> Author: stefan2
> Date: Tue Nov 9 15:51:54 2010
> New Revision: 1033040
>
> URL: http://svn.apache.org/viewvc?rev=1033040&view=rev
> Log:
> Fix error leaks in cache creation code. Also, having a NULL
> file handle cache is not allowed (it is only allowed for the
> membuffer cache).
>
> * subversion/libsvn_fs_util/caching.c
> (svn_fs__get_global_membuffer_cache): fix error leak
> (svn_fs__get_global_file_handle_cache): prevent PFs later by exiting
> the process immediately upon cache creation failure
>
> Suggested by: stsp
>
> Modified:
> subversion/branches/performance/subversion/libsvn_fs_util/caching.c
>
> Modified: subversion/branches/performance/subversion/libsvn_fs_util/caching.c
> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_fs_util/caching.c?rev=1033040&r1=1033039&r2=1033040&view=diff
> ==============================================================================
> --- subversion/branches/performance/subversion/libsvn_fs_util/caching.c (original)
> +++ subversion/branches/performance/subversion/libsvn_fs_util/caching.c Tue Nov 9 15:51:54 2010
> @@ -49,7 +49,8 @@ svn_fs_get_cache_config(void)
>
> /* 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.
> + * NULL will be returned if the desired cache size is 0 or if the cache
> + * could not be created for some reason.
> */
> svn_membuffer_t *
> svn_fs__get_global_membuffer_cache(void)
> @@ -75,12 +76,12 @@ svn_fs__get_global_membuffer_cache(void)
> apr_allocator_max_free_set(allocator, 1);
> pool = svn_pool_create_ex(NULL, allocator);
>
> - svn_cache__membuffer_cache_create
> - (&cache,
> - (apr_size_t)cache_size,
> - (apr_size_t)(cache_size / 16),
> - ! svn_fs_get_cache_config()->single_threaded,
> - pool);
> + svn_error_clear(svn_cache__membuffer_cache_create(
> + &cache,
> + (apr_size_t)cache_size,
> + (apr_size_t)(cache_size / 16),
> + ! svn_fs_get_cache_config()->single_threaded,
> + pool));
> }
>
> return cache;
> @@ -96,10 +97,23 @@ svn_fs__get_global_file_handle_cache(voi
> static svn_file_handle_cache_t *cache = NULL;
>
> if (!cache)
> - svn_file_handle_cache__create_cache(&cache,
> - cache_settings.file_handle_count,
> - !cache_settings.single_threaded,
> - svn_pool_create(NULL));
> + {
> + svn_error_t* err = svn_file_handle_cache__create_cache(
> + &cache,
> + cache_settings.file_handle_count,
> + !cache_settings.single_threaded,
> + svn_pool_create(NULL));
> + if (err)
> + {
> + svn_error_clear(err);
> +
> + /* We need the file handle cache. The only way that an error could
> + * occur would be some threading error. In that case, there is no
> + * way we could continue - not even in some limp home mode.
> + */
> + SVN_ERR_MALFUNCTION_NO_RETURN();
Haven't looked at it closely, but:
It seems a shame to lose ERR here. Could we (better) pass it to the
malfunction handler, or (at least) log it to the FS warning function?
> + }
> + }
>
> return cache;
> }
>
>
BTW, what's the status of the performance branch? Last I check, its
STATUS File was empty...
Re: performance branch; review of r1033040;
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 10.11.2010 00:47, Daniel Shahaf wrote:
> Stefan Fuhrmann wrote on Wed, Nov 10, 2010 at 00:32:14 +0100:
>> On 09.11.2010 23:37, Daniel Shahaf wrote:
>>> stefan2@apache.org wrote on Tue, Nov 09, 2010 at 15:51:54 -0000:
>>>> + {
>>>> + svn_error_t* err = svn_file_handle_cache__create_cache(&cache,
>>>> + cache_settings.file_handle_count,
>>>> + !cache_settings.single_threaded,
>>>> + svn_pool_create(NULL));
>>>> + if (err)
>>>> + {
>>>> + svn_error_clear(err);
>>>> +
>>>> + /* We need the file handle cache. The only way that an error could
>>>> + * occur would be some threading error. In that case, there is no
>>>> + * way we could continue - not even in some limp home mode.
>>>> + */
>>>> + SVN_ERR_MALFUNCTION_NO_RETURN();
>>> Haven't looked at it closely, but:
>>>
>>> It seems a shame to lose ERR here. Could we (better) pass it to the
>>> malfunction handler, or (at least) log it to the FS warning function?
>>>
>> Yes, losing the error is somewhat unsatisfying. Basically,
>> we had to duplicate SVN_ERR_ASSERT_NO_RETURN
>> to set the #expr parameter of svn_error__malfunction().
>>
> Actually I was thinking to pass a proper svn_error_t object. That means
> revving the malfunction handler and the ASSERT/MALFUNCTION macros to add
> an svn_error_t * parameter.
>
> For this particular caller, though, having an apr_status_t (aka apr_err)
> parameter to the malfunction handlers would suffice.
>
>> Since svn_file_handle_cache__create_cache() gets called
>> by svn_fs_set_cache_config(), we don't have a FS context
>> (not even in the caller) that we could use to dump ERR.
> I see :-(
r1037588 now logs the message from ERR before aborting.
>>>> + }
>>>> + }
>>>>
>>>> return cache;
>>>> }
>>>>
>>>>
>>> BTW, what's the status of the performance branch? Last I check, its
>>> STATUS File was empty...
>>>
>> Next item to review is the integrate-cache-membuffer branch.
> Is this for 1.7?
>
Yes, because it is relatively simple: it simply replaces
memcached with a new implementation and uses that
for fulltext caches only. If it should cause problems, it
can simply disabled in caching.c even shortly before
the 1.7 release.
And it certainly has the largest impact on overall performance.
Finally, it is the basis for most other optimizations.
>> Once that passed the review, large parts of the remaining stuff
>> could be reviewed and merged again using the STATUS file.
>> Some changes may require a further integration branch, e.g.
>> my extensions to the stream interface.
>>
>> The file handle cache should not be part of SVN 1.7 due to the
>> high risk of side-effects.
>>
> Okay.
>
>> Before I select more revisions for the review, I want go through
>> my mail and fix the issues that you and others found.
>>
> Cool. I thought there might be a few more low-hanging fruits --- e.g.,
> the svn_txdelta_* and svn_io_file_read_full2() work --- that can be
> merged sooner.
The stream handling improvements could be factored
out for review into a separate integration branch. The
code in question got reworked more than once, so a
review on the cumulative diff makes more sense than
reviewing all rejected intermediate steps.
-- Stefan^2.
performance branch; review of r1033040;
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Wed, Nov 10, 2010 at 00:32:14 +0100:
> On 09.11.2010 23:37, Daniel Shahaf wrote:
>> stefan2@apache.org wrote on Tue, Nov 09, 2010 at 15:51:54 -0000:
>>> + {
>>> + svn_error_t* err = svn_file_handle_cache__create_cache(&cache,
>>> + cache_settings.file_handle_count,
>>> + !cache_settings.single_threaded,
>>> + svn_pool_create(NULL));
>>> + if (err)
>>> + {
>>> + svn_error_clear(err);
>>> +
>>> + /* We need the file handle cache. The only way that an error could
>>> + * occur would be some threading error. In that case, there is no
>>> + * way we could continue - not even in some limp home mode.
>>> + */
>>> + SVN_ERR_MALFUNCTION_NO_RETURN();
>> Haven't looked at it closely, but:
>>
>> It seems a shame to lose ERR here. Could we (better) pass it to the
>> malfunction handler, or (at least) log it to the FS warning function?
>>
> Yes, losing the error is somewhat unsatisfying. Basically,
> we had to duplicate SVN_ERR_ASSERT_NO_RETURN
> to set the #expr parameter of svn_error__malfunction().
>
Actually I was thinking to pass a proper svn_error_t object. That means
revving the malfunction handler and the ASSERT/MALFUNCTION macros to add
an svn_error_t * parameter.
For this particular caller, though, having an apr_status_t (aka apr_err)
parameter to the malfunction handlers would suffice.
> Since svn_file_handle_cache__create_cache() gets called
> by svn_fs_set_cache_config(), we don't have a FS context
> (not even in the caller) that we could use to dump ERR.
I see :-(
>>> + }
>>> + }
>>>
>>> return cache;
>>> }
>>>
>>>
>> BTW, what's the status of the performance branch? Last I check, its
>> STATUS File was empty...
>>
> Next item to review is the integrate-cache-membuffer branch.
Is this for 1.7?
> Once that passed the review, large parts of the remaining stuff
> could be reviewed and merged again using the STATUS file.
> Some changes may require a further integration branch, e.g.
> my extensions to the stream interface.
>
> The file handle cache should not be part of SVN 1.7 due to the
> high risk of side-effects.
>
Okay.
> Before I select more revisions for the review, I want go through
> my mail and fix the issues that you and others found.
>
Cool. I thought there might be a few more low-hanging fruits --- e.g.,
the svn_txdelta_* and svn_io_file_read_full2() work --- that can be
merged sooner.
> -- Stefan^2.
>
performance branch; review of r1033040;
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Wed, Nov 10, 2010 at 00:32:14 +0100:
> On 09.11.2010 23:37, Daniel Shahaf wrote:
>> stefan2@apache.org wrote on Tue, Nov 09, 2010 at 15:51:54 -0000:
>>> + {
>>> + svn_error_t* err = svn_file_handle_cache__create_cache(&cache,
>>> + cache_settings.file_handle_count,
>>> + !cache_settings.single_threaded,
>>> + svn_pool_create(NULL));
>>> + if (err)
>>> + {
>>> + svn_error_clear(err);
>>> +
>>> + /* We need the file handle cache. The only way that an error could
>>> + * occur would be some threading error. In that case, there is no
>>> + * way we could continue - not even in some limp home mode.
>>> + */
>>> + SVN_ERR_MALFUNCTION_NO_RETURN();
>> Haven't looked at it closely, but:
>>
>> It seems a shame to lose ERR here. Could we (better) pass it to the
>> malfunction handler, or (at least) log it to the FS warning function?
>>
> Yes, losing the error is somewhat unsatisfying. Basically,
> we had to duplicate SVN_ERR_ASSERT_NO_RETURN
> to set the #expr parameter of svn_error__malfunction().
>
Actually I was thinking to pass a proper svn_error_t object. That means
revving the malfunction handler and the ASSERT/MALFUNCTION macros to add
an svn_error_t * parameter.
For this particular caller, though, having an apr_status_t (aka apr_err)
parameter to the malfunction handlers would suffice.
> Since svn_file_handle_cache__create_cache() gets called
> by svn_fs_set_cache_config(), we don't have a FS context
> (not even in the caller) that we could use to dump ERR.
I see :-(
>>> + }
>>> + }
>>>
>>> return cache;
>>> }
>>>
>>>
>> BTW, what's the status of the performance branch? Last I check, its
>> STATUS File was empty...
>>
> Next item to review is the integrate-cache-membuffer branch.
Is this for 1.7?
> Once that passed the review, large parts of the remaining stuff
> could be reviewed and merged again using the STATUS file.
> Some changes may require a further integration branch, e.g.
> my extensions to the stream interface.
>
> The file handle cache should not be part of SVN 1.7 due to the
> high risk of side-effects.
>
Okay.
> Before I select more revisions for the review, I want go through
> my mail and fix the issues that you and others found.
>
Cool. I thought there might be a few more low-hanging fruits --- e.g.,
the svn_txdelta_* and svn_io_file_read_full2() work --- that can be
merged sooner.
> -- Stefan^2.
>
Re: svn commit: r1033040 - /subversion/branches/performance/subversion/libsvn_fs_util/caching.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 09.11.2010 23:37, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Tue, Nov 09, 2010 at 15:51:54 -0000:
>> Author: stefan2
>> Date: Tue Nov 9 15:51:54 2010
>> New Revision: 1033040
>>
>> URL: http://svn.apache.org/viewvc?rev=1033040&view=rev
>> Log:
>> Fix error leaks in cache creation code. Also, having a NULL
>> file handle cache is not allowed (it is only allowed for the
>> membuffer cache).
>>
>> * subversion/libsvn_fs_util/caching.c
>> (svn_fs__get_global_membuffer_cache): fix error leak
>> (svn_fs__get_global_file_handle_cache): prevent PFs later by exiting
>> the process immediately upon cache creation failure
>>
>> Suggested by: stsp
>>
>> Modified:
>> subversion/branches/performance/subversion/libsvn_fs_util/caching.c
>>
>> Modified: subversion/branches/performance/subversion/libsvn_fs_util/caching.c
>> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_fs_util/caching.c?rev=1033040&r1=1033039&r2=1033040&view=diff
>> ==============================================================================
>> --- subversion/branches/performance/subversion/libsvn_fs_util/caching.c (original)
>> +++ subversion/branches/performance/subversion/libsvn_fs_util/caching.c Tue Nov 9 15:51:54 2010
>> @@ -49,7 +49,8 @@ svn_fs_get_cache_config(void)
>>
>> /* 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.
>> + * NULL will be returned if the desired cache size is 0 or if the cache
>> + * could not be created for some reason.
>> */
>> svn_membuffer_t *
>> svn_fs__get_global_membuffer_cache(void)
>> @@ -75,12 +76,12 @@ svn_fs__get_global_membuffer_cache(void)
>> apr_allocator_max_free_set(allocator, 1);
>> pool = svn_pool_create_ex(NULL, allocator);
>>
>> - svn_cache__membuffer_cache_create
>> - (&cache,
>> - (apr_size_t)cache_size,
>> - (apr_size_t)(cache_size / 16),
>> - ! svn_fs_get_cache_config()->single_threaded,
>> - pool);
>> + svn_error_clear(svn_cache__membuffer_cache_create(
>> +&cache,
>> + (apr_size_t)cache_size,
>> + (apr_size_t)(cache_size / 16),
>> + ! svn_fs_get_cache_config()->single_threaded,
>> + pool));
>> }
>>
>> return cache;
>> @@ -96,10 +97,23 @@ svn_fs__get_global_file_handle_cache(voi
>> static svn_file_handle_cache_t *cache = NULL;
>>
>> if (!cache)
>> - svn_file_handle_cache__create_cache(&cache,
>> - cache_settings.file_handle_count,
>> - !cache_settings.single_threaded,
>> - svn_pool_create(NULL));
>> + {
>> + svn_error_t* err = svn_file_handle_cache__create_cache(
>> +&cache,
>> + cache_settings.file_handle_count,
>> + !cache_settings.single_threaded,
>> + svn_pool_create(NULL));
>> + if (err)
>> + {
>> + svn_error_clear(err);
>> +
>> + /* We need the file handle cache. The only way that an error could
>> + * occur would be some threading error. In that case, there is no
>> + * way we could continue - not even in some limp home mode.
>> + */
>> + SVN_ERR_MALFUNCTION_NO_RETURN();
> Haven't looked at it closely, but:
>
> It seems a shame to lose ERR here. Could we (better) pass it to the
> malfunction handler, or (at least) log it to the FS warning function?
>
Yes, losing the error is somewhat unsatisfying. Basically,
we had to duplicate SVN_ERR_ASSERT_NO_RETURN
to set the #expr parameter of svn_error__malfunction().
Since svn_file_handle_cache__create_cache() gets called
by svn_fs_set_cache_config(), we don't have a FS context
(not even in the caller) that we could use to dump ERR.
>> + }
>> + }
>>
>> return cache;
>> }
>>
>>
> BTW, what's the status of the performance branch? Last I check, its
> STATUS File was empty...
>
Next item to review is the integrate-cache-membuffer branch.
Once that passed the review, large parts of the remaining stuff
could be reviewed and merged again using the STATUS file.
Some changes may require a further integration branch, e.g.
my extensions to the stream interface.
The file handle cache should not be part of SVN 1.7 due to the
high risk of side-effects.
Before I select more revisions for the review, I want go through
my mail and fix the issues that you and others found.
-- Stefan^2.