You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2014/04/19 14:44:08 UTC
svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.c fs_fs.h
Author: stefan2
Date: Sat Apr 19 12:44:07 2014
New Revision: 1588651
URL: http://svn.apache.org/r1588651
Log:
Reduce the size of an FSFS instance by using temporary pools
for temp. data during fs_open().
* subversion/libsvn_fs_fs/fs_fs.h
(svn_fs_fs__initialize_caches): Document that the POOL shall be
used for temporaries only.
* subversion/libsvn_fs_fs/caching.c
(svn_fs_fs__initialize_caches): Fix the only place where we used
POOL instead of FS->POOL for
something with svn_fs_t lifetime.
* subversion/libsvn_fs_fs/fs.c
(fs_serialized_init): Document POOL usage that we will now rely on.
(fs_open): Use a SUBPOOL for anything temporary during FS init.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/caching.c
subversion/trunk/subversion/libsvn_fs_fs/fs.c
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
Modified: subversion/trunk/subversion/libsvn_fs_fs/caching.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/caching.c?rev=1588651&r1=1588650&r2=1588651&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/caching.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/caching.c Sat Apr 19 12:44:07 2014
@@ -411,11 +411,11 @@ svn_fs_fs__initialize_caches(svn_fs_t *f
#ifdef SVN_DEBUG_CACHE_DUMP_STATS
/* schedule printing the global access statistics upon pool cleanup,
- * i.e. end of FSFS session.
- */
+ * i.e. when the repo instance gets closed / cleaned up.
+ */
if (membuffer)
- apr_pool_cleanup_register(pool,
- pool,
+ apr_pool_cleanup_register(fs->pool,
+ fs->pool,
dump_global_cache_statistics,
apr_pool_cleanup_null);
#endif
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.c?rev=1588651&r1=1588650&r2=1588651&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Sat Apr 19 12:44:07 2014
@@ -56,6 +56,9 @@
+/* Initialize the part of FS that requires global serialization across all
+ instances. The caller is responsible of ensuring that serialization.
+ Use COMMON_POOL for process-wide and POOL for temporary allocations. */
static svn_error_t *
fs_serialized_init(svn_fs_t *fs, apr_pool_t *common_pool, apr_pool_t *pool)
{
@@ -275,15 +278,19 @@ fs_open(svn_fs_t *fs,
apr_pool_t *pool,
apr_pool_t *common_pool)
{
+ apr_pool_t *subpool = svn_pool_create(pool);
+
SVN_ERR(svn_fs__check_fs(fs, FALSE));
SVN_ERR(initialize_fs_struct(fs));
- SVN_ERR(svn_fs_fs__open(fs, path, pool));
+ SVN_ERR(svn_fs_fs__open(fs, path, subpool));
- SVN_ERR(svn_fs_fs__initialize_caches(fs, pool));
+ SVN_ERR(svn_fs_fs__initialize_caches(fs, subpool));
SVN_MUTEX__WITH_LOCK(common_pool_lock,
- fs_serialized_init(fs, common_pool, pool));
+ fs_serialized_init(fs, common_pool, subpool));
+
+ svn_pool_destroy(subpool);
return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h?rev=1588651&r1=1588650&r2=1588651&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h Sat Apr 19 12:44:07 2014
@@ -218,7 +218,7 @@ svn_fs_fs__get_node_origin(const svn_fs_
/* Initialize all session-local caches in FS according to the global
- cache settings. Use POOL for allocations.
+ cache settings. Use POOL for temporary allocations.
Please note that it is permissible for this function to set some
or all of these caches to NULL, regardless of any setting. */
Re: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.c fs_fs.h
Posted by Branko Čibej <br...@wandisco.com>.
On 19.04.2014 17:35, Ivan Zhakov wrote:
> On 19 April 2014 19:25, Branko Čibej <br...@wandisco.com> wrote:
>> On 19.04.2014 17:12, Ivan Zhakov wrote:
>>
>> On 19 April 2014 16:44, <st...@apache.org> wrote:
>>
>> Author: stefan2
>> Date: Sat Apr 19 12:44:07 2014
>> New Revision: 1588651
>>
>> URL: http://svn.apache.org/r1588651
>> Log:
>> Reduce the size of an FSFS instance by using temporary pools
>> for temp. data during fs_open().
>>
>> * subversion/libsvn_fs_fs/fs_fs.h
>> (svn_fs_fs__initialize_caches): Document that the POOL shall be
>> used for temporaries only.
>>
>> * subversion/libsvn_fs_fs/caching.c
>> (svn_fs_fs__initialize_caches): Fix the only place where we used
>> POOL instead of FS->POOL for
>> something with svn_fs_t lifetime.
>>
>> * subversion/libsvn_fs_fs/fs.c
>> (fs_serialized_init): Document POOL usage that we will now rely on.
>> (fs_open): Use a SUBPOOL for anything temporary during FS init.
>>
>> Just creating subpool for temporary allocation violates pool design
>> imho. Proper fix would be add scratch pool parameter to
>> svn_fs_open()/svn_fs_create().
>>
>>
> Hi Branko,
>
>> Doesn't "violate" it, as such ... there's a balance to be maintained here.
> I agree about balance, but general idea of pool design that caller
> controls memory usage of callee.
>
>> Adding a scratch-pool parameter means revising the API and propagating the
>> revision up the call chain to all the places where a meaningful scratch pool
>> is already available. Sure it's better to do do that in the long run; but
>> does the change warrant the related code churn now? Consider that there's a
>> vtable involved here, too.
>>
> I still prefer adding scratch_pool: users interested of improved
> memory usage (like mod_dav_svn) can use new API. Btw memory usage may
> increase in some cases with subpools: every pool consumes 8k of
> memory, subpools are not cleared on error in general. So we got 8k
> memory wasted on error cases.
I think we're in violent agreement :) except on the minor point about
optimizing the error case.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.c fs_fs.h
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 April 2014 19:25, Branko Čibej <br...@wandisco.com> wrote:
> On 19.04.2014 17:12, Ivan Zhakov wrote:
>
> On 19 April 2014 16:44, <st...@apache.org> wrote:
>
> Author: stefan2
> Date: Sat Apr 19 12:44:07 2014
> New Revision: 1588651
>
> URL: http://svn.apache.org/r1588651
> Log:
> Reduce the size of an FSFS instance by using temporary pools
> for temp. data during fs_open().
>
> * subversion/libsvn_fs_fs/fs_fs.h
> (svn_fs_fs__initialize_caches): Document that the POOL shall be
> used for temporaries only.
>
> * subversion/libsvn_fs_fs/caching.c
> (svn_fs_fs__initialize_caches): Fix the only place where we used
> POOL instead of FS->POOL for
> something with svn_fs_t lifetime.
>
> * subversion/libsvn_fs_fs/fs.c
> (fs_serialized_init): Document POOL usage that we will now rely on.
> (fs_open): Use a SUBPOOL for anything temporary during FS init.
>
> Just creating subpool for temporary allocation violates pool design
> imho. Proper fix would be add scratch pool parameter to
> svn_fs_open()/svn_fs_create().
>
>
Hi Branko,
> Doesn't "violate" it, as such ... there's a balance to be maintained here.
I agree about balance, but general idea of pool design that caller
controls memory usage of callee.
> Adding a scratch-pool parameter means revising the API and propagating the
> revision up the call chain to all the places where a meaningful scratch pool
> is already available. Sure it's better to do do that in the long run; but
> does the change warrant the related code churn now? Consider that there's a
> vtable involved here, too.
>
I still prefer adding scratch_pool: users interested of improved
memory usage (like mod_dav_svn) can use new API. Btw memory usage may
increase in some cases with subpools: every pool consumes 8k of
memory, subpools are not cleared on error in general. So we got 8k
memory wasted on error cases.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.c fs_fs.h
Posted by Branko Čibej <br...@wandisco.com>.
On 19.04.2014 17:12, Ivan Zhakov wrote:
> On 19 April 2014 16:44, <st...@apache.org> wrote:
>> Author: stefan2
>> Date: Sat Apr 19 12:44:07 2014
>> New Revision: 1588651
>>
>> URL: http://svn.apache.org/r1588651
>> Log:
>> Reduce the size of an FSFS instance by using temporary pools
>> for temp. data during fs_open().
>>
>> * subversion/libsvn_fs_fs/fs_fs.h
>> (svn_fs_fs__initialize_caches): Document that the POOL shall be
>> used for temporaries only.
>>
>> * subversion/libsvn_fs_fs/caching.c
>> (svn_fs_fs__initialize_caches): Fix the only place where we used
>> POOL instead of FS->POOL for
>> something with svn_fs_t lifetime.
>>
>> * subversion/libsvn_fs_fs/fs.c
>> (fs_serialized_init): Document POOL usage that we will now rely on.
>> (fs_open): Use a SUBPOOL for anything temporary during FS init.
>>
> Just creating subpool for temporary allocation violates pool design
> imho. Proper fix would be add scratch pool parameter to
> svn_fs_open()/svn_fs_create().
Doesn't "violate" it, as such ... there's a balance to be maintained
here. Adding a scratch-pool parameter means revising the API and
propagating the revision up the call chain to all the places where a
meaningful scratch pool is already available. Sure it's better to do do
that in the long run; but does the change warrant the related code churn
now? Consider that there's a vtable involved here, too.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.c fs_fs.h
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 April 2014 16:44, <st...@apache.org> wrote:
> Author: stefan2
> Date: Sat Apr 19 12:44:07 2014
> New Revision: 1588651
>
> URL: http://svn.apache.org/r1588651
> Log:
> Reduce the size of an FSFS instance by using temporary pools
> for temp. data during fs_open().
>
> * subversion/libsvn_fs_fs/fs_fs.h
> (svn_fs_fs__initialize_caches): Document that the POOL shall be
> used for temporaries only.
>
> * subversion/libsvn_fs_fs/caching.c
> (svn_fs_fs__initialize_caches): Fix the only place where we used
> POOL instead of FS->POOL for
> something with svn_fs_t lifetime.
>
> * subversion/libsvn_fs_fs/fs.c
> (fs_serialized_init): Document POOL usage that we will now rely on.
> (fs_open): Use a SUBPOOL for anything temporary during FS init.
>
Just creating subpool for temporary allocation violates pool design
imho. Proper fix would be add scratch pool parameter to
svn_fs_open()/svn_fs_create().
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.c fs_fs.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, Apr 19, 2014 at 2:55 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: stefan2@apache.org [mailto:stefan2@apache.org]
> > Sent: zaterdag 19 april 2014 14:44
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1588651 - in
> > /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.c fs_fs.h
> >
> > Author: stefan2
> > Date: Sat Apr 19 12:44:07 2014
> > New Revision: 1588651
> >
> > URL: http://svn.apache.org/r1588651
> > Log:
> > Reduce the size of an FSFS instance by using temporary pools
> > for temp. data during fs_open().
> >
> > * subversion/libsvn_fs_fs/fs_fs.h
> > (svn_fs_fs__initialize_caches): Document that the POOL shall be
> > used for temporaries only.
> >
> > * subversion/libsvn_fs_fs/caching.c
> > (svn_fs_fs__initialize_caches): Fix the only place where we used
> > POOL instead of FS->POOL for
> > something with svn_fs_t lifetime.
> >
> > * subversion/libsvn_fs_fs/fs.c
> > (fs_serialized_init): Document POOL usage that we will now rely on.
> > (fs_open): Use a SUBPOOL for anything temporary during FS init.
>
> +1
> (Haven't reviewed all details, but the change looks good)
>
> Usually I rename relevant arguments of inner functions to scratch_pool
> (sometimes leaving a local variable with the old name) to make its use
> directly clear in every usage without relying on outside documentation.
>
+0 on that. I also tend to use result_pool / scratch_pool
even for functions with single pool parameters. However,
I see some weak counter indication:
* I found myself wondering whether there is / where is
the respective other pool.
* I vaguely remember being told that single pool funcs
should use POOL. So, there might have been a consensus
on that way back when.
Sounds like a good topic to discuss in Berlin because probably
few people if any actually feel passionate about it but taking
these things to the @dev list first is a sure recipe for flame wars.
On Sat, Apr 19, 2014 at 5:12 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> Just creating subpool for temporary allocation violates pool design
> imho. Proper fix would be add scratch pool parameter to
> svn_fs_open()/svn_fs_create().
>
In addition to what Brane already said (the similar vtable
access patch would have particularly bad ripple effects on
the API when switching to the two pool paradigm), I see a
good reason for using subpools - even if they were created
from scratch pools.
Every (buffered) file being opened consumes 8k, i.e. just
as much as a new sub-pool. Since initialization stacks often
don't involve loops, the same scratch pool will be used for
many files & temp object before actually being cleared up.
So, having a clear-able subpool can be a (usually minor)
improvement.
-- Stefan^2.
Re: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs:
caching.c fs.c fs_fs.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sat, Apr 19, 2014 at 2:55 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: stefan2@apache.org [mailto:stefan2@apache.org]
> > Sent: zaterdag 19 april 2014 14:44
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1588651 - in
> > /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.c fs_fs.h
> >
> > Author: stefan2
> > Date: Sat Apr 19 12:44:07 2014
> > New Revision: 1588651
> >
> > URL: http://svn.apache.org/r1588651
> > Log:
> > Reduce the size of an FSFS instance by using temporary pools
> > for temp. data during fs_open().
> >
> > * subversion/libsvn_fs_fs/fs_fs.h
> > (svn_fs_fs__initialize_caches): Document that the POOL shall be
> > used for temporaries only.
> >
> > * subversion/libsvn_fs_fs/caching.c
> > (svn_fs_fs__initialize_caches): Fix the only place where we used
> > POOL instead of FS->POOL for
> > something with svn_fs_t lifetime.
> >
> > * subversion/libsvn_fs_fs/fs.c
> > (fs_serialized_init): Document POOL usage that we will now rely on.
> > (fs_open): Use a SUBPOOL for anything temporary during FS init.
>
> +1
> (Haven't reviewed all details, but the change looks good)
>
> Usually I rename relevant arguments of inner functions to scratch_pool
> (sometimes leaving a local variable with the old name) to make its use
> directly clear in every usage without relying on outside documentation.
>
+0 on that. I also tend to use result_pool / scratch_pool
even for functions with single pool parameters. However,
I see some weak counter indication:
* I found myself wondering whether there is / where is
the respective other pool.
* I vaguely remember being told that single pool funcs
should use POOL. So, there might have been a consensus
on that way back when.
Sounds like a good topic to discuss in Berlin because probably
few people if any actually feel passionate about it but taking
these things to the @dev list first is a sure recipe for flame wars.
On Sat, Apr 19, 2014 at 5:12 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> Just creating subpool for temporary allocation violates pool design
> imho. Proper fix would be add scratch pool parameter to
> svn_fs_open()/svn_fs_create().
>
In addition to what Brane already said (the similar vtable
access patch would have particularly bad ripple effects on
the API when switching to the two pool paradigm), I see a
good reason for using subpools - even if they were created
from scratch pools.
Every (buffered) file being opened consumes 8k, i.e. just
as much as a new sub-pool. Since initialization stacks often
don't involve loops, the same scratch pool will be used for
many files & temp object before actually being cleared up.
So, having a clear-able subpool can be a (usually minor)
improvement.
-- Stefan^2.
RE: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.c fs_fs.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: zaterdag 19 april 2014 14:44
> To: commits@subversion.apache.org
> Subject: svn commit: r1588651 - in
> /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.c fs_fs.h
>
> Author: stefan2
> Date: Sat Apr 19 12:44:07 2014
> New Revision: 1588651
>
> URL: http://svn.apache.org/r1588651
> Log:
> Reduce the size of an FSFS instance by using temporary pools
> for temp. data during fs_open().
>
> * subversion/libsvn_fs_fs/fs_fs.h
> (svn_fs_fs__initialize_caches): Document that the POOL shall be
> used for temporaries only.
>
> * subversion/libsvn_fs_fs/caching.c
> (svn_fs_fs__initialize_caches): Fix the only place where we used
> POOL instead of FS->POOL for
> something with svn_fs_t lifetime.
>
> * subversion/libsvn_fs_fs/fs.c
> (fs_serialized_init): Document POOL usage that we will now rely on.
> (fs_open): Use a SUBPOOL for anything temporary during FS init.
+1
(Haven't reviewed all details, but the change looks good)
Usually I rename relevant arguments of inner functions to scratch_pool (sometimes leaving a local variable with the old name) to make its use directly clear in every usage without relying on outside documentation.
Bert
RE: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.c fs_fs.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: zaterdag 19 april 2014 14:44
> To: commits@subversion.apache.org
> Subject: svn commit: r1588651 - in
> /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.c fs_fs.h
>
> Author: stefan2
> Date: Sat Apr 19 12:44:07 2014
> New Revision: 1588651
>
> URL: http://svn.apache.org/r1588651
> Log:
> Reduce the size of an FSFS instance by using temporary pools
> for temp. data during fs_open().
>
> * subversion/libsvn_fs_fs/fs_fs.h
> (svn_fs_fs__initialize_caches): Document that the POOL shall be
> used for temporaries only.
>
> * subversion/libsvn_fs_fs/caching.c
> (svn_fs_fs__initialize_caches): Fix the only place where we used
> POOL instead of FS->POOL for
> something with svn_fs_t lifetime.
>
> * subversion/libsvn_fs_fs/fs.c
> (fs_serialized_init): Document POOL usage that we will now rely on.
> (fs_open): Use a SUBPOOL for anything temporary during FS init.
+1
(Haven't reviewed all details, but the change looks good)
Usually I rename relevant arguments of inner functions to scratch_pool (sometimes leaving a local variable with the old name) to make its use directly clear in every usage without relying on outside documentation.
Bert