RE: [PATCH] Reduce memory usage of svn log -v

Here is the revised patch following Greg Stein's comments:

Log:

Reduce the memory usage of 'verbose' log commands.

* subversion/libsvn_repos/log.c

  (detect_changed): Allocate and return the CHANGED hash set
  rather than take a pre-created one and fill it. Moved the
  declaration of ITEM into the loop. Use POOL for the
  CHANGES hash set and the hash iterator. Clear SUBPOOL
  at the start of each iteration.

  (svn_repos_get_logs): Removed allocation of CHANGED_PATHS --
  detected_changed will allocate it now.

-----Original Message-----
From: Greg Stein [mailto:gstein@lyra.org]
Sent: Thursday, May 15, 2003 1:16 AM
To: D.J. Heap
Cc: dev@subversion.tigris.org
Subject: Re: [PATCH] Reduce memory usage of svn log -v

On Wed, May 14, 2003 at 08:37:33PM -0600, D.J. Heap wrote:
> Scott Collins wrote:
>...
> >In your patch, would it be as effective to not add |scratchpool|, but
in
> >the place where you added |svn_pool_clear (scratchpool);|, substitute

> >|svn_pool_clear (subpool);|? But if turns out you must add
> >|scratchpool|, is it better if it comes out of |subpool| rather than
> >|pool|?
>...
> I believe subpool must not be cleared (actually I tried it and boom)
> because the hash iterator is in it?

Right.

Looking at that function, it would be fixed if we follow standard design
policy here:

* things allocated outside a loop are placed into the pool passed to the
  function. the caller knows how many times the function will be called
and
  will deal with handling the passed-in pool properly (clearing and
  destroying at the right times).
  
  placing these items into a subpool doesn't help since the peak memory
  usage is the same whether you place them directly into the pool or
into a
  subpool. thus, the policy to use the passed-in pool.

* anything within a loop should use a subpool (sometimes called an
"iterator
  pool"). the subpool is cleared on each entry to the loop.

Thus: place the svn_fs_paths_changed() can apr_has_first() results into
'pool'. The item and the paths placed into the item also go into the
pool
since they need to live past the function-return. The
svn_fs_copied_from()
stays in the subpool. The subpool gets cleared on loop-entry.

No scratchpool is needed -- the subpool should have filled that role
from
the start.

Cheers,
-g

p.s. separate change here: the 'item' variable declaration would
normally
  occur inside the block rather than the outermost block. we scope
things as
  tightly as possible.
p.p.s. the concept of passing in a pre-created hash table, and having
the
  function fill it in, is also a bit non-standard. it would be best for
the
  function to allocate the 'changed' hash table itself and return it.

-- 
Greg Stein, http://www.lyra.org/
**********************************************************************
This email and any files transmitted with it are confidential
and intended solely for the use of the individual or entity to
whom they are addressed. If you have received this email
in error please notify the system manager.
This footnote also confirms that this email message has been
swept by MIMEsweeper for the presence of computer viruses.
www.mimesweeper.com
**********************************************************************

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org