On Fri, Feb 11, 2011 at 8:18 AM, Bert Huijben <b...@qqmail.nl> wrote: > > >> -----Original Message----- >> From: hwri...@apache.org [mailto:hwri...@apache.org] >> Sent: donderdag 10 februari 2011 23:56 >> To: comm...@subversion.apache.org >> Subject: svn commit: r1069602 - >> /subversion/trunk/subversion/libsvn_wc/wc_db.c >> >> Author: hwright >> Date: Thu Feb 10 22:56:11 2011 >> New Revision: 1069602 >> >> URL: http://svn.apache.org/viewvc?rev=1069602&view=rev >> Log: >> Make the flush_entries() function fetch its own PDH, allowing callers >> to avoid >> fetching it. >> >> This has the side effect of causing flush_entries() to flush both the >> current, >> as well as the parent's caches in all cases. But since that will only >> happen >> in the backward compat scenario, it will have neglible real performance >> impact. >> >> * subversion/libsvn_wc/wc_db.c >> (flush_entries): Don't take a PDH param, fetch one internally. >> [elsewhere]: Adjust callers to not provide a PDH. >> >> Modified: >> subversion/trunk/subversion/libsvn_wc/wc_db.c >> >> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c >> URL: >> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_d >> b.c?rev=1069602&r1=1069601&r2=1069602&view=diff >> ======================================================================= >> ======= >> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original) >> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Thu Feb 10 22:56:11 >> 2011 >> @@ -1057,10 +1057,16 @@ gather_repo_children(const apr_array_hea >> /* */ >> static svn_error_t * >> flush_entries(svn_wc__db_t *db, >> - svn_wc__db_pdh_t *pdh, >> const char *local_abspath, >> apr_pool_t *scratch_pool) >> { >> + svn_wc__db_pdh_t *pdh; >> + const char *local_relpath; >> + >> + SVN_ERR(svn_wc__db_pdh_parse_local_abspath(&pdh, &local_relpath, db, >> + local_abspath, >> svn_sqlite__mode_readwrite, >> + scratch_pool, scratch_pool)); >> + >> if (pdh->adm_access) >> svn_wc__adm_access_set_entries(pdh->adm_access, NULL); > > This code used to be just a few pointer compares in the normal case of no > cached adm_access instances, but it is now splitting an abspath to a local > relpath (read: allocating ram a few times) via several hashtable lookups. In > some cases it might even perform disk-io. (By statting if a dirent is a file > or a directory) > > Calling svn_wc__db_pdh_parse_local_abspath is not that cheap. > > I think we should consider reverting this change and maybe add a helper which > takes a local_relpath instead for the cases where we already have that > available.
The endgame here is to not use PDHs at all the adm_access baton caching. Instead, I plan to have a simple hash with local_abspaths indexing the batons. As long as we don't mind being overly aggressive in clearing the cache, we won't need to parse the PDH (thus incurring the extra stat()) to flush the cache. Another, rather simple, strategy is to have a 'backward_compat_mode' flag in the wcroot which is only set when entering libsvn_wc via one of the legacy APIs. We could then easily check the flag for an early out in flush_entries(). Either way, we won't be parsing the pdh for long in flush_entries(). -Hyrum
