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.
> + 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.
> + 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
>
>
