On Fri, 2003-12-12 at 03:47, kfogel@tigris.org wrote:
> Author: kfogel
> Date: Thu Dec 11 20:47:59 2003
> New Revision: 7979
>
> Modified:
> trunk/subversion/libsvn_ra_local/ra_plugin.c
> trunk/subversion/libsvn_repos/repos.c
> trunk/subversion/mod_dav_svn/version.c
> trunk/subversion/svnserve/serve.c
> Log:
> Fix issue #1601: do deltification in core code instead of in the
> post-commit hook, but (where possible) deltify after responding to the
> client, to avoid user-visible delays.
>
> Thanks to Greg Hudson, Sander Striker, Mike Pilato, Erik Huelsmann,
> Ben Collins-Sussman, and Elvis Presley for their help.
>
> * subversion/svnserve/serve.c
> (server_baton_t): New boolean field 'daemon'.
> (serve): Store new 'daemon' argument in baton.
> (commit): Run deltification before or after sending the response,
> depending on b->daemon.
>
> * subversion/mod_dav_svn/version.c
> (struct cleanup_deltify_baton): New baton type for below.
> (cleanup_deltify): New pool cleanup function.
> (register_deltification_cleanup): New function to register above.
> (dav_svn_checkin, dav_svn_merge): Register deltification cleanup.
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (struct deltify_etc_baton): New baton type.
> (deltify_etc): New function, wraps original commit callback.
> (svn_ra_local__get_commit_editor): Initialize a deltify_etc_baton,
> use deltify_etc/baton in place of original callback/baton.
>
> * subversion/libsvn_repos/repos.c
> (create_hooks): Remove call to 'svnadmin deltify' from the
> post-commit hook template.
>
>
> Modified: trunk/subversion/libsvn_ra_local/ra_plugin.c
> ==============================================================================
> --- trunk/subversion/libsvn_ra_local/ra_plugin.c (original)
> +++ trunk/subversion/libsvn_ra_local/ra_plugin.c Thu Dec 11 20:47:59 2003
> @@ -307,6 +307,48 @@
> }
>
>
> +struct deltify_etc_baton
> +{
> + svn_fs_t *fs; /* the fs to deltify in */
> + apr_pool_t *pool; /* pool for scratch work */
> + svn_commit_callback_t callback; /* the original callback */
> + void *callback_baton; /* the original callback's baton */
> +};
> +
> +/* This implements 'svn_commit_callback_t'. Its invokes the original
> + (wrapped) callback, but also does deltification on the new revision.
> + BATON is 'struct deltify_etc_baton *'. */
> +static svn_error_t *
> +deltify_etc (svn_revnum_t new_revision,
> + const char *date,
> + const char *author,
> + void *baton)
> +{
> + struct deltify_etc_baton *db = baton;
> + svn_error_t *err1, *err2;
> +
> + /* Invoke the original callback first, in case someone's waiting to
> + know the revision number so they can go off and annotate an
> + issue or something. */
> + err1 = (*db->callback) (new_revision, date, author, db->callback_baton);
> +
> + /* But, deltification shouldn't be stopped just because someone's
> + random callback failed, so proceed unconditionally on to
> + deltification. */
> + err2 = svn_fs_deltify_revision (db->fs, new_revision, db->pool);
> +
> + /* It's more interesting if the original callback failed, so let
> + that one dominate. */
> + if (err1)
> + {
> + svn_error_clear (err2);
> + return err1;
> + }
> +
> + return err2;
> +}
> +
> +
> static svn_error_t *
> svn_ra_local__get_commit_editor (void *session_baton,
> const svn_delta_editor_t **editor,
> @@ -317,12 +359,18 @@
> apr_pool_t *pool)
> {
> svn_ra_local__session_baton_t *sess = session_baton;
> -
> + struct deltify_etc_baton *db = apr_palloc (pool, sizeof(*db));
> +
> + db->fs = sess->fs;
> + db->pool = pool;
> + db->callback = callback;
> + db->callback_baton = callback_baton;
> +
> /* Get the repos commit-editor */
> SVN_ERR (svn_repos_get_commit_editor (editor, edit_baton, sess->repos,
> sess->repos_url, sess->fs_path,
> sess->username, log_msg,
> - callback, callback_baton, pool));
> + deltify_etc, db, pool));
>
> return SVN_NO_ERROR;
> }
>
> Modified: trunk/subversion/libsvn_repos/repos.c
> ==============================================================================
> --- trunk/subversion/libsvn_repos/repos.c (original)
> +++ trunk/subversion/libsvn_repos/repos.c Thu Dec 11 20:47:59 2003
> @@ -637,11 +637,6 @@
> "REV=\"$2\""
> APR_EOL_STR
> APR_EOL_STR
> - "# Deltify predecessors of things changed in this revision."
> - APR_EOL_STR
> - "nice -2 svnadmin deltify \"$REPOS\" -r \"$REV\" &"
> - APR_EOL_STR
> - APR_EOL_STR
> "commit-email.pl \"$REPOS\" \"$REV\" commit-watchers@example.org"
> APR_EOL_STR
> "log-commit.py --repository \"$REPOS\" --revision \"$REV\""
>
> Modified: trunk/subversion/mod_dav_svn/version.c
> ==============================================================================
> --- trunk/subversion/mod_dav_svn/version.c (original)
> +++ trunk/subversion/mod_dav_svn/version.c Thu Dec 11 20:47:59 2003
> @@ -19,6 +19,7 @@
>
>
> #include <httpd.h>
> +#include <http_log.h>
> #include <mod_dav.h>
> #include <apr_tables.h>
> #include <apr_uuid.h>
> @@ -587,6 +588,90 @@
> return dav_svn_working_to_regular_resource(resource);
> }
>
> +
> +/* Closure object for cleanup_deltify. */
> +struct cleanup_deltify_baton
> +{
> + /* The repository in which to deltify. We use a path instead of an
> + object, because it's difficult to obtain a repos or fs object
> + with the right lifetime guarantees. */
> + const char *repos_path;
> +
> + /* The revision number against which to deltify. */
> + svn_revnum_t revision;
> +
> + /* The pool to use for all temporary allocation while working. This
> + may or may not be the same as the pool on which the cleanup is
> + registered, but obviously it must have a lifetime at least as
> + long as that pool. */
> + apr_pool_t *pool;
> +};
> +
> +
> +/* APR pool cleanup function to deltify against a just-committed
> + revision. DATA is a 'struct cleanup_deltify_baton *'.
> +
> + If any errors occur, log them in the httpd server error log, but
> + return APR_SUCCESS no matter what, as this is a pool cleanup
> + function and deltification is not a matter of correctness
> + anyway. */
> +static apr_status_t cleanup_deltify(void *data)
> +{
> + struct cleanup_deltify_baton *cdb = data;
> + svn_repos_t *repos;
> + svn_error_t *err;
> +
> + /* Yes, opening a repository registers a cleanup on the pool from
> + which we are already running as a cleanup. But these days that's
> + safe. A big +smooch+ to Sander Striker -- I'm not sure it was
> + he who implemented this, but he told me about it anyway :-). */
And now he finally remembers why it was a bad idea to have cleanups
do creation of objects instead of only tear-down. But, fortunately
there is a workaround. And possibly this warrants a change in the
pools code.
Straight from apr_pools.c (line 1398):
/* Run cleanups */
run_cleanups(&pool->cleanups);
pool->cleanups = NULL;
/* If new child pools showed up, this is a reason to raise a flag */
if (pool->child)
abort();
Now, this is in the debug code, in the production code, new child
pools are simply ignored == memory leaks (and possible resource
leaks).
The gist is: it is ok to do allocations in the pool that's being cleaned
up. It is also ok to register cleanups against that pool. It is
not ok to create subpools of the pool being cleaned up, _unless_
the cleanup destroys the subpools after it is done. IOW, a cleanup
has to clean up after itself. Simple workaround: create a subpool
of cdb->pool, pass that to svn_repos_open and svn_fs_deltify_revision,
destroy the subpool at the end of the cleanup.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 12 12:45:37 2003