subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: svn commit: r1588651 - in /subversion/trunk/subversion/libsvn_fs_fs: caching.c fs.c fs_fs.h
Date Sun, 20 Apr 2014 19:13:20 GMT
On Sat, Apr 19, 2014 at 2:55 PM, Bert Huijben <bert@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 <ivan@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.

Mime
View raw message