> -----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.
Bert
Received on 2015-10-26 20:34:01 CET