The log message for r18513 (which resolved issue #2151, "'svn ls' is
slow over ra_dav") makes me wonder something:
> ------------------------------------------------------------------------
> r18513 | rooneg | 2006-02-17 17:52:54 -0600 (Fri, 17 Feb 2006) | 45 lines
>
> 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.
Instead of adding an unconditional extra turnaround, why not just
issue the request we want right away, and see if it succeeds or not?
If it doesn't, we can just fall back to the old behavior.
I might dig deeper if I get time, but before I go down that road, does
anyone know off the top of their head why the usual try-then-fall-back
strategy wouldn't work in this case? Its advantage is that it only
does the extra network turnaround in the legacy case.
Here's the relevant part of the change:
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> --- subversion/libsvn_ra_dav/fetch.c (revision 18512)
> +++ subversion/libsvn_ra_dav/fetch.c (revision 18513)
> @@ -922,6 +922,13 @@
> return SVN_NO_ERROR;
> }
>
> +/* the property we need to fetch to see if the server we are
> + connected to supports the deadprop-count property. */
> +static const ne_propname deadprop_count_support_props[] =
> +{
> + { SVN_DAV_PROP_NS_DAV, "deadprop-count" },
> + { NULL }
> +};
>
> svn_error_t *svn_ra_dav__get_dir(svn_ra_session_t *session,
> const char *path,
> @@ -937,6 +944,7 @@
> apr_hash_t *resources;
> const char *final_url;
> apr_size_t final_url_n_components;
> + svn_boolean_t supports_deadprop_count;
> svn_ra_dav__session_t *ras = session->priv;
> const char *url = svn_path_url_add_component(ras->url->data, path, pool);
>
> @@ -964,13 +972,39 @@
> *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.
> + */
> +
> + {
> + const svn_string_t *deadprop_count;
> +
> + SVN_ERR(svn_ra_dav__get_props_resource(&rsrc, ras->sess,
> + final_url, NULL,
> + deadprop_count_support_props,
> + pool));
> +
> + deadprop_count = apr_hash_get(rsrc->propset,
> + SVN_RA_DAV__PROP_DEADPROP_COUNT,
> + APR_HASH_KEY_STRING);
> +
> + if (deadprop_count == NULL)
> + supports_deadprop_count = FALSE;
> + else
> + supports_deadprop_count = TRUE;
> + }
> +
> if (dirents)
> {
> ne_propname *which_props;
>
> /* if we didn't ask for the has_props field, we can get individual
> properties. */
> - if ((SVN_DIRENT_HAS_PROPS & dirent_fields) == 0)
> + if ((SVN_DIRENT_HAS_PROPS & dirent_fields) == 0
> + || supports_deadprop_count)
> {
> int num_props = 1; /* start with one for the final NULL */
>
> @@ -980,6 +1014,9 @@
> if (dirent_fields & SVN_DIRENT_SIZE)
> ++num_props;
>
> + if (dirent_fields & SVN_DIRENT_HAS_PROPS)
> + ++num_props;
> +
> if (dirent_fields & SVN_DIRENT_CREATED_REV)
> ++num_props;
>
> @@ -1012,6 +1049,12 @@
> which_props[num_props--].name = "getcontentlength";
> }
>
> + if (dirent_fields & SVN_DIRENT_HAS_PROPS)
> + {
> + which_props[num_props].nspace = SVN_DAV_PROP_NS_DAV;
> + which_props[num_props--].name = "deadprop-count";
> + }
> +
> if (dirent_fields & SVN_DIRENT_CREATED_REV)
> {
> which_props[num_props].nspace = "DAV:";
> @@ -1062,6 +1105,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;
> @@ -1099,22 +1143,51 @@
> {
> /* 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);
> -
> - 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;
> + propval = apr_hash_get(resource->propset,
> + SVN_RA_DAV__PROP_DEADPROP_COUNT,
> + APR_HASH_KEY_STRING);
> +
> + if (propval == NULL)
> + {
> + /* we thought that the server supported the
> + deadprop-count property. apparently not. */
> + return svn_error_create(SVN_ERR_INCOMPLETE_DATA, NULL,
> + _("Server response missing the "
> + "expected deadprop-count "
> + "property"));
> + }
> + else
> + {
> + prop_count = svn__atoui64(propval->data);
> + 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);
> + h; h = apr_hash_next(h))
> + {
> + const void *kkey;
> + void *vval;
> + apr_hash_this(h, &kkey, NULL, &vval);
> +
> + 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 (dirent_fields & SVN_DIRENT_CREATED_REV)
> {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Mar 6 21:40:48 2006