Re: [PATCH][libsvn_wc] Issue 2607: Post-commit working copy processing exhibits O(n^2) behaviour

On 9/21/06, Erik Huelsmann <ehuels@gmail.com> wrote:
>
> Well, we talked on IRC already, the log files were a big help!
>
> Here's the new log message:
>
> [[[
> Resolve issue 2607: post-commit processing exhibits O(n^2) behavior.
>
> * subversion/include/svn_wc.h
> * subversion/libsvn_wc/adm_ops.c
> (svn_wc_committed_queue_t): New type for commit item queues.
> (svn_wc_queue_committed): New. Adds a new item to the committed items
> queue. Optionally does required initialization.
> (svn_wc_process_committed_queue): New. Processes (with O(n) behavior)
> the queue of committed items, running all log files (which causes
> the entries file to be rewritten) as a last step.
>
> * subversion/libsvn_wc/adm_ops.c
> (process_committed_internal): New. Abstracted out of
> svn_wc_process_committed4 to serve as a base for
> svn_wc_process_committed_queue too. Changed to account for the fact
> that the base log number may not start at 0 anymore.
> (committed_queue_item_t): New. Structure to store parameters for each
> committed item.
> (affected_adm_t): New. Structure to do some book-keeping
> for the affected adm areas.
> (have_recursive_parent): New. Used to determine whether a path
> will already have been processed, because one of its parents
> was processed recursively.
> (svn_wc_process_committed4): Rewrite in terms of
> process_committed_internal.
>
> * subversion/libsvn_client/commit.c
> (have_processed_parent): Remove. Now obsolete.
> (svn_client_commit4): Adapted to use the new queued post-commit
> processing functions. Because we process all logs after queueing
> the items, there are fewer cases of SVN_ERR_WC_NOT_LOCKED to
> catch. Same thing for deleted items: the entry-existance check
> is no longer relevant: when we check for existance it'll still exist.
> ]]]
>
> And the patch is attached. More comments?
>
> Thanks in advance!
>
> bye,
>
Patch looks complex, maybe you could compete with Lieven's latest patch :)
Anyway my several comments:
1. Personaly I don't like implicit initialization of
svn_wc_committed_queue_t in svn_wc_queue_committed. I prefer to have
separated methods like
svn_wc_commited_queue_create/svn_wc_commited_queue_add. But it's my
personal preference.
2. I think it's good idea to rename argument 'queue' to 'queue_opaque'
and itroduce variable 'queue' of type apr_array_t. This eliminates
many ugly type conversions in code.
3. In function svn_wc_process_committed_queue: I think 'adm_path' is
better name for 'this_path' variable.
4. In same function your have code:
      if (have_recursive
          && have_recursive_parent(&have_recursive, queue, i, cqi->path,
                                   iterpool))
        continue;
But you can remove first check, because have_recursive_parent()
already checks and returns FALSE if have_recursive==FALSE.
5. In function have_recursive_parent() before code:
  if (! *have_recursive)
    return FALSE;

I would add comment like "Check maybe we know that queue doesn't
contain recursive items at all."

and before:
*have_recursive = found_recursive;
Something like "We've gone through all queue and know contain it
recursive items or not."
It was not so easy to understand for me. But of course may be I'm dumb :)

That's all. It looks that this patch should improve performance
dramatically, but I didn't test it and didn't run test suite.

-- 
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org