On Wed, May 27, 2015 at 12:11 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> On 27 May 2015 at 12:49, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
> wrote:
> > On Wed, May 27, 2015 at 11:28 AM, Ivan Zhakov <i...@visualsvn.com>
> wrote:
> >>
> >> It seems directory cache checked twice in function
> >> svn_fs_fs__rep_contents_dir_entry:
> >> [[[
> >> svn_error_t *
> >> svn_fs_fs__rep_contents_dir_entry(svn_fs_dirent_t **dirent,
> >> svn_fs_t *fs,
> >> node_revision_t *noderev,
> >> const char *name,
> >> apr_pool_t *result_pool,
> >> apr_pool_t *scratch_pool)
> >> {
> >> svn_boolean_t found = FALSE;
> >>
> >> /* find the cache we may use */
> >> pair_cache_key_t pair_key = { 0 };
> >> const void *key;
> >> svn_cache__t *cache = locate_dir_cache(fs, &key, &pair_key, noderev,
> >> scratch_pool);
> >> if (cache)
> >> {
> >> [...]
> >> SVN_ERR(svn_cache__get_partial((void **)dirent,
> >> &found,
> >> cache,
> >> key,
> >> svn_fs_fs__extract_dir_entry,
> >> &baton,
> >> result_pool));
> >> }
> >>
> >> /* fetch data from disk if we did not find it in the cache */
> >> if (! found)
> >> {
> >> [...]
> >>
> >> /* read the dir from the file system. It will probably be put it
> >> into the cache for faster lookup in future calls. */
> >> SVN_ERR(svn_fs_fs__rep_contents_dir(&entries, fs, noderev,
> >> scratch_pool, scratch_pool));
> >>
> >> [...]
> >> }
> >>
> >> return SVN_NO_ERROR;
> >> }
> >> ]]]
> >>
> >> And svn_fs_fs__rep_contents_dir() functions checks the dir cache again.
> >>
> >> Is my analysis correct or I missed something important?
> >
> >
> > Your analysis is correct and the code is slightly less efficient
> > that it could be. Feel free to add e.g. a "bypass_cache_lookup"
> > flag to the svn_fs_fs__rep_contents_dir() signature.
> >
> Thanks for confirming the issue. I'll fix it then.
>
Thanks.
> > However, the actual gains from this should be minimal because
> > the failed lookup is easily dwarfed by the directory parsing time.
> > Do you have a specific workload where the double lookup becomes
> > more noticeable?
> >
> No, I don't have any specific workloads. I've noticed it just by
> reading code around.
>
Ack.
-- Stefan^2.
