You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Berlin <db...@dberlin.org> on 2005/11/10 17:33:06 UTC

[wc-propcaching]: Cache existence of certain properties

This patch adds the ability to cache whether certain properties exist in
the entries file.  It does not cache their values.

This massively speeds up operations like update and commit, which need
to know about certain properties on each file or directory.

The implementation simply stores an extra attribute in the entries file,
which is a string containing the names of the properties that are
cached, and exist for a file/directory.

It was decided to use a single string for simplicity, and because using
multiple attributes doesn't actually buy you anything (in any case,
adding or removing to the list of cached properties will require format
upgrades).

The string has no particular ordering properties, and is space
separated.

The list of cached properties is also stored as a single string, comma
separated.  A new function, svn_wc_cached_properties, will return this
list of cached properties.  It is kept as a string again for simplicity.
Only the things updating the props file want to be able to split it into
an array, and do so.  The getter functions simply want to know if the
property they are asking about is in the list, and we can just use
strstr for that.

This code passes all regression tests.

However, the upgrading code for it is not written yet.  Thus, it will
only work properly on clean checkouts.  It looks like an upgrade will
require loading and saving all the props files so the appropriate
properties get cached in the entries.


[[[
Add caching of property existence for a few select properties.

* subversion/include/svn_wc.h
  (struct svn_wc_entry_t): Add has_properties member.
  (svn_wc_cached_properties): New prototype.

* subversion/libsvn_wc/props.c
  (svn_wc__install_props): Set has_properties from the
  from the props we are going to install.
  (svn_wc_prop_get): Short circuit the cached properties
  by checking if they exist before reading the props file.
  (svn_wc_cached_properties): New function.

* subversion/libsvn_wc/entries.c
  (svn_wc__atts_to_entry): Add code to handle has_properties.
  (write_entry): Ditto.
  (fold_entry): Ditto.
  (svn_wc_entry_dup): Ditto.

* subversion/libsvn_wc/entries.h
  (SVN_WC__ENTRY_ATTR_HAS_PROPERTIES): New macro.
  (SVN_WC__ENTRY_MODIFY_HAS_PROPERTIES): New macro.

* subversion/libsvn_wc/log.c
  (svn_wc__loggy_entry_modify): Add code to handle has_properties.
]]]


Re: [wc-propcaching]: Cache existence of certain properties

Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
Daniel Berlin wrote:

>The list of cached properties is also stored as a single string, comma
>separated.  A new function, svn_wc_cached_properties, will return this
>list of cached properties.  It is kept as a string again for simplicity.
>Only the things updating the props file want to be able to split it into
>an array, and do so.  The getter functions simply want to know if the
>property they are asking about is in the list, and we can just use
>strstr for that.
>  
>
While your log message is a little terse, your email is excellent as 
usual. I think it would be very useful if you could put at least this 
paragraph in the log message, or perhaps just a link to your email.

/Tobias


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Daniel Berlin <db...@dberlin.org>.
On Fri, 2005-11-11 at 12:43 -0500, Michael Sinz wrote:
> On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> > On Fri, 2005-11-11 at 10:30 -0500, Michael Sinz wrote:
> > > On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> > > > On Fri, 2005-11-11 at 17:39 +0300, Ivan Zhakov wrote:
> > > [...]
> > > > > I see problem here. For example if in future we will have property
> > > > > "svn:needs-lock" and "svn:need". strstr() will find substring
> > > > > "svn:need" in case when property "svn:needs-lock" exists. I consider
> > > > > this is problem.
> > > >
> > > > Uh, why would we do such a thing?
> > > > Anyway, if you are really concerned, i'll make it split the string into
> > > > an array and search it, but it seems kinda a waste :)
> > >
> > > No, don't do that - just make sure that the string match includes
> > > a terminator (space, null, etc)  (Easy in regexp)
> >
> > Well, then you have to guarantee that nothing strips whitespace from the
> > beginning and end of the string, or add terminators at the end of the
> > string besides null.
> > If we were going to do tha, then i'd rather just split it into an array.
> 
> So, what spaces get stripped?  The ones between the words?  I hope not.
> The ones after the last word? 
yes, possibly.


>  Should not matter.  For example, the
> search in perl for svn:need would be something like:
> 
> if ($data =~ m/(^|\s+)svn:need(\s+|$)/)
> {
>     ...
> }
> 
> (untested, off-the-top-of-my-head)  But this handles any whitespace,
> newline, eol, and end of string.  (Yes, I know the C code does not
> have the regexp but it is relatively trivial to handle multiple delimiters.)

I understand what would be necessary to handle it in C.
without a regexp library, the easiest way to do it is to find a match,
and see if the next character is space or end of string, and if so move
on.
Why bother, when you could also just use a seperator that isn't going to
get stripped, like "," or "|"



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Michael Sinz <Mi...@sinz.org>.
On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> On Fri, 2005-11-11 at 10:30 -0500, Michael Sinz wrote:
> > On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> > > On Fri, 2005-11-11 at 17:39 +0300, Ivan Zhakov wrote:
> > [...]
> > > > I see problem here. For example if in future we will have property
> > > > "svn:needs-lock" and "svn:need". strstr() will find substring
> > > > "svn:need" in case when property "svn:needs-lock" exists. I consider
> > > > this is problem.
> > >
> > > Uh, why would we do such a thing?
> > > Anyway, if you are really concerned, i'll make it split the string into
> > > an array and search it, but it seems kinda a waste :)
> >
> > No, don't do that - just make sure that the string match includes
> > a terminator (space, null, etc)  (Easy in regexp)
>
> Well, then you have to guarantee that nothing strips whitespace from the
> beginning and end of the string, or add terminators at the end of the
> string besides null.
> If we were going to do tha, then i'd rather just split it into an array.

So, what spaces get stripped?  The ones between the words?  I hope not.
The ones after the last word?  Should not matter.  For example, the
search in perl for svn:need would be something like:

if ($data =~ m/(^|\s+)svn:need(\s+|$)/)
{
    ...
}

(untested, off-the-top-of-my-head)  But this handles any whitespace,
newline, eol, and end of string.  (Yes, I know the C code does not
have the regexp but it is relatively trivial to handle multiple delimiters.)


--
Michael Sinz               Technology and Engineering Director/Consultant
"Starting Startups"                          mailto:Michael.Sinz@sinz.org
My place on the web                      http://www.sinz.org/Michael.Sinz

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [wc-propcaching]: Cache existence of certain properties

Posted by Daniel Berlin <db...@dberlin.org>.
On Fri, 2005-11-11 at 10:30 -0500, Michael Sinz wrote:
> On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> > On Fri, 2005-11-11 at 17:39 +0300, Ivan Zhakov wrote:
> [...]
> > > I see problem here. For example if in future we will have property
> > > "svn:needs-lock" and "svn:need". strstr() will find substring
> > > "svn:need" in case when property "svn:needs-lock" exists. I consider
> > > this is problem.
> >
> > Uh, why would we do such a thing?
> > Anyway, if you are really concerned, i'll make it split the string into
> > an array and search it, but it seems kinda a waste :)
> 
> No, don't do that - just make sure that the string match includes
> a terminator (space, null, etc)  (Easy in regexp)

Well, then you have to guarantee that nothing strips whitespace from the
beginning and end of the string, or add terminators at the end of the
string besides null.
If we were going to do tha, then i'd rather just split it into an array.




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Michael Sinz <Mi...@sinz.org>.
On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> On Fri, 2005-11-11 at 17:39 +0300, Ivan Zhakov wrote:
[...]
> > I see problem here. For example if in future we will have property
> > "svn:needs-lock" and "svn:need". strstr() will find substring
> > "svn:need" in case when property "svn:needs-lock" exists. I consider
> > this is problem.
>
> Uh, why would we do such a thing?
> Anyway, if you are really concerned, i'll make it split the string into
> an array and search it, but it seems kinda a waste :)

No, don't do that - just make sure that the string match includes
a terminator (space, null, etc)  (Easy in regexp)

--
Michael Sinz               Technology and Engineering Director/Consultant
"Starting Startups"                          mailto:Michael.Sinz@sinz.org
My place on the web                      http://www.sinz.org/Michael.Sinz

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [wc-propcaching]: Cache existence of certain properties

Posted by Daniel Berlin <db...@dberlin.org>.
On Fri, 2005-11-11 at 17:39 +0300, Ivan Zhakov wrote:
> On 11/10/05, Daniel Berlin <db...@dberlin.org> wrote:
> > This patch adds the ability to cache whether certain properties exist in
> > the entries file.  It does not cache their values.
> >
> > This massively speeds up operations like update and commit, which need
> > to know about certain properties on each file or directory.
> >
> > The implementation simply stores an extra attribute in the entries file,
> > which is a string containing the names of the properties that are
> > cached, and exist for a file/directory.
> >
> > It was decided to use a single string for simplicity, and because using
> > multiple attributes doesn't actually buy you anything (in any case,
> > adding or removing to the list of cached properties will require format
> > upgrades).
> >
> > The string has no particular ordering properties, and is space
> > separated.
> >
> > The list of cached properties is also stored as a single string, comma
> > separated.  A new function, svn_wc_cached_properties, will return this
> > list of cached properties.  It is kept as a string again for simplicity.
> > Only the things updating the props file want to be able to split it into
> > an array, and do so.  The getter functions simply want to know if the
> > property they are asking about is in the list, and we can just use
> > strstr for that.
> >
> I see problem here. For example if in future we will have property
> "svn:needs-lock" and "svn:need". strstr() will find substring
> "svn:need" in case when property "svn:needs-lock" exists. I consider
> this is problem.

Uh, why would we do such a thing?
Anyway, if you are really concerned, i'll make it split the string into
an array and search it, but it seems kinda a waste :)

> 
> PS: Also you patch contain tab symbols, Subversion team don't use tab
> spaces in sourcecode.
> 
Yeah, i fixed this already (my emacs regexp wasn't catching this dir).

> --
> Ivan Zhakov


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Ivan Zhakov <ch...@gmail.com>.
On 11/10/05, Daniel Berlin <db...@dberlin.org> wrote:
> This patch adds the ability to cache whether certain properties exist in
> the entries file.  It does not cache their values.
>
> This massively speeds up operations like update and commit, which need
> to know about certain properties on each file or directory.
>
> The implementation simply stores an extra attribute in the entries file,
> which is a string containing the names of the properties that are
> cached, and exist for a file/directory.
>
> It was decided to use a single string for simplicity, and because using
> multiple attributes doesn't actually buy you anything (in any case,
> adding or removing to the list of cached properties will require format
> upgrades).
>
> The string has no particular ordering properties, and is space
> separated.
>
> The list of cached properties is also stored as a single string, comma
> separated.  A new function, svn_wc_cached_properties, will return this
> list of cached properties.  It is kept as a string again for simplicity.
> Only the things updating the props file want to be able to split it into
> an array, and do so.  The getter functions simply want to know if the
> property they are asking about is in the list, and we can just use
> strstr for that.
>
I see problem here. For example if in future we will have property
"svn:needs-lock" and "svn:need". strstr() will find substring
"svn:need" in case when property "svn:needs-lock" exists. I consider
this is problem.

PS: Also you patch contain tab symbols, Subversion team don't use tab
spaces in sourcecode.

--
Ivan Zhakov

Re: [wc-propcaching]: Cache existence of certain properties

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 10 Nov 2005, Daniel Berlin wrote:

>
> >
> > The problem with using a string for this is space. An apr_uint32_t
> > with bits for signle props would be more space-efficient than a little
> > string for each entry. NOt that we are terribly space-efficient for
> > the entry cache, but that's another topic.
>
> This is true, and if you want, i'll move it to a uint32.
> But i think we should still write it out as a string.
>

This was discussed on IRC and I concluded that I can live with a
space-separated string (also see Dan's follow-up).  Anyone else dislikes
this?

> > Why do we need this as a public API. I'd like to keep this as an
> > implementation detail.
>
> Okay, that's fine.

There might actually be a reason for a user application to know if a
certain property is cached. I'm thinking of svn:needs-lock.  GUI might
think this is too expensive to retrieve for each file in a directory
listing if it is not cached and use the filesystems read-only state
instead. In that case this is dependent of the actual working copy. I
don't know if this is needed in practice. Let's wait until the need
arises. It is trivial to implement in that case.


> > >
> > > That TODO is for the above:-) But, sadly svn_wc__install_props is not
> > >  the only place where we muck with the properties.
>
> If you stare about, you will discover this really is the only place that
> seems to need to be changed.
> The other places you might think need to changed (like prop_set2) call
> install_props.
>
Discussed on IRC as well. I think revert_admin_things was the only other
place.


> > > +
> > > +const char *
> > > +svn_wc_cached_properties (void)
> > > +{
> > > +  return SVN_PROP_SPECIAL "," SVN_PROP_EXTERNALS "," SVN_PROP_NEEDS_LOCK;
> >
> > This list can be tweaked during the 1.4 cycle.
>
> Every time you add to the list, you will break existing wc's with newer
> clients.  Every time you delete from the list, you will break newer wc's
> with older clients. :)
>
Yes. That's why we'll need another wc format bump during the 1.4 cycle.

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Daniel Berlin <db...@dberlin.org>.
> 
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h	(revision 17287)
> > +++ subversion/include/svn_wc.h	(working copy)
> > @@ -1216,6 +1216,10 @@ typedef struct svn_wc_entry_t
> >     * @since New in 1.4. */
> >    svn_boolean_t prop_mods;
> >
> > +  /** Cached property existence for this entry.
> > +   * @since New in 1.4. */
> > +  const char *has_properties;
> > +
> 
> The problem with using a string for this is space. An apr_uint32_t
> with bits for signle props would be more space-efficient than a little
> string for each entry. NOt that we are terribly space-efficient for
> the entry cache, but that's another topic.

This is true, and if you want, i'll move it to a uint32.
But i think we should still write it out as a string.


> 
> > +/** Return a comma separated string containing the names of all of the
> > + *  properties that the working copy caches the existence of.
> > + *  @since New in 1.4.
> > + */
> > +const char *svn_wc_cached_properties (void);
> > +
> 
> Why do we need this as a public API. I'd like to keep this as an
> implementation detail.

Okay, that's fine.
So do you want this to be an int, or a string?


> 
> >
> >  #ifdef __cplusplus
> >  }
> > Index: subversion/libsvn_wc/props.c
> > ===================================================================
> > --- subversion/libsvn_wc/props.c	(revision 17287)
> > +++ subversion/libsvn_wc/props.c	(working copy)
> > @@ -314,9 +314,12 @@ svn_wc__install_props (svn_stringbuf_t *
> >    const char *full_path;
> >    const char *access_path = svn_wc_adm_access_path (adm_access);
> >    int access_len = strlen (access_path);
> > +  apr_array_header_t *cached_props;
> >    apr_array_header_t *prop_diffs;
> >    svn_wc_entry_t tmp_entry;
> >    svn_node_kind_t kind;
> > +  char *has_properties_string = NULL;
> > +  int i;
> >
> >    /* Non-empty path without trailing slash need an extra slash removed */
> >    if (access_len != 0 && access_path[access_len - 1] != '/')
> > @@ -342,11 +345,32 @@ svn_wc__install_props (svn_stringbuf_t *
> >    SVN_ERR (svn_prop_diffs (&prop_diffs, working_props, base_props, pool));
> >    tmp_entry.prop_mods = (prop_diffs->nelts > 0);
> >
> > +
> > +
> > +  cached_props = svn_cstring_split (svn_wc_cached_properties (), ",",
> > +				    TRUE, pool);
> > +  for (i = 0; i < cached_props->nelts; i++)
> > +    {
> > +      const char *proptolookfor  = APR_ARRAY_IDX (cached_props, i, const char *);
> > +      if (apr_hash_get (working_props, proptolookfor, APR_HASH_KEY_STRING) != NULL)
> > +	{
> > +	  if (!has_properties_string)
> > +	    has_properties_string = "";
> > +	  has_properties_string = apr_pstrcat (pool, has_properties_string,
> > +					       proptolookfor, " ", NULL);
> > +	}
> > +    }
> > +
> > +  tmp_entry.has_properties = has_properties_string;
> > +
> >    /* ### TODO: Check props in working props and update flags for specific
> >       props when we have such flags. */
> >
> > That TODO is for the above:-) But, sadly svn_wc__install_props is not
> >  the only place where we muck with the properties.

If you stare about, you will discover this really is the only place that
seems to need to be changed.
The other places you might think need to changed (like prop_set2) call
install_props.

So where else are you thinking we need to change?

> We can be even smarter here for boolean props. Their existence implies
> that they have the value "*", so we don't need to load the property
> file at all in that case. (We could even avoid putting this in the
> properties file at all in this case. Whether that complication is
> worth it I don't know.)

True.  I didn't bother with this yet.

> 
> >    if (kind == svn_prop_wc_kind)
> >      {
> >        return svn_wc__wcprop_get (value, name, path, adm_access, pool);
> > @@ -2147,3 +2201,9 @@ svn_wc__has_magic_property (apr_array_he
> >      }
> >    return FALSE;
> >  }
> > +
> > +const char *
> > +svn_wc_cached_properties (void)
> > +{
> > +  return SVN_PROP_SPECIAL "," SVN_PROP_EXTERNALS "," SVN_PROP_NEEDS_LOCK;
> 
> This list can be tweaked during the 1.4 cycle. 

Every time you add to the list, you will break existing wc's with newer
clients.  Every time you delete from the list, you will break newer wc's
with older clients. :)


> Externals seem good. I
> guess it speeds up update and status?

Yes.




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Daniel Berlin <db...@dberlin.org>.
On Thu, 2005-11-10 at 21:44 +0100, Peter N. Lundblad wrote:
> On Thu, 10 Nov 2005, Daniel Berlin wrote:
> 
> > This patch adds the ability to cache whether certain properties exist in
> > the entries file.  It does not cache their values.
> >
> There are stylistic stuff, but I see this as a prototype.
> 
> 
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h	(revision 17287)
> > +++ subversion/include/svn_wc.h	(working copy)
> > @@ -1216,6 +1216,10 @@ typedef struct svn_wc_entry_t
> >     * @since New in 1.4. */
> >    svn_boolean_t prop_mods;
> >
> > +  /** Cached property existence for this entry.
> > +   * @since New in 1.4. */
> > +  const char *has_properties;
> > +
> 
> The problem with using a string for this is space. An apr_uint32_t
> with bits for signle props would be more space-efficient than a little
> string for each entry. NOt that we are terribly space-efficient for
> the entry cache, but that's another topic.

Actually, i'm not sure this makes sense, thinking harder about it.

Everywhere else treats props as strings, and compares prop names against
strings.  I don't really think it's worth the 10 bytes per prop name you
are going to save to 

1. Establish a parallel mapping between property names and ints,
including a lookup table.
2. Lookup the name everywhere just to get the int.
3. Then store it back in the entries file as a string.

Seems like an awful lot of work just to save ~15 bytes per entry.

Even if you had 40,000 entries all kept in cache at once, you are
talking, this isn't even a megabyte of saved memory.

--Dan


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 10 Nov 2005, Daniel Berlin wrote:

> This patch adds the ability to cache whether certain properties exist in
> the entries file.  It does not cache their values.
>
There are stylistic stuff, but I see this as a prototype.


> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h	(revision 17287)
> +++ subversion/include/svn_wc.h	(working copy)
> @@ -1216,6 +1216,10 @@ typedef struct svn_wc_entry_t
>     * @since New in 1.4. */
>    svn_boolean_t prop_mods;
>
> +  /** Cached property existence for this entry.
> +   * @since New in 1.4. */
> +  const char *has_properties;
> +

The problem with using a string for this is space. An apr_uint32_t
with bits for signle props would be more space-efficient than a little
string for each entry. NOt that we are terribly space-efficient for
the entry cache, but that's another topic.


> +/** Return a comma separated string containing the names of all of the
> + *  properties that the working copy caches the existence of.
> + *  @since New in 1.4.
> + */
> +const char *svn_wc_cached_properties (void);
> +

Why do we need this as a public API. I'd like to keep this as an
implementation detail.

>
>  #ifdef __cplusplus
>  }
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c	(revision 17287)
> +++ subversion/libsvn_wc/props.c	(working copy)
> @@ -314,9 +314,12 @@ svn_wc__install_props (svn_stringbuf_t *
>    const char *full_path;
>    const char *access_path = svn_wc_adm_access_path (adm_access);
>    int access_len = strlen (access_path);
> +  apr_array_header_t *cached_props;
>    apr_array_header_t *prop_diffs;
>    svn_wc_entry_t tmp_entry;
>    svn_node_kind_t kind;
> +  char *has_properties_string = NULL;
> +  int i;
>
>    /* Non-empty path without trailing slash need an extra slash removed */
>    if (access_len != 0 && access_path[access_len - 1] != '/')
> @@ -342,11 +345,32 @@ svn_wc__install_props (svn_stringbuf_t *
>    SVN_ERR (svn_prop_diffs (&prop_diffs, working_props, base_props, pool));
>    tmp_entry.prop_mods = (prop_diffs->nelts > 0);
>
> +
> +
> +  cached_props = svn_cstring_split (svn_wc_cached_properties (), ",",
> +				    TRUE, pool);
> +  for (i = 0; i < cached_props->nelts; i++)
> +    {
> +      const char *proptolookfor  = APR_ARRAY_IDX (cached_props, i, const char *);
> +      if (apr_hash_get (working_props, proptolookfor, APR_HASH_KEY_STRING) != NULL)
> +	{
> +	  if (!has_properties_string)
> +	    has_properties_string = "";
> +	  has_properties_string = apr_pstrcat (pool, has_properties_string,
> +					       proptolookfor, " ", NULL);
> +	}
> +    }
> +
> +  tmp_entry.has_properties = has_properties_string;
> +
>    /* ### TODO: Check props in working props and update flags for specific
>       props when we have such flags. */
>
That TODO is for the above:-) But, sadly svn_wc__install_props is not
>  the only place where we muck with the properties. There are more
>  places to be updated, but let's discuss the general design
>  first. So, this needs to be factored out in a separate function.
>  Long-term we want to minimize the number of places that touches
>  properties, as per Zhakov's proposed property abstraction API.

> @@ -1302,9 +1346,19 @@ svn_wc_prop_get (const svn_string_t **va
>  {
>    svn_error_t *err;
>    apr_hash_t *prophash;
> -
>    enum svn_prop_kind kind = svn_property_kind (NULL, name);
>
> +  if (strstr (svn_wc_cached_properties (), name))
> +    {
> +      const svn_wc_entry_t *entry;
> +      SVN_ERR (svn_wc_entry (&entry, path, adm_access, TRUE, pool));
> +      if (!entry->has_properties || !strstr (entry->has_properties, name))
> +	{
> +	  *value = NULL;
> +	  return SVN_NO_ERROR;
> +	}
> +    }
> +

We can be even smarter here for boolean props. Their existence implies
that they have the value "*", so we don't need to load the property
file at all in that case. (We could even avoid putting this in the
properties file at all in this case. Whether that complication is
worth it I don't know.)

>    if (kind == svn_prop_wc_kind)
>      {
>        return svn_wc__wcprop_get (value, name, path, adm_access, pool);
> @@ -2147,3 +2201,9 @@ svn_wc__has_magic_property (apr_array_he
>      }
>    return FALSE;
>  }
> +
> +const char *
> +svn_wc_cached_properties (void)
> +{
> +  return SVN_PROP_SPECIAL "," SVN_PROP_EXTERNALS "," SVN_PROP_NEEDS_LOCK;

This list can be tweaked during the 1.4 cycle. Externals seem good. I
guess it speeds up update and status?

Thanks for this prototype, which confirms the improvements we hoped
for:-)

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Daniel Berlin <db...@dberlin.org>.
On Fri, 2005-11-11 at 11:16 -0800, Jim Blandy wrote:
> On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> > On Fri, 2005-11-11 at 10:26 -0800, Jim Blandy wrote:
> > > What happens if a property name contains a comma or a space?
> >
> > Can't, we don't allow them, for good reason
> 
> Okay.  Is this mentioned in the book somewhere? 
I don't believe so.

I actually had to go checking the code to make sure this was a real
verification check, and not some command line limitation.



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Jim Blandy <ji...@red-bean.com>.
On 11/11/05, Daniel Berlin <db...@dberlin.org> wrote:
> On Fri, 2005-11-11 at 10:26 -0800, Jim Blandy wrote:
> > What happens if a property name contains a comma or a space?
>
> Can't, we don't allow them, for good reason

Okay.  Is this mentioned in the book somewhere?  I looked for it
before I posted.
If not, I'll write it up.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: [wc-propcaching]: Cache existence of certain properties

Posted by Daniel Berlin <db...@dberlin.org>.
On Fri, 2005-11-11 at 10:26 -0800, Jim Blandy wrote:
> What happens if a property name contains a comma or a space?

Can't, we don't allow them, for good reason

dberlin@linux:~/svn-propcaching> svn propset "foo bar" 5 .
svn: Bad property name: 'foo bar'
dberlin@linux:~/svn-propcaching> svn propset "foo|bar" 5 .
svn: Bad property name: 'foo|bar'
dberlin@linux:~/svn-propcaching> svn propset "foo,bar" 5 .
svn: Bad property name: 'foo,bar'

They aren't allow in XML (which is what the entries file is stored in),
and XML does not allow tag names or attribute names to contain
whitespace (http://www.w3.org/TR/REC-xml/#NT-NameChar)

So not using a single string really doesn't help you here,  even if *we
did relax that restriction on property names* because you can't have
random property-names as XML tags or attribute names anyway, which would
be the next most obvious way to do it.


FYI: At Zhakov's suggestion, the separator being used is actually now
"|", not comma or space, since it looks prettier :).

--Dan


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Ivan Zhakov <ch...@gmail.com>.
On 11/11/05, Jim Blandy <ji...@red-bean.com> wrote:
> What happens if a property name contains a comma or a space?
Anyway we are talking about "svn:" properties, so we could be smart
and make it without comma or space.

--
Ivan Zhakov

Re: [wc-propcaching]: Cache existence of certain properties

Posted by Jim Blandy <ji...@red-bean.com>.
What happens if a property name contains a comma or a space?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org