Re: svn commit: r1481594 - /subversion/trunk/subversion/libsvn_repos/log.c

On Sun, May 12, 2013 at 8:20 PM, Daniel Shahaf <danielsh_at_elego.de> wrote:

> stefan2_at_apache.org wrote on Sun, May 12, 2013 at 16:17:50 -0000:
> > Author: stefan2
> > Date: Sun May 12 16:17:50 2013
> > New Revision: 1481594
> >
> > URL: http://svn.apache.org/r1481594
> > Log:
> > Significantly reduce I/O during 'svn log -v' and 'svn log' with authz.
> > There is no need to read the copy-from info from the repository
> > if it is already available in the changed path list.
> >
> > * subversion/libsvn_repos/log.c
> > (detect_changed): use the copyfrom info of the change, if available
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_repos/log.c
> >
> > Modified: subversion/trunk/subversion/libsvn_repos/log.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/log.c?rev=1481594&r1=1481593&r2=1481594&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_repos/log.c (original)
> > +++ subversion/trunk/subversion/libsvn_repos/log.c Sun May 12 16:17:50
> 2013
> > @@ -311,11 +311,15 @@ detect_changed(apr_hash_t **changed,
> >
> > if ((action == 'A') || (action == 'R'))
> > {
> > - const char *copyfrom_path;
> > - svn_revnum_t copyfrom_rev;
> > + const char *copyfrom_path = change->copyfrom_path;
> > + svn_revnum_t copyfrom_rev = change->copyfrom_rev;
> >
> > - SVN_ERR(svn_fs_copied_from(&copyfrom_rev, &copyfrom_path,
> > - root, path, subpool));
> > + /* the following is a potentially expensive operation since
> on FSFS
> > + we will follow the DAG from ROOT to PATH and that requires
> > + actually reading the directories along the way. */
> > + if (!change->copyfrom_known)
> > + SVN_ERR(svn_fs_copied_from(&copyfrom_rev, &copyfrom_path,
> > + root, path, subpool));
> >
>
> Elsewhere I've seen Philip do:
>
> if (!change->copyfrom_known)
> SVN_ERR(svn_fs_copied_from(&change->copyfrom_rev,
> &change->copyfrom_path,
> root, path, subpool));
> copyfrom_rev = change->copyfrom_rev;
> copyfrom_path = change->copyfrom_path;
>
> That seems to make sense here, since 'change' will remain available to
> the caller after this function returns.
>

I agree with the general idea but in this case, the changes
won't be used anywhere else in this function or the caller.
Also, there is a pool lifetime issue.

-- Stefan^2.

-- 
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*