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.

Btw the only caller of svn_fs_create() in our code is svn_repos_create().

> (Any specific reason this wan't created in a deprecated.c file?)
>
Good point. I'll add deprecated.c file in libsvn_fs tomorrow.

-- 
Ivan Zhakov