On 09.02.2012 16:05, Daniel Shahaf wrote:
> stefan2_at_apache.org wrote on Wed, Feb 08, 2012 at 00:44:26 -0000:
>> Author: stefan2
>> Date: Wed Feb 8 00:44:26 2012
>> New Revision: 1241718
>>
>> URL: http://svn.apache.org/viewvc?rev=1241718&view=rev
>> Log:
>> Major improvement in delta window handling: Cache intermediate
>> combined delta windows such that changes "close by" won't need
>> to discover and read the whole chain again.
>>
>> For algorithms that traverse history linearly, this optimization
>> gives delta combination an amortized constant runtime.
>>
>> For now, we only cache representations< 100kB. Support for larger
>> reps can be added later.
>>
>> * subversion/libsvn_fs_fs/fs.h
>> (fs_fs_data_t): add cache for combined windows
>> * subversion/libsvn_fs_fs/caching.c
>> (svn_fs_fs__initialize_caches): initialize that cache
>>
>> * subversion/libsvn_fs_fs/fs_fs.c
>> (rep_state): add reference to new cache
>> (create_rep_state_body): init that reference
>> (rep_read_baton): add reference to cached base window
>> (get_cached_combined_window, set_cached_combined_window):
>> new utility functions
>> (build_rep_list): terminate delta chain early if cached
>> base window is available
>> (rep_read_get_baton): adapt caller
>> (get_combined_window): re-implement
>> (get_contents): handle new special case; adapt to
>> get_combined_window() signature changes
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_fs_fs/caching.c
>> subversion/trunk/subversion/libsvn_fs_fs/fs.h
>> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>>
> I haven't reviewed this, but a question:
>
>> +/* Read the WINDOW_P for the rep state RS from the current FSFS session's
>> + * cache. This will be a no-op and IS_CACHED will be set to FALSE if no
>> + * cache has been given. If a cache is available IS_CACHED will inform
>> + * the caller about the success of the lookup. Allocations (of the window
>> + * in particualar) will be made from POOL.
>> + */
>> +static svn_error_t *
>> +get_cached_combined_window(svn_stringbuf_t **window_p,
>> + struct rep_state *rs,
>> + svn_boolean_t *is_cached,
>> + apr_pool_t *pool)
>> +{
>> + if (! rs->combined_cache)
>> + {
>> + /* txdelta window has not been enabled */
>> + *is_cached = FALSE;
>> + }
>> + else
>> + {
>> + /* ask the cache for the desired txdelta window */
>> + return svn_cache__get((void **)window_p,
>> + is_cached,
>> + rs->combined_cache,
>> + get_window_key(rs, rs->start, pool),
> How does the cache key identify the particular combined window being
> cached?
Undeltified windows use the same key as their deltified
representation; basically revision file + offset. The distinction
between deltified and un-deltified is made by the cache
instance prefix.
> get_window_key() may return "". If it returns "" when called as an
> argument to svn_cache__set() and then also here, won't the cache return
> a wrong result?
>
There is a comment in get_window_key() for this case.
"" will only be returned if we can't get the name of the
open APR file. This is virtually impossible. If it happens
anyways, it will hit the deltified window cache first, we will
report a repository corruption.
But maybe we should change the cache API definition
to support and reject NULL keys. get_window_key() could
then return simply NULL and could do with fewer assumptions.
-- Stefan^2.
Received on 2012-02-12 03:07:06 CET