You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2013/05/12 18:17:50 UTC
svn commit: r1481594 - /subversion/trunk/subversion/libsvn_repos/log.c
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(©from_rev, ©from_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(©from_rev, ©from_path,
+ root, path, subpool));
if (copyfrom_path && SVN_IS_VALID_REVNUM(copyfrom_rev))
{
Re: svn commit: r1481594 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, May 12, 2013 at 8:20 PM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@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(©from_rev, ©from_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(©from_rev, ©from_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>*
*
*
Re: svn commit: r1481594 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, May 12, 2013 at 8:20 PM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@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(©from_rev, ©from_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(©from_rev, ©from_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>*
*
*
Re: svn commit: r1481594 -
/subversion/trunk/subversion/libsvn_repos/log.c
Posted by Daniel Shahaf <da...@elego.de>.
stefan2@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(©from_rev, ©from_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(©from_rev, ©from_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.
Daniel
> if (copyfrom_path && SVN_IS_VALID_REVNUM(copyfrom_rev))
> {
>
>
Re: svn commit: r1481594 -
/subversion/trunk/subversion/libsvn_repos/log.c
Posted by Daniel Shahaf <da...@elego.de>.
stefan2@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(©from_rev, ©from_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(©from_rev, ©from_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.
Daniel
> if (copyfrom_path && SVN_IS_VALID_REVNUM(copyfrom_rev))
> {
>
>
Re: svn commit: r1481594 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, May 12, 2013 at 6:21 PM, Stefan Sperling <st...@elego.de> wrote:
> On Sun, May 12, 2013 at 04:17:50PM -0000, stefan2@apache.org wrote:
> > 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.
>
> Should we backport this to 1.8?
>
Added this plus a similar patch to STATUS.
-- Stefan^2.
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*
Re: svn commit: r1481594 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, May 12, 2013 at 6:21 PM, Stefan Sperling <st...@elego.de> wrote:
> On Sun, May 12, 2013 at 04:17:50PM -0000, stefan2@apache.org wrote:
> > 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.
>
> Should we backport this to 1.8?
>
--
*Join one of our free daily demo sessions on* *Scaling Subversion for the
Enterprise <http://www.wandisco.com/training/webinars>*
*
*
Re: svn commit: r1481594 -
/subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Sperling <st...@elego.de>.
On Sun, May 12, 2013 at 04:17:50PM -0000, stefan2@apache.org wrote:
> 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.
Should we backport this to 1.8?