On Sat, Apr 19, 2014 at 2:55 PM, Bert Huijben <b...@qqmail.nl> wrote:
> > > > -----Original Message----- > > From: stef...@apache.org [mailto:stef...@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 <i...@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.