On 26.10.2015 20:33, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan_at_visualsvn.com]
>> Sent: maandag 26 oktober 2015 19:37
>> To: dev_at_subversion.apache.org; Bert Huijben <bert_at_qqmail.nl>
>> Subject: Re: svn commit: r1710631 - in /subversion/trunk/subversion:
>> include/svn_fs.h libsvn_fs/fs-loader.c libsvn_repos/repos.c
>> tests/svn_test_fs.c
>>
>> On 26 October 2015 at 20:20, Bert Huijben <bert_at_qqmail.nl> wrote:
>>>> -----Original Message-----
>>>> From: ivan_at_apache.org [mailto:ivan_at_apache.org]
>>>> Sent: maandag 26 oktober 2015 16:38
>>>> To: commits_at_subversion.apache.org
>>>> Subject: svn commit: r1710631 - in /subversion/trunk/subversion:
>>>> include/svn_fs.h libsvn_fs/fs-loader.c libsvn_repos/repos.c
>>>> tests/svn_test_fs.c
>>>>
>>>> Author: ivan
>>>> Date: Mon Oct 26 15:37:49 2015
>>>> New Revision: 1710631
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1710631&view=rev
>>>> Log:
>>>> Switch svn_fs_create() to result/scratch pool paradigm.
>>>>
>>>
>>>> Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.c
>>>> URL:
>>>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/fs-
>>>> loader.c?rev=1710631&r1=1710630&r2=1710631&view=diff
>>>>
>> ==========================================================
>>>> ====================
>>>> --- subversion/trunk/subversion/libsvn_fs/fs-loader.c (original)
>>>> +++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Mon Oct 26
>> 15:37:49
>>>> 2015
>>>> @@ -506,11 +506,13 @@ svn_fs_set_warning_func(svn_fs_t *fs, sv
>>>> }
>>>>
>>>> svn_error_t *
>>>> -svn_fs_create(svn_fs_t **fs_p, const char *path, apr_hash_t
>> *fs_config,
>>>> - apr_pool_t *pool)
>>>> +svn_fs_create2(svn_fs_t **fs_p,
>>>> + const char *path,
>>>> + apr_hash_t *fs_config,
>>>> + apr_pool_t *result_pool,
>>>> + apr_pool_t *scratch_pool)
>>>> {
>>>> fs_library_vtable_t *vtable;
>>>> - apr_pool_t *scratch_pool = svn_pool_create(pool);
>>>>
>>>> const char *fs_type = svn_hash__get_cstring(fs_config,
>>>> SVN_FS_CONFIG_FS_TYPE,
>>>> @@ -522,17 +524,25 @@ svn_fs_create(svn_fs_t **fs_p, const cha
>>>> SVN_ERR(write_fs_type(path, fs_type, scratch_pool));
>>>>
>>>> /* Perform the actual creation. */
>>>> - *fs_p = fs_new(fs_config, pool);
>>>> + *fs_p = fs_new(fs_config, result_pool);
>>>>
>>>> SVN_ERR(vtable->create(*fs_p, path, common_pool_lock,
>> scratch_pool,
>>>> common_pool));
>>>> SVN_ERR(vtable->set_svn_fs_open(*fs_p, svn_fs_open2));
>>>>
>>>> - svn_pool_destroy(scratch_pool);
>>>> return SVN_NO_ERROR;
>>>> }
>>>>
>>>> svn_error_t *
>>>> +svn_fs_create(svn_fs_t **fs_p,
>>>> + const char *path,
>>>> + apr_hash_t *fs_config,
>>>> + apr_pool_t *pool)
>>>> +{
>>>> + return svn_fs_create2(fs_p, path, fs_config, pool, pool);
>>> I think it would be nice to create the scratch pool here now, that used to be
>> in svn_fs_create() before introducing the pool.
>>> Most likely not having a scratch pool will have a huge hit on existing callers
>> or the old
>>> function wouldn't have created its own subpool.
>> Subpool in svn_fs_create() function was introduced three days ago in
>> r1710360. Our pool usage convention is very clear that caller should
>> control memory usage and functions should not create subpool for
>> bounded memory usage.
> Sure, but there are cases where we introduced a subpool over a decade ago... We can't just remove them and assume that old code catches up.
>
> If this subpool was added a few days ago, please ignore...
>
> But I spend many hours over the last few years debugging cases where files were suddenly left open, caused by similar changes. And keeping a filesystem open has a huge impact on caching behavior.
>
>> Btw the only caller of svn_fs_create() in our code is svn_repos_create().
> But it is a public api, that most likely originated before 1.0.
Yes, it did. Creating the scratch pool in the compatibility wrapper
makes old code magically use less memory. If the only reason not to do
this is blindly following a convention, then I don't think that reason
is good enough.
-- Brane
Received on 2015-10-26 21:36:04 CET