Stefan Fuhrmann wrote on Wed, Nov 10, 2010 at 00:32:14 +0100:
> On 09.11.2010 23:37, Daniel Shahaf wrote:
>> stef...@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.
>
