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

Re: svn commit: r1078914 - /subversion/trunk/subversion/libsvn_wc/wc_db_pdh.c

From: Hyrum K Wright <hyrum_at_hyrumwright.org>
Date: Mon, 7 Mar 2011 15:34:43 -0600

On Mon, Mar 7, 2011 at 3:03 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> hwright_at_apache.org wrote on Mon, Mar 07, 2011 at 20:03:37 -0000:
>> Author: hwright
>> Date: Mon Mar  7 20:03:37 2011
>> New Revision: 1078914
>>
>> URL: http://svn.apache.org/viewvc?rev=1078914&view=rev
>> Log:
>> Reduce the number of stat()s in wc-ng by implementing a very simple LRU cache.
>> This could probably be improved upon, and I welcome such improvements, but
>> even with this implementation, we get a ~10% decrease in test run times (on
>> a non-ram-disk test platform).
>>
>> In addition, we get a 82.7% cache hit rate when running basic_tests.py, and
>> a 82.0% hit rate throughout the entire test suite.
>>
>> * subversion/libsvn_wc/wc_db_pdh.c
>>   (get_path_kind): New.
>>   (svn_wc__db_wcroot_parse_local_abaspath): Use the caching function to get
>>     node kinds.
>>
>> Modified:
>>     subversion/trunk/subversion/libsvn_wc/wc_db_pdh.c
>>
>> Modified: subversion/trunk/subversion/libsvn_wc/wc_db_pdh.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db_pdh.c?rev=1078914&r1=1078913&r2=1078914&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_wc/wc_db_pdh.c (original)
>> +++ subversion/trunk/subversion/libsvn_wc/wc_db_pdh.c Mon Mar  7 20:03:37 2011
>> @@ -85,6 +85,55 @@ get_old_version(int *version,
>>  }
>>
>>
>> +/* A helper function to parse_local_abspath() which returns the on-disk KIND
>> +   of LOCAL_ABSPATH, using DB and SCRATCH_POOL as needed.
>> +
>> +   This function may do strange things, but at long as it comes up with the
>> +   Right Answer, we should be happy.  */
>> +static svn_error_t *
>> +get_path_kind(svn_wc__db_t *db,
>> +              const char *local_abspath,
>> +              svn_node_kind_t *kind,
>> +              apr_pool_t *scratch_pool)
>> +{
>> +  static char *cached_abspath = NULL;
>> +  static size_t cache_size = 0;
>> +  static svn_node_kind_t cached_kind;
>> +
>> +  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.
>> +
>
> Nice!
>
>> +     Using malloc()/free() divorces us from the db->state_pool, and a set
>> +     of possible memory leaks associated with using it.  */
>> +
>> +  if (cached_abspath && strcmp(cached_abspath, local_abspath) == 0)
>> +    {
>> +      /* Cache hit! */
>> +      *kind = cached_kind;
>> +      return SVN_NO_ERROR;
>> +    }
>> +
>> +  abspath_size = strlen(local_abspath);
>> +  if (abspath_size > cache_size)
>> +    {
>> +      free(cached_abspath);
>
> So you're free()ing a the result of strcpy().  Possibly weird question,
> but: is this permitted?  Nothing in strcpy(3) says it is.

strcpy() doesn't allocate memory, just duplicates the contents into a
pre-allocated buffer. The memory being free()'d is allocated by
malloc().

>> +      cache_size = abspath_size * 2;
>> +      cached_abspath = malloc(cache_size);
>> +    }
>> +  strcpy(cached_abspath, local_abspath);
>> +
>
> If local_abspath is the empty string, then abspath_size would be zero
> and the allocated buffer would be zero bytes --- but then the strcpy()
> would try to fill a NUL character into that buffer.
>
> So, in order to support platforms where "" is a valid abspath, I think
> you should set the cache size to 2*abspath_size+1.

You do point out an error, but only in some scenarios. cache_size
will only grow, it won't shrink, so the above could only happen if ""
is used upon the first invocation. (Or, alternatively, if a path of
size 2 * cache_size were used.) r1078960 fixes this off-by-one error.

>> +  SVN_ERR(svn_io_check_special_path(local_abspath, &cached_kind,
>> +                                    &special /* unused */, scratch_pool));
>> +  *kind = cached_kind;
>> +
>> +  return SVN_NO_ERROR;
>> +}
>> +
>> +
>>  /* */
>>  static svn_error_t *
>>  verify_no_work(svn_sqlite__db_t *sdb)
>> @@ -307,7 +356,6 @@ svn_wc__db_wcroot_parse_local_abspath(sv
>>    const char *local_dir_abspath;
>>    const char *original_abspath = local_abspath;
>>    svn_node_kind_t kind;
>> -  svn_boolean_t special;
>>    const char *build_relpath;
>>    svn_wc__db_wcroot_t *probe_wcroot;
>>    svn_wc__db_wcroot_t *found_wcroot = NULL;
>> @@ -351,8 +399,7 @@ svn_wc__db_wcroot_parse_local_abspath(sv
>>       ### rid of this stat() call. it is going to happen for EVERY call
>>       ### into wc_db which references a file. calls for directories could
>>       ### get an early-exit in the hash lookup just above.  */
>> -  SVN_ERR(svn_io_check_special_path(local_abspath, &kind,
>> -                                    &special /* unused */, scratch_pool));
>> +  SVN_ERR(get_path_kind(db, local_abspath, &kind, scratch_pool));
>>    if (kind != svn_node_dir)
>>      {
>>        /* If the node specified by the path is NOT present, then it cannot
>>
>>
>
Received on 2011-03-07 22:35:16 CET

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