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 22:33, Bert Huijben <bert_at_qqmail.nl> 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...
Ack.

>
> 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.
>
Yes, it happens sometimes. Btw cannot rely on subpool clearing to
release 'critical' resources since we usually do not clear subpool on
error condition.

>>
>> 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.
>
Sure, but I don't think that many of our API users uses
svn_fs_create() directly and care so much about memory usage. Memory
usage could be important for svn_fs_open()/svn_repos_open(), but not
for svn_fs_create(). Anyway that callers may use new API.

>> (Any specific reason this wan't created in a deprecated.c file?)
>>
> Good point. I'll add deprecated.c file in libsvn_fs tomorrow.
>
I've moved svn_fs_create() and other deprecated libsvn_fs functions to
deprecated.c in r1710749. Thanks for review!

-- 
Ivan Zhakov