On Mon, Mar 7, 2011 at 3:03 PM, Daniel Shahaf <d...@daniel.shahaf.name> wrote:
> hwri...@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
>>
>>
>
