Woo, thanks for the comments. My notes below. As well I have attached my
revised patch.
[[[
Fix for issue 2151 "'svn ls' is slow over ra_dav"
This patch implements a solution to issue 2151.
We now only request the needed props in the PROPFIND
for server listings. 'svn ls' is now noticably faster.
In most cases 'ls' takes about half the time and half
the bandwidth - In some case even better results.
For backward compatibility, before doing the long PROPFIND
we make another simpler PROPFIND to see if the server
supports the new type of request (supports the
deadprop-count prop). If it does we use the new scheme
and performance is improved - If not then we use the old
scheme and the slowness persists.
To summarise: only patched servers and patched clients
have improved speed. Mismatched configurations (old client
or server) are not improved but still work. Regular dav
clients are still slow.
Patch by: Jean-Marc Godbout <jean-marc@computrad.com>
Suggested by: Erik Scrafford <erik@scrafford.org>
Ben Collins-Sussman <sussman@collab.net>
Everyone else who contributed to issue 2151
* subversion/mod_dav_svn/liveprops.c
(SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
(dav_svn_props[]): Added the deadprop-count prop to the list of
supported props.
(dav_svn_insert_prop): Returns the number of deadprops
found in the fs for that file.
* subversion/libsvn_ra_dav/ra_dav.h
(SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
(ELEM_deadprop_count): Added the prop to the list of props
* subversion/libsvn_ra_dav/props.c
(elem_definition, propfind_elements): Added ELEM_deadprop_count
to the list of propfind props
* subversion/libsvn_ra_dav/fetch.c
(dirent_props[]): New array, list of props to get in a dirent PROPFIND
(deadprop_count_support_props[ ]): New array, list of props to
get in a root PROPFIND
(svn_ra_dav__get_dir): check if server supports 'deadprop-count',
and if so, request fewer properties
]]]
- Show quoted text -
On 8/30/05, Ben Collins-Sussman <sussman@collab.net> wrote:
>
> Thanks, this patch looks pretty good!
>
> This is a review from me and kfogel.
>
> The review includes both formatting problems, minor nits, and also
> some substantive comments on the changes. We look forward to the next
> iteration of the patch. :-)
>
>
> > [[[
> > Fix for issue 2151 "'svn ls' is slow over ra_dav"
> >
> > This patch implements a solution to issue 2151. We now only request
> > the needed props in the PROPFIND for server listings. 'svn ls' is now
> > noticably faster. In most cases 'ls' takes about half the time and
> > half the bandwidth - In some case even better results.
> >
> > For backward compatibility, before doing the long PROPFIND we make
> > another simpler PROPFIND to see if the server supports the new
> type of
> > request (supports the deadprop-count prop). If it does we use the new
> > scheme and performance is improved - If not then we use the old
> scheme
> > and the slowness persists.
> >
>
> Here's another strategy that we've been pondering:
>
> We see that the new 'deadprop-count' property is already in your short
> list of properties to fetch. Instead of unconditionally doing an
> extra PROPFIND for the new 'deadprop-count' property, why not just
> *always* request the short list in the depth-1 PROPFIND? If the new
> property doesn't come back, then repeat the depth-1 PROPFIND, asking
> for <allprops>.
>
> The problem with this is that 'svn ls' will now be about twice as slow
> against *old* servers -- doing two depth-1 PROPFINDs instead of one.
> So we wouldn't just be rewarding those who upgrade servers with 2x
> speed, we'd be actively *punishing* people who don't upgrade their
> servers with 1/2 speed. This may cause riots.
>
> So we're thinking that maybe we should stick with your strategy for
> now, but say, two releases from now, go with our strategy. By then,
> most servers will have upgraded, and it'll be silly for clients to be
> wasting time on an extra unconditional turnaround.
>
> Does that sound like a good idea?
Yes. There is another strategy which is to add the state of the server to
the session. That way you would only do the propfind if you are unsure if
the server supports the deadprop-count prop because you can't determine it
from other clues (result of an OPTIONS request for example). This could be
transferred to issue 1161 (excess propfinds). I will add this to the bottom
of the issue's discussion once I get home (I'm currently on the road).
1. Add the supports_deadprop_count variable to the session. If 2 "svn ls"
are done in the same session, then the second time around you wouldn't need
to do that discovery propfind. This also simplifies #2.
2. Make an OPTIONS request to get the server options, then set the variable
in the session. "ls" would then always know and wouldn't need to do the
propfind.
The additional OPTIONS would be a round trip that would replace the
propfind. It's currently not better than the propfind (it's still a round
trip) but if we also use it for other things, then we could roll in the
deadprop-count discovery in that.
This is another patch though I think. Another note about your solution is
that this extra propfind is not that expensive - we already have 7-8
comparable propfinds, this is simply one more.. and it could be removed on
implementation of the OPTIONS solution eventually. But I agree that the
ultimate goal would be to have fully patched clients and servers be as
efficient as possible, but I'm thinking that in this case the drawback of
unpatched servers would be very dramatic!
Another change that would require an upgrade would be to make the server be
more DeltaV compliant by simply not sending any DeltaV props on an allprop
request (as described in issue #2151). This would mean that all older
clients couldn't do "ls"s against patched servers... but it would make other
dav clients be much faster withe the subversion server.
> To summarise: only patched servers and patched clients have improved
> > speed. Mismatched configurations (old client or server) are not
> > improved but still work. Regular dav
> > clients are still slow.
> >
> > Patch by: Jean-Marc Godbout <jean-marc@computrad.com>
> >
> > Suggested by: Erik Scrafford < erik@scrafford.org>
> > Ben Collins-Sussman <sussman@collab.net>
> > Everyone else who contributed to issue 2151
> >
> >
> > * subversion/mod_dav_svn/liveprops.c
> > (SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
> > (dav_svn_props[]): Added the deadprop-count prop to the list of
> > supported props.
> > (case SVN_PROPID_deadprop_count): Returns the number of deadprops
> > found in the fs for that file.
> >
>
>
> The name of the affected function goes in the parentheses: the 'case'
> shouldn't be in there, but rather "(dav_svn_insert_prop)".
Fixed
> * subversion/libsvn_ra_dav/ra_dav.h
> > (SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
> > (ELEM_deadprop_count): Added the prop to the list of props
> >
> > * subversion/libsvn_ra_dav/props.c
> > (elem_definition, propfind_elements): Added ELEM_deadprop_count
> > to the list of propfind props
> >
> > * subversion/libsvn_ra_dav/fetch.c
> > (dirent_props[]): list of props to get in a dirent PROPFIND
> > (root_props[]): list of props to get in a root PROPFIND
>
>
> Just a nit: when we add a new top-level symbol to the codebase, our
> general convention is to indicate that it's new, e.g.
>
> (dirent_props[]): New array, a list of props to get in a dirent
> PROPFIND.
> (root_props[]): New array, a list of props to get in a root PROPFIND.
Fixed
> (supports_deadprop_count): indicates if the server supports
> > the deadprop_count prop
> > (prop_count): the number of user props for the current dirent
> > as returned by the server
> > ]]]
>
>
> Non-top-level symbols don't get listed in parens. Instead, mention
> the function in which these variables were added, and describe the
> overall change to the function, e.g.
>
> (svn_ra_dav__get_dir): check if server supports 'deadprop-count',
> and if so, request fewer properties.
Done
- Show quoted text -
> >
> >
> >
> > Index: subversion/mod_dav_svn/liveprops.c
> > ===================================================================
> > --- subversion/mod_dav_svn/liveprops.c (revision 15946)
> > +++ subversion/mod_dav_svn/liveprops.c (working copy)
> > @@ -67,7 +67,8 @@
> > enum {
> > SVN_PROPID_baseline_relative_path = 1,
> > SVN_PROPID_md5_checksum,
> > - SVN_PROPID_repository_uuid
> > + SVN_PROPID_repository_uuid,
> > + SVN_PROPID_deadprop_count
> > };
> >
> > static const dav_liveprop_spec dav_svn_props[] =
> > @@ -96,6 +97,7 @@
> > SVN_RO_SVN_PROP(baseline_relative_path, baseline-relative-path),
> > SVN_RO_SVN_PROP(md5_checksum, md5-checksum),
> > SVN_RO_SVN_PROP(repository_uuid, repository-uuid),
> > + SVN_RO_SVN_PROP(deadprop_count, deadprop-count),
> >
> > { 0 } /* sentinel */
> > };
> > @@ -563,6 +565,30 @@
> > }
> > break;
> >
> > + case SVN_PROPID_deadprop_count:
> > + {
> > + if (resource->type != DAV_RESOURCE_TYPE_REGULAR)
> > + return DAV_PROP_INSERT_NOTSUPP;
> > +
> > + unsigned int propcount;
> > +
> > + apr_hash_t *proplist;
>
>
> We write to C89 standards (not C99), which means you can't declare
> variables right in the middle of a block like that. It needs to be
> declared at the beginning of a block.
Fixed. Also, is there a way to have the build fail if the code isn't C89
compliant?
> + serr = svn_fs_node_proplist(&proplist,
> > + resource->info->root.root,
> > + resource->info->repos_path, p);
> > + if (serr != NULL)
> > + {
> > + /* ### what to do? */
> > + svn_error_clear(serr);
> > + value = "###error###";
> > + break;
> > + }
> > +
> > + propcount = apr_hash_count(proplist);
> > + value = apr_psprintf(p, "%u", propcount);
> > + break;
> > + }
> > +
> > default:
> > /* ### what the heck was this property? */
> > return DAV_PROP_INSERT_NOTDEF;
> > Index: subversion/libsvn_ra_dav/ra_dav.h
> > ===================================================================
> > --- subversion/libsvn_ra_dav/ra_dav.h (revision 15946)
> > +++ subversion/libsvn_ra_dav/ra_dav.h (working copy)
> > @@ -368,6 +368,7 @@
> >
> > #define SVN_RA_DAV__PROP_REPOSITORY_UUID SVN_DAV_PROP_NS_DAV
> "repository-uuid"
> >
> > +#define SVN_RA_DAV__PROP_DEADPROP_COUNT "deadprop-count"
>
>
> Our liveprops all live in the SVN_DAV_PROP_NS_DAV namespace; see the
> repository-uuid definition as an example. Just add that constant to
> your declaration.
Done
>
> > typedef struct {
> > /* what is the URL for this resource */
> > @@ -707,7 +708,8 @@
> > ELEM_lock_owner,
> > ELEM_lock_comment,
> > ELEM_lock_creationdate,
> > - ELEM_lock_expirationdate
> > + ELEM_lock_expirationdate,
> > + ELEM_deadprop_count,
> > };
>
>
> In C89, you can't put a comma after the last element in a list. If
> only this were python. :-)
Done,
- Show quoted text -
>
> > /* ### docco */
> > Index: subversion/libsvn_ra_dav/props.c
> > ===================================================================
> > --- subversion/libsvn_ra_dav/props.c (revision 15946)
> > +++ subversion/libsvn_ra_dav/props.c (working copy)
> > @@ -111,6 +111,7 @@
> > { ELEM_baseline_relpath, SVN_RA_DAV__PROP_BASELINE_RELPATH, 1 },
> > { ELEM_md5_checksum, SVN_RA_DAV__PROP_MD5_CHECKSUM, 1 },
> > { ELEM_repository_uuid, SVN_RA_DAV__PROP_REPOSITORY_UUID, 1 },
> > + { ELEM_deadprop_count, SVN_RA_DAV__PROP_DEADPROP_COUNT, 1 },
> > { 0 }
> > };
> >
> > @@ -147,6 +148,8 @@
> > SVN_RA_DAV__XML_CDATA },
> > { SVN_DAV_PROP_NS_DAV, "repository-uuid", ELEM_repository_uuid,
> > SVN_RA_DAV__XML_CDATA },
> > + { SVN_DAV_PROP_NS_DAV, "deadprop-count", ELEM_deadprop_count,
> > + SVN_RA_DAV__XML_CDATA },
> >
> > /* Unknowns */
> > { "", "", ELEM_unknown, SVN_RA_DAV__XML_COLLECT },
> > Index: subversion/libsvn_ra_dav/fetch.c
> > ===================================================================
> > --- subversion/libsvn_ra_dav/fetch.c (revision 15946)
> > +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> > @@ -914,7 +914,25 @@
> > return SVN_NO_ERROR;
> > }
> >
> > +/* the properties we need to fill in an svn_dirent_t, used by
> > + svn_ra_get_dir(). */
> > +static const ne_propname dirent_props[] =
> > +{
> > + { "DAV:", "resourcetype" }, /* kind */
> > + { "DAV:", "getcontentlength" }, /* size */
> > + { "DAV:", "version-name" }, /* created_rev */
> > + { "DAV:", "creationdate" }, /* time */
> > + { "DAV:", "creator-displayname" }, /* last_author */
> > + { SVN_DAV_PROP_NS_DAV, "deadprop-count" }, /* has_props */
> > + { NULL }
> > +};
>
>
> Just for readability, can you make the comments line up?
Yes sir.
>
> > +static const ne_propname root_props[] =
> > +{
> > + { SVN_DAV_PROP_NS_DAV, "deadprop-count" },
> > + { NULL }
> > +};
> > +
>
>
> Can we have a different name for this array? We're not sure what
> 'root' has to do with anything...
>
> Also, can you add a docstring before the array, just like
> dirent_props[] has?
>
> (Also, assuming you agree with our long-term strategy, this array will
> eventually go away, since we won't be doing the extra PROPFIND.)
Good point, I'm sorry about that unfortunate name.. for a while it made
sense in my head. Renamed to deadprop_count_support_props. More descriptive.
Also the array will disappear, yes - whichever strategy is used.
> svn_error_t *svn_ra_dav__get_dir(svn_ra_session_t *session,
> > const char *path,
> > svn_revnum_t revision,
> > @@ -955,13 +973,46 @@
> > *fetched_rev = got_rev;
> > }
> >
> > + /* For issue 2151:
> > + See if we are dealing with a server that understand the
> > + deadprop-count prop. If it doesn't we'll need to do an
> > + allprop PROPFIND - If it does, we'll execute a more
> > + targetted PROPFIND.
> > + */
> > + svn_boolean_t supports_deadprop_count;
> > +
>
>
> Again, new variables need to be declared at the beginning of a block.
Done
> + {
> > + const svn_string_t *deadprop_count;
> > +
> > + SVN_ERR( svn_ra_dav__get_props_resource(&rsrc, ras->sess,
> > + final_url, NULL,
> root_props,
> > + pool) );
> > +
> > + deadprop_count = apr_hash_get(rsrc->propset, "deadprop-
> count", APR_HASH_KEY_STRING);
>
>
> Oops, this line is more than 80 chars. Please fix.
Done
> +
> > + if (deadprop_count == NULL)
> > + supports_deadprop_count = FALSE;
> > + else
> > + supports_deadprop_count = TRUE;
> > + }
> > +
>
>
> Someday this whole block will go away, when we stop doing the extra
> PROPFIND.
Yes.
- Show quoted text -
> if (dirents)
> > {
> > /* Just like Nautilus, Cadaver, or any other browser, we do a
> > PROPFIND on the directory of depth 1. */
> > - SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> > - final_url, NE_DEPTH_ONE,
> > - NULL, NULL /* all props */,
> pool) );
> > + if (supports_deadprop_count == TRUE)
> > + {
> > + SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> > + final_url, NE_DEPTH_ONE,
> > + NULL, dirent_props,
> pool) );
> > + }
> > + else /* The server doesn't support the deadprop_count prop,
> fallback */
> > + {
> > + SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> > + final_url, NE_DEPTH_ONE,
> > + NULL, NULL /* allprops */,
> > + pool) );
> > + }
> >
> > /* Count the number of path components in final_url. */
> > final_url_n_components = svn_path_component_count(final_url);
> > @@ -981,6 +1032,7 @@
> > const svn_string_t *propval;
> > apr_hash_index_t *h;
> > svn_dirent_t *entry;
> > + apr_int64_t prop_count;
> >
> > apr_hash_this (hi, &key, NULL, &val);
> > childname = key;
> > @@ -1009,20 +1061,41 @@
> >
> > /* does this resource contain any 'svn' or 'custom'
> properties,
> > i.e. ones actually created and set by the user? */
> > - for (h = apr_hash_first (pool, resource->propset);
> > - h; h = apr_hash_next (h))
> > + if (supports_deadprop_count == TRUE)
> > {
> > - const void *kkey;
> > - void *vval;
> > - apr_hash_this (h, &kkey, NULL, &vval);
> > + propval = apr_hash_get(resource->propset,
> > +
> SVN_RA_DAV__PROP_DEADPROP_COUNT,
> > + APR_HASH_KEY_STRING);
> > +
> > + if (propval == NULL)
> > + entry->has_props = FALSE;
>
>
> If (supports_deadprop_count == TRUE) and there's no property
> value... that's an error, isn't it? It means the server has sent
> bogus data, no? Shouldn't we treat this as an error condition, rather
> than just setting has_props = FALSE?
My reasoning behind that is that if you were listing something NOT
DAV_RESOURCE_TYPE_REGULAR, then the server wouldn't have a deadprop-count
for that resource. This would mean that has_props should be false. We don't
list anything that's not DAV_RESOURCE_TYPE_REGULAR though, so this is moot.
Changed. I was debating between SVN_ERR_INCOMPLETE_DATA and
SVN_ERR_PROPERTY_NOT_FOUND. I settled on the former as from the point of vue
of the user who would recieve this error it provides a better picture of
what actually happenned (users don't care about properties in this case -
they care about a server sending invalid data). I don't feel too strongly
about this and you might have a better idea.
> + else
> > + {
> > + prop_count = svn__atoui64(propval->data);
> >
> > - if (strncmp((const char *)kkey,
> SVN_DAV_PROP_NS_CUSTOM,
> > - sizeof(SVN_DAV_PROP_NS_CUSTOM) - 1) == 0)
> > - entry->has_props = TRUE;
> > -
> > - else if (strncmp((const char *)kkey,
> SVN_DAV_PROP_NS_SVN,
> > - sizeof(SVN_DAV_PROP_NS_SVN) - 1)
> == 0)
> > - entry->has_props = TRUE;
> > + if (prop_count == 0)
> > + entry->has_props = FALSE;
> > + else
> > + entry->has_props = TRUE;
> > + }
> > + }
> > + else /* The server doesn't support the deadprop_count
> prop, fallback */
> > + {
> > + for (h = apr_hash_first (pool, resource->propset);
>
>
> Whoops! You accidentally inserted a tab character in the line above.
> Make sure your editor isn't doing that. We use only spaces in our
> source code.
Indeed I had changed my editor mid way through my changes. I tried to remove
all tabs. I missed that one evidently. Fixed.
> + h; h = apr_hash_next (h))
> > + {
> > + const void *kkey;
> > + void *vval;
> > + apr_hash_this (h, &kkey, NULL, &vval);
> > +
>
> We know this isn't your code here, but we just noticed that 'vval' is
> never used. Can you clean it up by just removing that variable and
> passing NULL as the last argument to apr_hash_this() ?
Done.
> + if (strncmp((const char *)kkey,
> SVN_DAV_PROP_NS_CUSTOM,
> > + sizeof(SVN_DAV_PROP_NS_CUSTOM) - 1)
> == 0)
> > + entry->has_props = TRUE;
> > +
> > + else if (strncmp((const char *)kkey,
> SVN_DAV_PROP_NS_SVN,
> > + sizeof(SVN_DAV_PROP_NS_SVN) -
> 1) == 0)
> > + entry->has_props = TRUE;
> > + }
> > }
> >
> > /* created_rev & friends */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Sep 1 07:20:57 2005