On 13 April 2015 at 15:23, <rhuij...@apache.org> wrote: > Author: rhuijben > Date: Mon Apr 13 12:23:20 2015 > New Revision: 1673170 > > URL: http://svn.apache.org/r1673170 > Log: > Add an fs layer api that allows obtaining just a boolean indicating whether > properties exist on a node, instead of always obtaining the properties and > checking their count. > > This is by far the most expensive operation on 'svn ls -v' in Subversion <= > 1.8.x on huge directories as it requires fetching much 'new' data, and has > the risk of trashing the node cache. > > r1673153 made new 'svn' clients stop asking for this information for this > scenario but existing clients do ask and so will most likely many third > party clients (confirmed for TortoiseSVN), will keep asking for this > information. > > This function introduces the FS api and updates callers, but doesn't provide > optimized implementations yet, so the result is that this doesn't change > runtime behaviour yet, but just moves the implementation into the fs layer. > > I hope this patch will be accepted for 1.9.0 to allow further improvements > in later patches, potentially after 1.9.0. > Hi Bert,
It looks like a interesting optimization, but I've some concerns about mod_dav_svn protocol part. See my review inline. > Modified: subversion/trunk/subversion/mod_dav_svn/liveprops.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/liveprops.c?rev=1673170&r1=1673169&r2=1673170&view=diff > ============================================================================== > --- subversion/trunk/subversion/mod_dav_svn/liveprops.c (original) > +++ subversion/trunk/subversion/mod_dav_svn/liveprops.c Mon Apr 13 12:23:20 > 2015 > @@ -787,30 +787,58 @@ insert_prop_internal(const dav_resource > > case SVN_PROPID_deadprop_count: > { > - unsigned int propcount; > - apr_hash_t *proplist; > - > if (resource->type != DAV_RESOURCE_TYPE_REGULAR) > return DAV_PROP_INSERT_NOTSUPP; > > - serr = svn_fs_node_proplist(&proplist, > - resource->info->root.root, > - resource->info->repos_path, > scratch_pool); > - if (serr != NULL) > + if (resource->info->repos->is_svn_client) > { > - ap_log_rerror(APLOG_MARK, APLOG_ERR, serr->apr_err, > - resource->info->r, > - "Can't fetch proplist of '%s': " > - "%s", > - resource->info->repos_path, > - serr->message); > - svn_error_clear(serr); > - value = error_value; > - break; > + svn_boolean_t has_props; > + /* Retrieving the actual properties is quite expensive while > + svn clients only want to know if there are properties, and > + in many cases aren't interested at all (see r1673153) */ > + serr = svn_fs_node_has_props(&has_props, > + resource->info->root.root, > + resource->info->repos_path, > + scratch_pool); > + > + if (serr != NULL) > + { > + ap_log_rerror(APLOG_MARK, APLOG_ERR, serr->apr_err, > + resource->info->r, > + "Can't fetch has props of '%s': " > + "%s", > + resource->info->repos_path, > + serr->message); > + svn_error_clear(serr); > + value = error_value; > + break; > + } > + > + value = has_props ? "99" /* Magic (undocumented) value */ : "0"; > } > + else > + { > + unsigned int propcount; > + apr_hash_t *proplist; > + serr = svn_fs_node_proplist(&proplist, > + resource->info->root.root, > + resource->info->repos_path, > scratch_pool); > + if (serr != NULL) > + { > + ap_log_rerror(APLOG_MARK, APLOG_ERR, serr->apr_err, > + resource->info->r, > + "Can't fetch proplist of '%s': " > + "%s", > + resource->info->repos_path, > + serr->message); > + svn_error_clear(serr); > + value = error_value; > + break; > + } As far I understand the new server will return magic value (99) for dead-prop-count DAV property for all Subversion clients. Is it correct? It looks like hacky approach and it could break backward compatibility. I'm aware that currently we don't use deadprop-count other than check for non-zero, but I don't think we should change server-side behavior anyway. The proper solution would be add new DAV property like "has-dead-props", advertise it support using capability header and then request it from client instead of "deadprop-count". What do you think? -- Ivan Zhakov
