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_at_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.
Received on 2010-11-22 03:07:29 CET