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

Re: [PATCH]: Avoid extra stat calls during report_revisions on some filesystems

From: Daniel Berlin <dberlin_at_dberlin.org>
Date: 2005-11-17 14:47:39 CET

On Thu, 2005-11-17 at 06:33 +0100, Branko Čibej wrote:
> Daniel Berlin wrote:
> > This patch is an implementation of the idea i posted in the thread
> > titled "The cost of svn_io_get_dirents2 and early error messages".
> >
> > It uses a function that only gets filenames in the directory, instead of
> > extra information that we don't really need, and pushes the error
> > handling down into the adm retrieval function.
> >
> Not just the error handling, also the stat'ing.

Yes, but the stat'ing only happens in the error case now, instead of all
the time, otherwise this would not be a win at all.

>
> Which, I suspect, will hurt performance on filesystems where you get
> stat info for free with the list of file names. That includes all
> Windows filesystems.

I don't believe this is true, for the reason above.

Though you are the second person to believe this.

It probably looks like this because the patch context sucks.
However, take a gander at the retrieval function that was modified:

svn_error_t *
svn_wc_adm_retrieve (svn_wc_adm_access_t **adm_access,
                     svn_wc_adm_access_t *associated,
                     const char *path,
                     apr_pool_t *pool)
{
  SVN_ERR (svn_wc__adm_retrieve_internal (adm_access, associated, path,
pool));

  /* Most of the code expects access batons to exist, so returning an
error
     generally makes the calling code simpler as it doesn't need to
check
     for NULL batons. */
  if (! *adm_access)
    {
      <code that will stat and return an error>
    }
 return SVN_NO_ERROR
}

Note that

1. The retrieval_internal function only does a hash lookup, nothing
more.

2. It will *always* return an error when !*adm_access.

So
3. The regular path does *not stat at all*.

>
> Wouldn't it be better if svn_io_get_dir_filenames returned the same kind
> of hash as svn_io_get_dirents2, but only filled in the other info if
> it's available? IIRC APR will tell us which fields in the apr_finfo_t
> are valid, and will fill more than just the name, even if that's all you
> request. Later on, this information could be used to avoid further stats.

I'm not sure how it could be used to avoid further stats.
Again, the stat'ing only happens in the error case.
If we issue an error, it's going to return from the report_revisions
function, and thus, there is nothing left to pass our stat information
to.

>
> -- Brane
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Nov 17 14:48:37 2005

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.