[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1079588 - in /subversion/trunk/subversion/libsvn_wc: wc_db_pdh.c wc_db_private.h

From: Greg Stein <gstein_at_gmail.com>
Date: Tue, 8 Mar 2011 20:45:49 -0500

On Tue, Mar 8, 2011 at 17:40, <hwright_at_apache.org> wrote:
> Author: hwright
> Date: Tue Mar  8 22:40:09 2011
> New Revision: 1079588
>
> URL: http://svn.apache.org/viewvc?rev=1079588&view=rev
> Log:
> Update the wc-ng stat caching code to use the well-established stringbuf,
> rather than reinventing said wheel.

Cool.

>...
> +++ subversion/trunk/subversion/libsvn_wc/wc_db_pdh.c Tue Mar  8 22:40:09 2011
> @@ -97,28 +97,34 @@ get_path_kind(svn_wc__db_t *db,
>               apr_pool_t *scratch_pool)
>  {
>   svn_boolean_t special;
> -  size_t abspath_size;
>
>   /* This implements a *really* simple LRU cache, where "simple" is defined
>      as "only one element".  In other words, we remember the most recently
>      queried path, and nothing else.  This gives >80% cache hits. */
>
>   if (db->parse_cache.abspath
> -        && strcmp(db->parse_cache.abspath, local_abspath) == 0)
> +        && strcmp(db->parse_cache.abspath->data, local_abspath) == 0)
>     {
>       /* Cache hit! */
>       *kind = db->parse_cache.kind;
>       return SVN_NO_ERROR;
>     }
>
> -  abspath_size = strlen(local_abspath);
> -  if (abspath_size >= db->parse_cache.size)
> +  if (!db->parse_cache.abspath)
>     {
> -      db->parse_cache.size = abspath_size * 2 + 1;
> -      db->parse_cache.abspath = apr_palloc(db->state_pool,
> -                                           db->parse_cache.size);
> +      db->parse_cache.abspath = svn_stringbuf_create(local_abspath,
> +                                                     db->state_pool);
> +    }
> +  else
> +    {
> +      size_t abspath_size = strlen(local_abspath);
> +
> +      if (abspath_size < db->parse_cache.abspath->blocksize)
> +        svn_stringbuf_set(db->parse_cache.abspath, local_abspath);
> +      else
> +        db->parse_cache.abspath = svn_stringbuf_ncreate(local_abspath,
> +                                         abspath_size * 2 + 1, db->state_pool);

Hunh? Why are you creating a NEW stringbuf? The whole point of
stringbuf is that it reallocs in place. It manages the size and
everything.

I don't have the testing setup right now, or I'd just go make the
changes, but that's never as good as shared knowledge.

So. First thing: just embed the svn_stringbuf_t in the db structure.
No need for a pointer-to-stringbuf. Just make sure it is zeroed out
during initialization (ie. apr_pcalloc()).

Then, all you need to do is one line:

  svn_stringbuf_set(&db->parse_cache.abspath, local_abspath);

That will replace your entire if/else block above. No strlen. No
recreation of a stringbuf. No separate set(). No initial creation...
One line.

>...
> +++ subversion/trunk/subversion/libsvn_wc/wc_db_private.h Tue Mar  8 22:40:09 2011
> @@ -56,8 +56,7 @@ struct svn_wc__db_t {
>      get_path_kind() for use. */
>   struct
>   {
> -    char *abspath;
> -    size_t size;
> +    svn_stringbuf_t *abspath;

Per above, remove the ptr here, and ensure that init will set the
stringbuf to all zeroes.

Cheers,
-g
Received on 2011-03-09 02:46:20 CET

This is an archived mail posted to the Subversion Dev mailing list.