You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2015/10/26 16:37:49 UTC
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.
* subversion/include/svn_fs.h
(svn_fs_create2): Revv svn_fs_create() with SCRATCH_POOL argument.
(svn_fs_create): Deprecate.
* subversion/libsvn_fs/fs-loader.c
(svn_fs_create2): Revv. Use SCRATCH_POOL for temporary allocations instead
of creating new subpool.
(svn_fs_create): Implement deprecated function.
* subversion/libsvn_repos/repos.c
(svn_repos_create): Use svn_fs_create2().
* subversion/tests/svn_test_fs.c
(create_fs): Use svn_fs_create2().
Modified:
subversion/trunk/subversion/include/svn_fs.h
subversion/trunk/subversion/libsvn_fs/fs-loader.c
subversion/trunk/subversion/libsvn_repos/repos.c
subversion/trunk/subversion/tests/svn_test_fs.c
Modified: subversion/trunk/subversion/include/svn_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_fs.h?rev=1710631&r1=1710630&r2=1710631&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_fs.h (original)
+++ subversion/trunk/subversion/include/svn_fs.h Mon Oct 26 15:37:49 2015
@@ -269,11 +269,13 @@ svn_fs_set_warning_func(svn_fs_t *fs,
* @c NULL, the options it contains modify the behavior of the
* filesystem. The interpretation of @a fs_config is specific to the
* filesystem back-end. The new filesystem may be closed by
- * destroying @a pool.
+ * destroying @a result_pool.
+ *
+ * Use @a scratch_pool for temporary allocations.
*
* @note The lifetime of @a fs_config must not be shorter than @a
- * pool's. It's a good idea to allocate @a fs_config from @a pool or
- * one of its ancestors.
+ * result_pool's. It's a good idea to allocate @a fs_config from
+ * @a result_pool or one of its ancestors.
*
* If @a fs_config contains a value for #SVN_FS_CONFIG_FS_TYPE, that
* value determines the filesystem type for the new filesystem.
@@ -289,8 +291,22 @@ svn_fs_set_warning_func(svn_fs_t *fs,
* though the caller should not rely upon any particular default if they
* wish to ensure that a filesystem of a specific type is created.
*
+ * @since New in 1.10.
+ */
+svn_error_t *
+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);
+
+/**
+ * Like svn_fs_create2(), but without @a scratch_pool.
+ *
+ * @deprecated Provided for backward compatibility with the 1.9 API.
* @since New in 1.1.
*/
+SVN_DEPRECATED
svn_error_t *
svn_fs_create(svn_fs_t **fs_p,
const char *path,
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);
+}
+
+svn_error_t *
svn_fs_open2(svn_fs_t **fs_p, const char *path, apr_hash_t *fs_config,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
Modified: subversion/trunk/subversion/libsvn_repos/repos.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/repos.c?rev=1710631&r1=1710630&r2=1710631&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/repos.c (original)
+++ subversion/trunk/subversion/libsvn_repos/repos.c Mon Oct 26 15:37:49 2015
@@ -1175,8 +1175,8 @@ svn_repos_create(svn_repos_t **repos_p,
SVN_ERR(lock_repos(repos, FALSE, FALSE, scratch_pool));
/* Create an environment for the filesystem. */
- if ((err = svn_fs_create(&repos->fs, repos->db_path, fs_config,
- result_pool)))
+ if ((err = svn_fs_create2(&repos->fs, repos->db_path, fs_config,
+ result_pool, scratch_pool)))
{
/* If there was an error making the filesytem, e.g. unknown/supported
* filesystem type. Clean up after ourselves. Yes this is safe because
Modified: subversion/trunk/subversion/tests/svn_test_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/svn_test_fs.c?rev=1710631&r1=1710630&r2=1710631&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/svn_test_fs.c (original)
+++ subversion/trunk/subversion/tests/svn_test_fs.c Mon Oct 26 15:37:49 2015
@@ -119,7 +119,7 @@ create_fs(svn_fs_t **fs_p,
runs. */
SVN_ERR(svn_io_remove_dir2(name, TRUE, NULL, NULL, pool));
- SVN_ERR(svn_fs_create(fs_p, name, fs_config, pool));
+ SVN_ERR(svn_fs_create2(fs_p, name, fs_config, pool, pool));
if (! *fs_p)
return svn_error_create(SVN_ERR_FS_GENERAL, NULL,
"Couldn't alloc a new fs object.");
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
Posted by Branko Čibej <br...@apache.org>.
On 26.10.2015 20:33, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> Sent: maandag 26 oktober 2015 19:37
>> To: dev@subversion.apache.org; Bert Huijben <be...@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 <be...@qqmail.nl> wrote:
>>>> -----Original Message-----
>>>> From: ivan@apache.org [mailto:ivan@apache.org]
>>>> Sent: maandag 26 oktober 2015 16:38
>>>> To: commits@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
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
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 October 2015 at 22:33, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> Sent: maandag 26 oktober 2015 19:37
>> To: dev@subversion.apache.org; Bert Huijben <be...@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 <be...@qqmail.nl> wrote:
>> >> -----Original Message-----
>> >> From: ivan@apache.org [mailto:ivan@apache.org]
>> >> Sent: maandag 26 oktober 2015 16:38
>> >> To: commits@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
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
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: maandag 26 oktober 2015 19:37
> To: dev@subversion.apache.org; Bert Huijben <be...@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 <be...@qqmail.nl> wrote:
> >> -----Original Message-----
> >> From: ivan@apache.org [mailto:ivan@apache.org]
> >> Sent: maandag 26 oktober 2015 16:38
> >> To: commits@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
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
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 October 2015 at 20:20, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: ivan@apache.org [mailto:ivan@apache.org]
>> Sent: maandag 26 oktober 2015 16:38
>> To: commits@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
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
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: ivan@apache.org [mailto:ivan@apache.org]
> Sent: maandag 26 oktober 2015 16:38
> To: commits@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.
(Any specific reason this wan't created in a deprecated.c file?)
Bert
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
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: ivan@apache.org [mailto:ivan@apache.org]
> Sent: maandag 26 oktober 2015 16:38
> To: commits@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.
(Any specific reason this wan't created in a deprecated.c file?)
Bert