You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Josh Pieper <jj...@pobox.com> on 2004/05/23 02:29:28 UTC
[PATCH] Execute groups of log-files, was (Re: Improving the performance of libsvn_wc for checkouts/updates/switches)
Philip Martin wrote:
> Josh Pieper <jj...@pobox.com> writes:
>
> > So, how about we execute all the log files in order, then erase them
> > in reverse order? That should take care of any unforseen crashes,
> > yes?
>
> Probably. I think we could also delete log files as we go provided
> cleanup did apr_dir_read to identify the lowest numbered log file.
Ok Philip, since you're the resident libsvn_wc expert, think you could
look over this patch and make sure everything looks OK before I
commit? It performs as well as anything I've done before and should
behave identically to the current trunk as far as resumed updates.
-Josh
-----------------------
Improve the performance of the libsvn_wc update editor by letting it
run multiple log files at the same time. When running a group of log
files, the resulting entries file is written out only once at the
end. Previously, the entries file was read and written at the
beginning and end of every log file.
As a result of this change, all logs in the working copy directory
have a numerical suffix that indicates which number they are in a
sequence. During cleanup, the log files are executed in order, but
they are deleted in reverse order so that stale log files cannot be
left behind.
* subversion/libsvn_wc/props.c
(svn_wc_merge_prop_diffs): Append a ".1" to the log files created
here since they are not part of the update editor.
* subversion/libsvn_wc/adm_ops.c
(svn_wc_process_committed): Append a ".1" to the log files here too.
* subversion/libsvn_wc/lock.c
(svn_wc__adm_is_cleanup_required): Once again append a ".1" to the
checked for log file.
* subversion/libsvn_wc/log.c
(svn_wc__run_log): Loop through all files named "log.1", "log.2",
etc. until a non-existant file is found. After executing this log
file group, erase them in reverse order.
* subversion/libsvn_wc/update_editor.c
(struct dir_baton): Add a field to keep track of the next available
log file number for this directory.
(cleanup_dir_baton, cleanup_dir_baton_child): New, called when a
dir_baton is destroyed. They run any left over log files so that
updates are easily resumable if a soft-interrupt caused the editor
to end.
(make_dir_baton): Initialize the new log file number field.
(struct file_baton): Add a field to record the directory that this
file is located in.
(make_file_baton): Initialize the dir_baton field.
(do_entry_deletion): Add a parameter that accepts a pointer to an
integer that contains the next log file number to use. Reset this
log number after running the log file group.
(delete_entry): Pass the log file number to do_entry_deletion.
(close_directory): Always run the current log file group.
(install_file): Accept a log file number, use it, and also perform a
merge dry run if necessary so that the correct conflict status can
be reported to the caller. Previously running the log file would
generate this information.
(close_edit): Pass a log number of 1 to do_entry_deletion.
(svn_wc_add_repos_file): Pass a log number of 1 to install file and
run the resulting log file.
Index: subversion/libsvn_wc/props.c
===================================================================
--- subversion/libsvn_wc/props.c (revision 9860)
+++ subversion/libsvn_wc/props.c (working copy)
@@ -299,7 +299,7 @@
if (! dry_run)
{
- SVN_ERR (svn_wc__open_adm_file (&log_fp, parent, SVN_WC__ADM_LOG,
+ SVN_ERR (svn_wc__open_adm_file (&log_fp, parent, SVN_WC__ADM_LOG ".1",
(APR_WRITE | APR_CREATE), /* not excl */
pool));
log_accum = svn_stringbuf_create ("", pool);
@@ -317,7 +317,7 @@
log_accum->len, NULL, pool),
apr_psprintf (pool, _("Error writing log for '%s'"), path));
- SVN_ERR (svn_wc__close_adm_file (log_fp, parent, SVN_WC__ADM_LOG,
+ SVN_ERR (svn_wc__close_adm_file (log_fp, parent, SVN_WC__ADM_LOG ".1",
1, /* sync */ pool));
SVN_ERR (svn_wc__run_log (adm_access, NULL, pool));
}
Index: subversion/libsvn_wc/log.c
===================================================================
--- subversion/libsvn_wc/log.c (revision 9860)
+++ subversion/libsvn_wc/log.c (working copy)
@@ -1274,6 +1274,9 @@
char buf[BUFSIZ];
apr_size_t buf_len;
apr_file_t *f = NULL;
+ const char *logfile_path;
+ int log_number;
+ apr_pool_t *iterpool = svn_pool_create (pool);
/* kff todo: use the tag-making functions here, now. */
const char *log_start
@@ -1293,29 +1296,45 @@
a ghost open tag. */
SVN_ERR (svn_xml_parse (parser, log_start, strlen (log_start), 0));
- /* Parse the log file's contents. */
- SVN_ERR_W (svn_wc__open_adm_file (&f, svn_wc_adm_access_path (adm_access),
- SVN_WC__ADM_LOG, APR_READ, pool),
- _("Couldn't open log"));
-
- do {
- buf_len = sizeof (buf);
+ for (log_number = 1; ; log_number++)
+ {
+ svn_pool_clear (iterpool);
+ logfile_path = apr_psprintf (iterpool, "log.%d", log_number);
+ /* Parse the log file's contents. */
+ err = svn_wc__open_adm_file (&f, svn_wc_adm_access_path (adm_access),
+ logfile_path, APR_READ, iterpool);
+ if (err)
+ {
+ if (APR_STATUS_IS_ENOENT (err->apr_err))
+ {
+ svn_error_clear (err);
+ break;
+ }
+ else
+ {
+ SVN_ERR_W (err, _("Couldn't open log"));
+ }
+ }
+
+ do {
+ buf_len = sizeof (buf);
+
+ err = svn_io_file_read (f, buf, &buf_len, iterpool);
+ if (err && !APR_STATUS_IS_EOF(err->apr_err))
+ return svn_error_createf
+ (err->apr_err, err,
+ _("Error reading administrative log file in '%s'"),
+ svn_wc_adm_access_path (adm_access));
+
+ SVN_ERR (svn_xml_parse (parser, buf, buf_len, 0));
+
+ } while (! err);
+
+ svn_error_clear (err);
+ SVN_ERR (svn_io_file_close (f, iterpool));
+ }
- err = svn_io_file_read (f, buf, &buf_len, pool);
- if (err && !APR_STATUS_IS_EOF(err->apr_err))
- return svn_error_createf
- (err->apr_err, err,
- _("Error reading administrative log file in '%s'"),
- svn_wc_adm_access_path (adm_access));
- SVN_ERR (svn_xml_parse (parser, buf, buf_len, 0));
-
- } while (! err);
-
- svn_error_clear (err);
- SVN_ERR (svn_io_file_close (f, pool));
-
-
/* Pacify Expat with a pointless closing element tag. */
SVN_ERR (svn_xml_parse (parser, log_end, strlen (log_end), 1));
@@ -1336,9 +1355,15 @@
}
else
{
- /* No 'killme'? Remove the logfile; its commands have been executed. */
- SVN_ERR (svn_wc__remove_adm_file (svn_wc_adm_access_path (adm_access),
- pool, SVN_WC__ADM_LOG, NULL));
+ for (log_number--; log_number > 0; log_number--)
+ {
+ svn_pool_clear (iterpool);
+ logfile_path = apr_psprintf (iterpool, "log.%d", log_number);
+
+ /* No 'killme'? Remove the logfile; its commands have been executed. */
+ SVN_ERR (svn_wc__remove_adm_file (svn_wc_adm_access_path (adm_access),
+ iterpool, logfile_path, NULL));
+ }
}
return SVN_NO_ERROR;
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 9860)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -242,7 +242,7 @@
/* Open a log file in the administrative directory */
SVN_ERR (svn_wc__open_adm_file (&log_fp,
svn_wc_adm_access_path (adm_access),
- SVN_WC__ADM_LOG,
+ SVN_WC__ADM_LOG ".1",
(APR_WRITE | APR_CREATE),
pool));
@@ -378,7 +378,7 @@
apr_psprintf (pool, _("Error writing log file for '%s'"), path));
SVN_ERR (svn_wc__close_adm_file (log_fp, svn_wc_adm_access_path (adm_access),
- SVN_WC__ADM_LOG,
+ SVN_WC__ADM_LOG ".1",
TRUE, /* sync */
pool));
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 9860)
+++ subversion/libsvn_wc/lock.c (working copy)
@@ -893,7 +893,8 @@
{
svn_node_kind_t kind;
const char *log_path = svn_wc__adm_path (svn_wc_adm_access_path (adm_access),
- FALSE, pool, SVN_WC__ADM_LOG, NULL);
+ FALSE, pool, SVN_WC__ADM_LOG ".1",
+ NULL);
/* The presence of a log file demands cleanup */
SVN_ERR (svn_io_check_path (log_path, &kind, pool));
Index: subversion/libsvn_wc/update_editor.c
===================================================================
--- subversion/libsvn_wc/update_editor.c (revision 9860)
+++ subversion/libsvn_wc/update_editor.c (working copy)
@@ -141,6 +141,9 @@
/* The bump information for this directory. */
struct bump_dir_info *bump_info;
+ /* The current log file number. */
+ int log_number;
+
/* The pool in which this baton itself is allocated. */
apr_pool_t *pool;
};
@@ -211,7 +214,53 @@
return entry->url;
}
+/* An APR pool cleanup handler. This runs the log file for a
+ directory baton. */
+static apr_status_t
+cleanup_dir_baton (void *dir_baton)
+{
+ struct dir_baton *db = dir_baton;
+ svn_error_t *err;
+ apr_status_t apr_err;
+ svn_wc_adm_access_t *adm_access;
+ /* If there are no log files to write, return immediately. */
+ if (db->log_number == 1)
+ return APR_SUCCESS;
+
+ err = svn_wc_adm_retrieve (&adm_access, db->edit_baton->adm_access,
+ db->path, apr_pool_parent_get (db->pool));
+
+ if (err)
+ {
+ apr_err = err->apr_err;
+ svn_error_clear (err);
+ return apr_err;
+ }
+
+ err = svn_wc__run_log (adm_access, NULL, apr_pool_parent_get (db->pool));
+
+ if (err)
+ {
+ apr_err = err->apr_err;
+ svn_error_clear (err);
+ return apr_err;
+ }
+
+ return APR_SUCCESS;
+}
+
+/* An APR pool cleanup handler. This is a child handler, it removes
+ the mail pool handler. */
+static apr_status_t
+cleanup_dir_baton_child (void *dir_baton)
+{
+ struct dir_baton *db = dir_baton;
+ apr_pool_cleanup_kill (db->pool, db, cleanup_dir_baton);
+ return APR_SUCCESS;
+}
+
+
/* Return a new dir_baton to represent NAME (a subdirectory of
PARENT_BATON). If PATH is NULL, this is the root directory of the
edit. */
@@ -291,11 +340,15 @@
d->edit_baton = eb;
d->parent_baton = pb;
- d->pool = pool;
+ d->pool = svn_pool_create (pool);
d->propchanges = apr_array_make (pool, 1, sizeof (svn_prop_t));
d->added = added;
d->bump_info = bdi;
+ d->log_number = 1;
+ apr_pool_cleanup_register (d->pool, d, cleanup_dir_baton,
+ cleanup_dir_baton_child);
+
return d;
}
@@ -443,6 +496,9 @@
/* The global edit baton. */
struct edit_baton *edit_baton;
+ /* The parent directory of this file. */
+ struct dir_baton *dir_baton;
+
/* Pool specific to this file_baton. */
apr_pool_t *pool;
@@ -520,6 +576,7 @@
f->propchanges = apr_array_make (pool, 1, sizeof (svn_prop_t));
f->bump_info = pb->bump_info;
f->added = adding;
+ f->dir_baton = pb;
/* No need to initialize f->digest, since we used pcalloc(). */
@@ -789,7 +846,8 @@
static svn_error_t *
do_entry_deletion (struct edit_baton *eb,
const char *parent_path,
- const char *path,
+ const char *path,
+ int *log_number,
apr_pool_t *pool)
{
apr_file_t *log_fp = NULL;
@@ -811,7 +869,8 @@
SVN_ERR (svn_wc__open_adm_file (&log_fp,
parent_path,
- SVN_WC__ADM_LOG,
+ apr_psprintf (pool, SVN_WC__ADM_LOG
+ ".%d", *log_number),
(APR_WRITE | APR_CREATE), /* not excl */
pool));
@@ -862,7 +921,8 @@
SVN_ERR (svn_wc__close_adm_file (log_fp,
parent_path,
- SVN_WC__ADM_LOG,
+ apr_psprintf (pool, SVN_WC__ADM_LOG
+ ".%d", (*log_number)++),
TRUE, /* sync */
pool));
@@ -906,6 +966,7 @@
SVN_ERR (leftmod_error_chain (svn_wc__run_log (adm_access, NULL, pool),
logfile_path, parent_path, pool));
+ *log_number = 1;
/* The passed-in `path' is relative to the anchor of the edit, so if
* the operation was invoked on something other than ".", then
@@ -934,7 +995,8 @@
apr_pool_t *pool)
{
struct dir_baton *pb = parent_baton;
- return do_entry_deletion (pb->edit_baton, pb->path, path, pool);
+ return do_entry_deletion (pb->edit_baton, pb->path, path, &pb->log_number,
+ pool);
}
@@ -1125,27 +1187,28 @@
struct dir_baton *db = dir_baton;
svn_wc_notify_state_t prop_state = svn_wc_notify_state_unknown;
apr_array_header_t *entry_props, *wc_props, *regular_props;
-
+ svn_wc_adm_access_t *adm_access;
+
SVN_ERR (svn_categorize_props (db->propchanges, &entry_props, &wc_props,
®ular_props, pool));
+ SVN_ERR (svn_wc_adm_retrieve (&adm_access, db->edit_baton->adm_access,
+ db->path, db->pool));
+
/* If this directory has property changes stored up, now is the time
to deal with them. */
if (regular_props->nelts || entry_props->nelts || wc_props->nelts)
{
- svn_wc_adm_access_t *adm_access;
apr_file_t *log_fp = NULL;
/* to hold log messages: */
svn_stringbuf_t *entry_accum = svn_stringbuf_create ("", db->pool);
- SVN_ERR (svn_wc_adm_retrieve (&adm_access, db->edit_baton->adm_access,
- db->path, db->pool));
-
/* Open log file */
SVN_ERR (svn_wc__open_adm_file (&log_fp,
db->path,
- SVN_WC__ADM_LOG,
+ apr_psprintf (db->pool, SVN_WC__ADM_LOG
+ ".%d", db->log_number),
(APR_WRITE | APR_CREATE), /* not excl */
db->pool));
@@ -1244,14 +1307,17 @@
/* The log is ready to run, close it. */
SVN_ERR (svn_wc__close_adm_file (log_fp,
db->path,
- SVN_WC__ADM_LOG,
+ apr_psprintf (db->pool, SVN_WC__ADM_LOG
+ ".%d", db->log_number),
TRUE, /* sync */
db->pool));
- /* Run the log. */
- SVN_ERR (svn_wc__run_log (adm_access, NULL, db->pool));
}
+ /* Run the log. */
+ SVN_ERR (svn_wc__run_log (adm_access, NULL, db->pool));
+ db->log_number = 1;
+
/* We're done with this directory, so remove one reference from the
bump information. This may trigger a number of actions. See
maybe_bump_dir_info() for more information. */
@@ -1710,6 +1776,7 @@
install_file (svn_wc_notify_state_t *content_state,
svn_wc_notify_state_t *prop_state,
svn_wc_adm_access_t *adm_access,
+ int *log_number,
const char *file_path,
svn_revnum_t new_revision,
const char *new_text_path,
@@ -1731,6 +1798,7 @@
svn_boolean_t magic_props_changed = FALSE;
apr_array_header_t *regular_props = NULL, *wc_props = NULL,
*entry_props = NULL;
+ enum svn_wc_merge_outcome_t merge_outcome;
/* The code flow does not depend upon these being set to NULL, but
it removes a gcc 3.1 `might be used uninitialized in this
@@ -1765,7 +1833,8 @@
right now. */
SVN_ERR (svn_wc__open_adm_file (&log_fp,
parent_dir,
- SVN_WC__ADM_LOG,
+ apr_psprintf (pool, SVN_WC__ADM_LOG ".%d",
+ *log_number),
(APR_WRITE | APR_CREATE), /* not excl */
pool));
@@ -2050,6 +2119,7 @@
textual changes into the working file. */
const char *oldrev_str, *newrev_str;
const svn_wc_entry_t *e;
+ const char *base;
/* Create strings representing the revisions of the
old and new text-bases. */
@@ -2083,6 +2153,19 @@
/* If a conflict happens, then the entry will be
marked "Conflicted" and will track either 2 or 3 new
temporary fulltext files that resulted. */
+
+ /* Run a dry-run of the merge to see if a conflict will
+ occur. This is needed so we can report back to the
+ client as the changes come in. */
+ base = svn_wc_adm_access_path (adm_access);
+
+ SVN_ERR (svn_wc_merge (svn_path_join (base, txtb, pool),
+ svn_path_join (base, tmp_txtb, pool),
+ svn_path_join (base, base_name, pool),
+ adm_access,
+ oldrev_str, newrev_str, ".mine",
+ TRUE, &merge_outcome, diff3_cmd,
+ pool));
} /* end: working file exists and has mods */
} /* end: working file has mods */
@@ -2190,27 +2273,17 @@
log_accum->len, NULL, pool),
apr_psprintf (pool, _("Error writing log for '%s'"), file_path));
- /* The log is ready to run. Close it and run it! */
- SVN_ERR (svn_wc__close_adm_file (log_fp, parent_dir, SVN_WC__ADM_LOG,
+ /* The log is done, close it. */
+ SVN_ERR (svn_wc__close_adm_file (log_fp, parent_dir,
+ apr_psprintf (pool, SVN_WC__ADM_LOG ".%d",
+ (*log_number)++),
TRUE, /* sync */ pool));
- SVN_ERR (svn_wc__run_log (adm_access, diff3_cmd, pool));
if (content_state)
{
- const svn_wc_entry_t *entry;
- svn_boolean_t tc, pc;
-
/* Initialize the state of our returned value. */
*content_state = svn_wc_notify_state_unknown;
- /* ### There should be a more efficient way of finding out whether
- or not the file is modified|merged|conflicted. If the
- svn_wc__run_log() call above could return a special error code
- in case of a conflict or something, that would work. */
-
- SVN_ERR (svn_wc_entry (&entry, file_path, adm_access, TRUE, pool));
- SVN_ERR (svn_wc_conflicted_p (&tc, &pc, parent_dir, entry, pool));
-
/* This is kind of interesting. Even if no new text was
installed (i.e., new_text_path was null), we could still
report a pre-existing conflict state. Say a file, already
@@ -2218,7 +2291,7 @@
update. Then we'll notify that it has text conflicts. This
seems okay to me. I guess. I dunno. You? */
- if (tc)
+ if (merge_outcome == svn_wc_merge_conflict)
*content_state = svn_wc_notify_state_conflicted;
else if (new_text_path)
{
@@ -2277,6 +2350,7 @@
SVN_ERR (install_file (&content_state,
&prop_state,
adm_access,
+ &fb->dir_baton->log_number,
fb->path,
(*eb->target_revision),
new_text_path,
@@ -2317,13 +2391,15 @@
{
struct edit_baton *eb = edit_baton;
const char *target_path = svn_path_join (eb->anchor, eb->target, pool);
+ int log_number = 1;
/* If there is a target and that target is missing, then it
apparently wasn't re-added by the update process, so we'll
pretend that the editor deleted the entry. The helper function
do_entry_deletion() will take care of the necessary steps. */
if ((*eb->target) && (svn_wc__adm_missing (eb->adm_access, target_path)))
- SVN_ERR (do_entry_deletion (eb, eb->anchor, eb->target, pool));
+ SVN_ERR (do_entry_deletion (eb, eb->anchor, eb->target, &log_number,
+ pool));
/* The editor didn't even open the root; we have to take care of
some cleanup stuffs. */
@@ -2737,6 +2813,7 @@
{
const char *new_URL;
apr_array_header_t *propchanges;
+ int log_number = 1;
/* Fabricate the anticipated new URL of the target. */
{
@@ -2757,6 +2834,7 @@
SVN_ERR (install_file (NULL,
NULL,
adm_access,
+ &log_number,
dst_path,
0,
new_text_path,
@@ -2770,5 +2848,7 @@
NULL,
pool));
+ SVN_ERR (svn_wc__run_log (adm_access, NULL, pool));
+
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Execute groups of log-files, was (Re: Improving the performance of libsvn_wc for checkouts/updates/switches)
Posted by Josh Pieper <jj...@pobox.com>.
Josh Pieper wrote:
> Philip Martin wrote:
> > The problem is that ra_dav is the only layer that uses wcprops, if you
> > only test over ra_local you won't see wcprops related failures.
> >
> > The original problem generated some dev list discussion between me and
> > Karl, and some code from Karl. As I recall the problem was that an
> > interrupted update caused the wcprops and the text-base/revision to be
> > out-of-sync (I can't remember which one was wrong). I don't know
> > whether the wcprops code is still fragile, or whether Karl's change
> > fixed it.
>
> Ok, I did some testing over ra_dav and found no problems. It passes
> the whole test-suite, plus I did quite a bit of checkouts/updating and
> interrupting without any issues. I also looked at issue #1136, which
> I think is what you were referring to here. I was unable to reproduce
> that failure with the new code either.
>
> One last concern. Do we need to be worried about compatability? This
> change would prevent trunk from running the log files from older
> working copies since their names have changed. Should I hack in
> something to try "log" before trying "log.1" or can we just accept
> that amount of change?
It turns out that keeping compatability wasn't too hard, it in fact
reduced the number of files I had to touch. Therefore, I went ahead
and committed this optimization in r9868.
There is one other optimization I want to make to the working copy
code, which should get it finally to O(n) for very large directory
sizes. I'll post that in a new thread later on, this thread is
starting to get pretty deep.
-Josh
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Execute groups of log-files, was (Re: Improving the performance of libsvn_wc for checkouts/updates/switches)
Posted by kf...@collab.net.
Josh Pieper <jj...@pobox.com> writes:
> Philip Martin wrote:
> > The original problem generated some dev list discussion between me and
> > Karl, and some code from Karl. As I recall the problem was that an
> > interrupted update caused the wcprops and the text-base/revision to be
> > out-of-sync (I can't remember which one was wrong). I don't know
> > whether the wcprops code is still fragile, or whether Karl's change
> > fixed it.
>
> Ok, I did some testing over ra_dav and found no problems. It passes
> the whole test-suite, plus I did quite a bit of checkouts/updating and
> interrupting without any issues. I also looked at issue #1136, which
> I think is what you were referring to here. I was unable to reproduce
> that failure with the new code either.
You wouldn't generally see the problem in normal testing, you'd only
see it in interrupted operations over ra_dav and other extraordinary
circumstances.
Let me dredge up what I can from the musty caverns of memory...
Well, hmm, this might be resolved issue #1136, in which case what I
said above isn't true -- you *can* get a bug in normal operations.
The patch in that issue helps explain why wcprops are "special"
sometimes.
> One last concern. Do we need to be worried about compatability? This
> change would prevent trunk from running the log files from older
> working copies since their names have changed. Should I hack in
> something to try "log" before trying "log.1" or can we just accept
> that amount of change?
The only operation this could affect would be 'svn cleanup', since log
files shouldn't be present when any other operation starts. So the
scenario here is that someone
1. does some operation that leaves the working copy in need of cleanup
2. upgrades their subversion :-)
3. runs the next operation, is told they must run 'svn cleanup' first
4. tries 'svn cleanup'
Step 2 seems unlikely, though I suppose it could happen to someone.
They can always downgrade temporarily to solve it, though, so there's
a workaround. So I don't think we really need to consider log
behavior to be part of the "working copy format"; no need to worry
about compatibility here, IMHO.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Execute groups of log-files, was (Re: Improving the performance of libsvn_wc for checkouts/updates/switches)
Posted by Josh Pieper <jj...@pobox.com>.
Philip Martin wrote:
> The problem is that ra_dav is the only layer that uses wcprops, if you
> only test over ra_local you won't see wcprops related failures.
>
> The original problem generated some dev list discussion between me and
> Karl, and some code from Karl. As I recall the problem was that an
> interrupted update caused the wcprops and the text-base/revision to be
> out-of-sync (I can't remember which one was wrong). I don't know
> whether the wcprops code is still fragile, or whether Karl's change
> fixed it.
Ok, I did some testing over ra_dav and found no problems. It passes
the whole test-suite, plus I did quite a bit of checkouts/updating and
interrupting without any issues. I also looked at issue #1136, which
I think is what you were referring to here. I was unable to reproduce
that failure with the new code either.
One last concern. Do we need to be worried about compatability? This
change would prevent trunk from running the log files from older
working copies since their names have changed. Should I hack in
something to try "log" before trying "log.1" or can we just accept
that amount of change?
-Josh
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Execute groups of log-files, was (Re: Improving the
performance of libsvn_wc for checkouts/updates/switches)
Posted by Philip Martin <ph...@codematters.co.uk>.
Josh Pieper <jj...@pobox.com> writes:
> Philip Martin wrote:
>> > @@ -2083,6 +2153,19 @@
>> > /* If a conflict happens, then the entry will be
>> > marked "Conflicted" and will track either 2 or 3 new
>> > temporary fulltext files that resulted. */
>>
>> Is the above comment now redundant?
>
> Well, my new comment doesn't really state it, but I suppose it can go.
Your comment doesn't need to state it, it's information that applies
to the old code.
>> > +
>> > + /* Run a dry-run of the merge to see if a conflict will
>> > + occur. This is needed so we can report back to the
>> > + client as the changes come in. */
>> > + base = svn_wc_adm_access_path (adm_access);
>> > +
>> > + SVN_ERR (svn_wc_merge (svn_path_join (base, txtb, pool),
>> > + svn_path_join (base, tmp_txtb, pool),
>> > + svn_path_join (base, base_name, pool),
>> > + adm_access,
>> > + oldrev_str, newrev_str, ".mine",
>> > + TRUE, &merge_outcome, diff3_cmd,
>> > + pool));
>>
>> This is a bit unfortunate. I think I see what you are doing, the old
>> code could examine the wc state to get this information but in the new
>> code the notification callback is called before the log file is run.
>> A dry-run merge can be time consuming when dealing with large files,
>> although since only files with local mods are affected I guess it may
>> not be too bad in practice. I don't know if there are any
>> alternatives, I guess it's difficult to postpone the notification
>> callback.
>
> Yeah, I thought about it some too and decided that a constant factor
> extra merge per file with local mods was probably worth it to drop the
> O(n^2) behavior. I imagine more people will run into the checkout of
> a large directory than would run into having many large files with
> local mods. And at worst the latter half would be only twice as slow.
> The former could easily be many orders of magnitude slower.
I tend to agreee, but I still think it's unfortunate that we have to
do a potentially expensive operation twice.
>> Have you tested over ra_dav? I recall we had problems in the past
>> with wcprops during an interrupted update, although I can't remember
>> the exact details. It's possible we made the wcprops code robust
>> enough for this change to work.
>
> No I haven't. Is the editor somehow driven differently via ra_dav
> that would cause different problems? Was there an issue, and can you
> remember it?
The problem is that ra_dav is the only layer that uses wcprops, if you
only test over ra_local you won't see wcprops related failures.
The original problem generated some dev list discussion between me and
Karl, and some code from Karl. As I recall the problem was that an
interrupted update caused the wcprops and the text-base/revision to be
out-of-sync (I can't remember which one was wrong). I don't know
whether the wcprops code is still fragile, or whether Karl's change
fixed it.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Execute groups of log-files, was (Re: Improving the performance of libsvn_wc for checkouts/updates/switches)
Posted by Josh Pieper <jj...@pobox.com>.
Philip Martin wrote:
> > @@ -2083,6 +2153,19 @@
> > /* If a conflict happens, then the entry will be
> > marked "Conflicted" and will track either 2 or 3 new
> > temporary fulltext files that resulted. */
>
> Is the above comment now redundant?
Well, my new comment doesn't really state it, but I suppose it can go.
> > +
> > + /* Run a dry-run of the merge to see if a conflict will
> > + occur. This is needed so we can report back to the
> > + client as the changes come in. */
> > + base = svn_wc_adm_access_path (adm_access);
> > +
> > + SVN_ERR (svn_wc_merge (svn_path_join (base, txtb, pool),
> > + svn_path_join (base, tmp_txtb, pool),
> > + svn_path_join (base, base_name, pool),
> > + adm_access,
> > + oldrev_str, newrev_str, ".mine",
> > + TRUE, &merge_outcome, diff3_cmd,
> > + pool));
>
> This is a bit unfortunate. I think I see what you are doing, the old
> code could examine the wc state to get this information but in the new
> code the notification callback is called before the log file is run.
> A dry-run merge can be time consuming when dealing with large files,
> although since only files with local mods are affected I guess it may
> not be too bad in practice. I don't know if there are any
> alternatives, I guess it's difficult to postpone the notification
> callback.
Yeah, I thought about it some too and decided that a constant factor
extra merge per file with local mods was probably worth it to drop the
O(n^2) behavior. I imagine more people will run into the checkout of
a large directory than would run into having many large files with
local mods. And at worst the latter half would be only twice as slow.
The former could easily be many orders of magnitude slower.
> Have you tested over ra_dav? I recall we had problems in the past
> with wcprops during an interrupted update, although I can't remember
> the exact details. It's possible we made the wcprops code robust
> enough for this change to work.
No I haven't. Is the editor somehow driven differently via ra_dav
that would cause different problems? Was there an issue, and can you
remember it?
-Josh
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Execute groups of log-files, was (Re: Improving the
performance of libsvn_wc for checkouts/updates/switches)
Posted by Philip Martin <ph...@codematters.co.uk>.
Josh Pieper <jj...@pobox.com> writes:
> Philip Martin wrote:
>> Probably. I think we could also delete log files as we go provided
>> cleanup did apr_dir_read to identify the lowest numbered log file.
Well done! You ignored my thinko about deleting log files as we go;
the log files can only be deleted once the entries file has been
written.
> @@ -2083,6 +2153,19 @@
> /* If a conflict happens, then the entry will be
> marked "Conflicted" and will track either 2 or 3 new
> temporary fulltext files that resulted. */
Is the above comment now redundant?
> +
> + /* Run a dry-run of the merge to see if a conflict will
> + occur. This is needed so we can report back to the
> + client as the changes come in. */
> + base = svn_wc_adm_access_path (adm_access);
> +
> + SVN_ERR (svn_wc_merge (svn_path_join (base, txtb, pool),
> + svn_path_join (base, tmp_txtb, pool),
> + svn_path_join (base, base_name, pool),
> + adm_access,
> + oldrev_str, newrev_str, ".mine",
> + TRUE, &merge_outcome, diff3_cmd,
> + pool));
This is a bit unfortunate. I think I see what you are doing, the old
code could examine the wc state to get this information but in the new
code the notification callback is called before the log file is run.
A dry-run merge can be time consuming when dealing with large files,
although since only files with local mods are affected I guess it may
not be too bad in practice. I don't know if there are any
alternatives, I guess it's difficult to postpone the notification
callback.
Have you tested over ra_dav? I recall we had problems in the past
with wcprops during an interrupted update, although I can't remember
the exact details. It's possible we made the wcprops code robust
enough for this change to work.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Execute groups of log-files, was (Re: Improving the performance
of libsvn_wc for checkouts/updates/switches)
Posted by Branko Čibej <br...@xbc.nu>.
Josh Pieper wrote:
>@@ -1293,29 +1296,45 @@
> a ghost open tag. */
> SVN_ERR (svn_xml_parse (parser, log_start, strlen (log_start), 0));
>
>- /* Parse the log file's contents. */
>- SVN_ERR_W (svn_wc__open_adm_file (&f, svn_wc_adm_access_path (adm_access),
>- SVN_WC__ADM_LOG, APR_READ, pool),
>- _("Couldn't open log"));
>-
>- do {
>- buf_len = sizeof (buf);
>+ for (log_number = 1; ; log_number++)
>+ {
>+ svn_pool_clear (iterpool);
>+ logfile_path = apr_psprintf (iterpool, "log.%d", log_number);
>
>
Eek. I think this should be SVN_WC__ADM_LOG ".%d", not "log.%d", to be
consistent with usage elsewhere.
>@@ -1336,9 +1355,15 @@
> }
> else
> {
>- /* No 'killme'? Remove the logfile; its commands have been executed. */
>- SVN_ERR (svn_wc__remove_adm_file (svn_wc_adm_access_path (adm_access),
>- pool, SVN_WC__ADM_LOG, NULL));
>+ for (log_number--; log_number > 0; log_number--)
>+ {
>+ svn_pool_clear (iterpool);
>+ logfile_path = apr_psprintf (iterpool, "log.%d", log_number);
>
>
Same here.
>@@ -811,7 +869,8 @@
>
> SVN_ERR (svn_wc__open_adm_file (&log_fp,
> parent_path,
>- SVN_WC__ADM_LOG,
>+ apr_psprintf (pool, SVN_WC__ADM_LOG
>+ ".%d", *log_number),
>
>
Yes, exactly like this. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org