On 26 October 2015 at 22:33, Bert Huijben <[email protected]> wrote: >> -----Original Message----- >> From: Ivan Zhakov [mailto:[email protected]] >> Sent: maandag 26 oktober 2015 19:37 >> To: [email protected]; Bert Huijben <[email protected]> >> 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 <[email protected]> wrote: >> >> -----Original Message----- >> >> From: [email protected] [mailto:[email protected]] >> >> Sent: maandag 26 oktober 2015 16:38 >> >> To: [email protected] >> >> 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
