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?
> 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)".
> * 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.
> (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.
>
>
>
> 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.
> + 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.
>
> 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. :-)
>
> /* ### 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?
>
> +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.)
> 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.
> + {
> + 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.
> +
> + 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.
> 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?
> + 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.
> + 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() ?
> + 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 */
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 30 20:40:00 2005