You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2013/06/25 10:34:44 UTC
Re: svn commit: r1494966 -
/subversion/trunk/subversion/libsvn_client/ra.c
On Thu, Jun 20, 2013 at 12:56:05PM -0000, stefan2@apache.org wrote:
> Author: stefan2
> Date: Thu Jun 20 12:56:05 2013
> New Revision: 1494966
>
> URL: http://svn.apache.org/r1494966
> Log:
> Optimize 'svn log' and related operations: Teach the client to skip
> the RA call when querying locations segments for repository roots.
> We already know the answer.
>
> * subversion/libsvn_client/ra.c
> (svn_client__repos_location_segments): special case repo roots
>
> Modified:
> subversion/trunk/subversion/libsvn_client/ra.c
>
> Modified: subversion/trunk/subversion/libsvn_client/ra.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/ra.c?rev=1494966&r1=1494965&r2=1494966&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/ra.c (original)
> +++ subversion/trunk/subversion/libsvn_client/ra.c Thu Jun 20 12:56:05 2013
> @@ -601,8 +601,32 @@ svn_client__repos_location_segments(apr_
> struct gls_receiver_baton_t gls_receiver_baton;
> const char *old_session_url;
> svn_error_t *err;
> + const char *rel_path;
>
> *segments = apr_array_make(pool, 8, sizeof(svn_location_segment_t *));
> +
> + /* Save us an RA layer round trip if we are on the repository root and
> + know the result in advance. It's fair to assume that the repo root
> + has already been cached in ra_session.
> +
> + We also assume that all parameters are valid and reivisons properly
> + ordered. Otherwise, the error behavior might differ.
> + */
> + SVN_ERR(svn_ra_get_path_relative_to_root(ra_session, &rel_path, url, pool));
> + if (rel_path && rel_path[0] == 0)
> + {
> + svn_location_segment_t *segment = apr_pcalloc(pool, sizeof(*segment));
> + segment->range_start
> + = end_revision <= start_revision ? end_revision : 0;
> + segment->range_end
> + = end_revision <= start_revision ? start_revision : 0;
The above logic looks strange.
If I understand correctly, we return the following location segment
for the root node, if end_revision < start_revision:
[end revision, start revision]
and otherwise we return the following location segment for the
root node:
[0, 0]
I would have expected the following instead:
[0, start_revision] (if start_revision is the younger rev)
or:
[0, end_revision] (if end_revision is the younger rev)
Because the root node exists in all revisions starting at zero and
ending at max(start_revision, end_revision).
Also, svn_ra_get_location_segments(), which this function is a wrapper
of, is supposed to treat start and end revisions of SVN_INVALID_REVNUM
in a special way:
* @a end_rev may be @c SVN_INVALID_REVNUM to indicate that you want
* to trace the history of the object to its origin.
*
* @a start_rev may be @c SVN_INVALID_REVNUM to indicate "the HEAD
* revision". Otherwise, @a start_rev must be younger than @a end_rev
* (unless @a end_rev is @c SVN_INVALID_REVNUM).
Your client-side optimization doesn't seem to account for these cases.
Why not?
> + segment->path = rel_path;
> + APR_ARRAY_PUSH(*segments, svn_location_segment_t *) = segment;
> +
> + return SVN_NO_ERROR;
> + }
> +
> + /* Do it the hard way and ask the repository layer. */
> gls_receiver_baton.segments = *segments;
> gls_receiver_baton.ctx = ctx;
> gls_receiver_baton.pool = pool;
>
Re: svn commit: r1494966 - /subversion/trunk/subversion/libsvn_client/ra.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Jun 25, 2013 at 10:34 AM, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Jun 20, 2013 at 12:56:05PM -0000, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Thu Jun 20 12:56:05 2013
> > New Revision: 1494966
> >
> > URL: http://svn.apache.org/r1494966
> > Log:
> > Optimize 'svn log' and related operations: Teach the client to skip
> > the RA call when querying locations segments for repository roots.
> > We already know the answer.
> >
> > * subversion/libsvn_client/ra.c
> > (svn_client__repos_location_segments): special case repo roots
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_client/ra.c
> >
> > Modified: subversion/trunk/subversion/libsvn_client/ra.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/ra.c?rev=1494966&r1=1494965&r2=1494966&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_client/ra.c (original)
> > +++ subversion/trunk/subversion/libsvn_client/ra.c Thu Jun 20 12:56:05
> 2013
> > @@ -601,8 +601,32 @@ svn_client__repos_location_segments(apr_
> > struct gls_receiver_baton_t gls_receiver_baton;
> > const char *old_session_url;
> > svn_error_t *err;
> > + const char *rel_path;
> >
> > *segments = apr_array_make(pool, 8, sizeof(svn_location_segment_t *));
> > +
> > + /* Save us an RA layer round trip if we are on the repository root and
> > + know the result in advance. It's fair to assume that the repo root
> > + has already been cached in ra_session.
> > +
> > + We also assume that all parameters are valid and reivisons properly
> > + ordered. Otherwise, the error behavior might differ.
> > + */
> > + SVN_ERR(svn_ra_get_path_relative_to_root(ra_session, &rel_path, url,
> pool));
> > + if (rel_path && rel_path[0] == 0)
> > + {
> > + svn_location_segment_t *segment = apr_pcalloc(pool,
> sizeof(*segment));
> > + segment->range_start
> > + = end_revision <= start_revision ? end_revision : 0;
> > + segment->range_end
> > + = end_revision <= start_revision ? start_revision : 0;
>
> The above logic looks strange.
>
> If I understand correctly, we return the following location segment
> for the root node, if end_revision < start_revision:
>
> [end revision, start revision]
>
> and otherwise we return the following location segment for the
> root node:
>
> [0, 0]
>
> I would have expected the following instead:
>
> [0, start_revision] (if start_revision is the younger rev)
>
> or:
>
> [0, end_revision] (if end_revision is the younger rev)
>
> Because the root node exists in all revisions starting at zero and
> ending at max(start_revision, end_revision).
>
>
> Also, svn_ra_get_location_segments(), which this function is a wrapper
> of, is supposed to treat start and end revisions of SVN_INVALID_REVNUM
> in a special way:
>
> * @a end_rev may be @c SVN_INVALID_REVNUM to indicate that you want
> * to trace the history of the object to its origin.
> *
> * @a start_rev may be @c SVN_INVALID_REVNUM to indicate "the HEAD
> * revision". Otherwise, @a start_rev must be younger than @a end_rev
> * (unless @a end_rev is @c SVN_INVALID_REVNUM).
>
> Your client-side optimization doesn't seem to account for these cases.
> Why not?
>
> > + segment->path = rel_path;
> > + APR_ARRAY_PUSH(*segments, svn_location_segment_t *) = segment;
> > +
> > + return SVN_NO_ERROR;
> > + }
> > +
> > + /* Do it the hard way and ask the repository layer. */
> > gls_receiver_baton.segments = *segments;
> > gls_receiver_baton.ctx = ctx;
> > gls_receiver_baton.pool = pool;
> >
>
Due to the heatwave, I obviously wrote that code using
half a braincell at best. r1496886 should be much better.
Thanks for the review!
-- Stefan^2.