You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2011/02/10 23:56:11 UTC
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_db.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);
@@ -1480,7 +1486,7 @@ svn_wc__db_base_add_directory(svn_wc__db
scratch_pool));
/* ### worry about flushing child subdirs? */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -1555,7 +1561,7 @@ svn_wc__db_base_add_file(svn_wc__db_t *d
insert_base_node, &ibb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -1628,7 +1634,7 @@ svn_wc__db_base_add_symlink(svn_wc__db_t
insert_base_node, &ibb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -1696,7 +1702,7 @@ add_absent_excluded_not_present_node(svn
insert_base_node, &ibb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -1800,7 +1806,7 @@ svn_wc__db_base_remove(svn_wc__db_t *db,
db_base_remove, &brb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -2761,7 +2767,7 @@ svn_wc__db_op_copy_dir(svn_wc__db_t *db,
SVN_ERR(svn_sqlite__with_transaction(pdh->wcroot->sdb,
insert_working_node, &iwb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -2837,7 +2843,7 @@ svn_wc__db_op_copy_file(svn_wc__db_t *db
SVN_ERR(svn_sqlite__with_transaction(pdh->wcroot->sdb,
insert_working_node, &iwb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -2909,7 +2915,7 @@ svn_wc__db_op_copy_symlink(svn_wc__db_t
SVN_ERR(svn_sqlite__with_transaction(pdh->wcroot->sdb,
insert_working_node, &iwb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -2945,7 +2951,7 @@ svn_wc__db_op_add_directory(svn_wc__db_t
SVN_ERR(svn_sqlite__with_transaction(pdh->wcroot->sdb,
insert_working_node, &iwb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -2981,7 +2987,7 @@ svn_wc__db_op_add_file(svn_wc__db_t *db,
SVN_ERR(svn_sqlite__with_transaction(pdh->wcroot->sdb,
insert_working_node, &iwb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -3021,7 +3027,7 @@ svn_wc__db_op_add_symlink(svn_wc__db_t *
SVN_ERR(svn_sqlite__with_transaction(pdh->wcroot->sdb,
insert_working_node, &iwb,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -3320,7 +3326,7 @@ svn_wc__db_op_set_changelist(svn_wc__db_
/* No need to flush the parent entries; changelists were not stored in the
stub */
- SVN_ERR(flush_entries(db, pdh, NULL, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -3382,7 +3388,7 @@ svn_wc__db_op_mark_resolved(svn_wc__db_t
}
/* Some entries have cached the above values. Kapow!! */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -3490,7 +3496,7 @@ svn_wc__db_op_set_tree_conflict(svn_wc__
scratch_pool));
/* There may be some entries, and the lock info is now out of date. */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -3532,7 +3538,7 @@ svn_wc__db_op_revert_actual(svn_wc__db_t
}
/* Some entries have cached the above values. Kapow!! */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -3804,7 +3810,7 @@ svn_wc__db_temp_op_remove_entry(svn_wc__
scratch_pool, scratch_pool));
VERIFY_USABLE_WCROOT(pdh->wcroot);
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
sdb = pdh->wcroot->sdb;
wc_id = pdh->wcroot->wc_id;
@@ -3839,7 +3845,7 @@ svn_wc__db_temp_op_remove_working(svn_wc
scratch_pool, scratch_pool));
VERIFY_USABLE_WCROOT(pdh->wcroot);
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
STMT_DELETE_WORKING_NODE));
@@ -3862,7 +3868,7 @@ update_depth_values(svn_wc__db_t *db,
svn_sqlite__stmt_t *stmt;
/* Flush any entries before we start monkeying the database. */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
excluded
@@ -4433,7 +4439,7 @@ svn_wc__db_temp_op_delete(svn_wc__db_t *
SVN_ERR(svn_sqlite__with_transaction(b.wcroot->sdb, temp_op_delete_txn,
&b, scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -5982,7 +5988,7 @@ svn_wc__db_global_commit(svn_wc__db_t *d
scratch_pool));
/* We *totally* monkeyed the entries. Toss 'em. */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -6073,7 +6079,7 @@ svn_wc__db_global_update(svn_wc__db_t *d
#endif
/* We *totally* monkeyed the entries. Toss 'em. */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -6136,7 +6142,7 @@ svn_wc__db_global_record_fileinfo(svn_wc
scratch_pool));
/* We *totally* monkeyed the entries. Toss 'em. */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -6183,7 +6189,7 @@ svn_wc__db_lock_add(svn_wc__db_t *db,
SVN_ERR(svn_sqlite__insert(NULL, stmt));
/* There may be some entries, and the lock info is now out of date. */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -6218,7 +6224,7 @@ svn_wc__db_lock_remove(svn_wc__db_t *db,
SVN_ERR(svn_sqlite__step_done(stmt));
/* There may be some entries, and the lock info is now out of date. */
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -8169,7 +8175,7 @@ svn_wc__db_temp_op_set_base_incomplete(s
local_dir_abspath, FALSE, scratch_pool);
if (pdh != NULL)
- SVN_ERR(flush_entries(db, pdh, local_dir_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_dir_abspath, scratch_pool));
}
return SVN_NO_ERROR;
@@ -8239,7 +8245,7 @@ svn_wc__db_temp_op_start_directory_updat
start_directory_update_txn, &du,
scratch_pool));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -8390,7 +8396,7 @@ make_copy_txn(void *baton,
SVN_ERR(svn_sqlite__step_done(stmt));
}
- SVN_ERR(flush_entries(mcb->db, mcb->pdh, mcb->local_abspath, iterpool));
+ SVN_ERR(flush_entries(mcb->db, mcb->local_abspath, iterpool));
svn_pool_destroy(iterpool);
@@ -8562,7 +8568,7 @@ svn_wc__db_temp_op_set_file_external(svn
}
SVN_ERR(svn_sqlite__step_done(stmt));
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
@@ -8768,7 +8774,7 @@ svn_wc__db_temp_op_set_rev_and_repos_rel
VERIFY_USABLE_WCROOT(pdh->wcroot);
baton.wcroot = pdh->wcroot;
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
SVN_ERR(svn_sqlite__with_transaction(baton.wcroot->sdb,
set_rev_relpath_txn,
@@ -8863,7 +8869,7 @@ svn_wc__db_temp_op_set_new_dir_to_incomp
VERIFY_USABLE_WCROOT(pdh->wcroot);
baton.wcroot = pdh->wcroot;
- SVN_ERR(flush_entries(db, pdh, local_abspath, scratch_pool));
+ SVN_ERR(flush_entries(db, local_abspath, scratch_pool));
SVN_ERR(svn_sqlite__with_transaction(baton.wcroot->sdb,
set_new_dir_to_incomplete_txn,
Re: svn commit: r1069602 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Fri, Feb 11, 2011 at 8:18 AM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: hwright@apache.org [mailto:hwright@apache.org]
>> Sent: donderdag 10 februari 2011 23:56
>> To: commits@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
RE: svn commit: r1069602 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hwright@apache.org [mailto:hwright@apache.org]
> Sent: donderdag 10 februari 2011 23:56
> To: commits@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.
Bert
RE: svn commit: r1069602 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hwright@apache.org [mailto:hwright@apache.org]
> Sent: donderdag 10 februari 2011 23:56
> To: commits@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.
Bert