On Sat, Apr 19, 2014 at 2:55 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
> > -----Original Message-----
> > From: stefan2_at_apache.org [mailto:stefan2_at_apache.org]
> > Sent: zaterdag 19 april 2014 14:44
> > To: commits_at_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 <ivan_at_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.
Received on 2014-04-20 21:13:52 CEST