Re: svn commit: r33602 - trunk/subversion/libsvn_client

[Julian Foad <ju...@btopenworld.com>]

On Sun, 2008-10-12 at 03:48 -0700, rhuijben@tigris.org wrote:
> Author: rhuijben
> Date: Sun Oct 12 03:48:03 2008
> New Revision: 33602
> 
> Log:
> Stop sleeping after each individual merge when batching several merges in 
> a single command (Cherry picking)

Bert, this sounds like a great improvement.

> Implement this by passing a svn_boolean_t* use_sleep to all function that
> require a sleep for timestamp integrity. The calling function will then call
> svn_sleep_for_timestamps() when use_sleep is set to TRUE.

In the implementation, notice that many of our functions have far too
many arguments for a human mind to keep track of already. Instead of
passing another argument, would it be better to put the "needs sleep"
variable in the merge command baton which appears to be already passed
around to the required places?

(I see places where the baton is named "merge_options", and that name
would then want to be improved because it would no longer contain just
options, but that's not a problem.)

- Julian


> * subversion/libsvn_client/merge.c
>   (drive_merge_report_editor, do_file_merge): Add use_sleep parameter and set
>     its value to TRUE instead of calling svn_sleep_for_timestamps().  
>   (do_directory_merge, do_merge, merge_cousins_and_supplement_mergeinfo): 
>     Add use_sleep parameter and pass it to functions now requiring this
>     parameter.
>   (svn_client_merge3, svn_client_merge_reintegrate, svn_client_merge_peg3):
>     Pass use_sleep parameters to functions and call svn_sleep_for_timestamps() if
>     this value is TRUE. Also check for use_sleep if the function returns
>     an error.



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

RE: svn commit: r33602 - trunk/subversion/libsvn_client

[Julian Foad <ju...@btopenworld.com>]

On Sun, 2008-10-12 at 17:10 +0200, Bert Huijben wrote:
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > [...] I'll commit the
> > attached patch when the test suite completes successfully if you've no
> > objection.
> 
> It certainly is valuable to clean this up.
> 
> +1 and thanks for looking into this! :)

Committed revision 33606.

- Julian



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

RE: svn commit: r33602 - trunk/subversion/libsvn_client

[Bert Huijben <be...@vmoo.com>]

> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: Sunday, October 12, 2008 5:08 PM
> To: Bert Huijben
> Cc: dev@subversion.tigris.org
> Subject: RE: svn commit: r33602 - trunk/subversion/libsvn_client
> 
> Bert Huijben wrote:
> > > > Implement this by passing a svn_boolean_t* use_sleep to all
> function
> > > that
> > > > require a sleep for timestamp integrity. The calling function
> will
> > > then call
> > > > svn_sleep_for_timestamps() when use_sleep is set to TRUE.
> > >
> > > In the implementation, notice that many of our functions have far
> too
> > > many arguments for a human mind to keep track of already. Instead
> of
> > > passing another argument, would it be better to put the "needs
> sleep"
> > > variable in the merge command baton which appears to be already
> passed
> > > around to the required places?
> > >
> > > (I see places where the baton is named "merge_options", and that
> name
> > > would then want to be improved because it would no longer contain
> just
> > > options, but that's not a problem.)
> >
> > Merge options and the baton are two completely different things. The
> > merge_options parameter is an array of string parameters passed to
> the diff3
> > tool.
> 
> Oops, sorry, I didn't look hard enough. Yes, "merge_options" is not the
> baton. I meant the merge_cmd_baton_t. And I see that only some of the
> functions take this baton.
> 
> > [...] (But this cleanup is unrelated to this
> > performance work; there some other candidates for inclusion).
> 
> Sure, but any clean-up is valuable in this complex source file, and
> it's
> easier and more likely to happen now than later. I'll commit the
> attached patch when the test suite completes successfully if you've no
> objection.

It certainly is valuable to clean this up.

+1 and thanks for looking into this! :)

	Bert
> 
> Thanks.
> - Julian



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

RE: svn commit: r33602 - trunk/subversion/libsvn_client

[Julian Foad <ju...@btopenworld.com>]

Bert Huijben wrote:
> > > Implement this by passing a svn_boolean_t* use_sleep to all function
> > that
> > > require a sleep for timestamp integrity. The calling function will
> > then call
> > > svn_sleep_for_timestamps() when use_sleep is set to TRUE.
> > 
> > In the implementation, notice that many of our functions have far too
> > many arguments for a human mind to keep track of already. Instead of
> > passing another argument, would it be better to put the "needs sleep"
> > variable in the merge command baton which appears to be already passed
> > around to the required places?
> > 
> > (I see places where the baton is named "merge_options", and that name
> > would then want to be improved because it would no longer contain just
> > options, but that's not a problem.)
> 
> Merge options and the baton are two completely different things. The
> merge_options parameter is an array of string parameters passed to the diff3
> tool. 

Oops, sorry, I didn't look hard enough. Yes, "merge_options" is not the
baton. I meant the merge_cmd_baton_t. And I see that only some of the
functions take this baton.

> [...] (But this cleanup is unrelated to this
> performance work; there some other candidates for inclusion).

Sure, but any clean-up is valuable in this complex source file, and it's
easier and more likely to happen now than later. I'll commit the
attached patch when the test suite completes successfully if you've no
objection.

Thanks.
- Julian

RE: svn commit: r33602 - trunk/subversion/libsvn_client

[Bert Huijben <be...@vmoo.com>]

> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: Sunday, October 12, 2008 4:00 PM
> To: dev@subversion.tigris.org; rhuijben@tigris.org
> Cc: svn@subversion.tigris.org
> Subject: Re: svn commit: r33602 - trunk/subversion/libsvn_client
> 
> On Sun, 2008-10-12 at 03:48 -0700, rhuijben@tigris.org wrote:
> > Author: rhuijben
> > Date: Sun Oct 12 03:48:03 2008
> > New Revision: 33602
> >
> > Log:
> > Stop sleeping after each individual merge when batching several
> merges in
> > a single command (Cherry picking)
> 
> Bert, this sounds like a great improvement.

In the test merge over file:/// I profiled last week I had originally 17
seconds on a. 7 seconds were the absolute path code and 9 are just waiting
in this part of the code. (Which are now reduced to just 1.1 seconds). So
the total merge is now from 17 seconds back to 1.5 or something.

> 
> > Implement this by passing a svn_boolean_t* use_sleep to all function
> that
> > require a sleep for timestamp integrity. The calling function will
> then call
> > svn_sleep_for_timestamps() when use_sleep is set to TRUE.
> 
> In the implementation, notice that many of our functions have far too
> many arguments for a human mind to keep track of already. Instead of
> passing another argument, would it be better to put the "needs sleep"
> variable in the merge command baton which appears to be already passed
> around to the required places?
> 
> (I see places where the baton is named "merge_options", and that name
> would then want to be improved because it would no longer contain just
> options, but that's not a problem.)

Merge options and the baton are two completely different things. The
merge_options parameter is an array of string parameters passed to the diff3
tool. 

* @a merge_options (an array of <tt>const char *</tt>), if non-NULL,
 * is used to pass additional command line arguments to the merge
 * processes (internal or external).  @see
 * svn_diff_file_options_parse().

I can't add anything there. The baton is only passed in the lowest layers,
but might be extended there. (But this cleanup is unrelated to this
performance work; there some other candidates for inclusion).

I'm working on getting this sleeping minimized, as we found this quite a
performance killer in some common AnkhSVN working copy actions. (And I think
99% of our users uses NTFS, where sleeping a millisecond would be more than
enough. Apr gives us a microsecond timestamp resolution there).

	Bert

> 
> - Julian
> 
> 
> > * subversion/libsvn_client/merge.c
> >   (drive_merge_report_editor, do_file_merge): Add use_sleep parameter
> and set
> >     its value to TRUE instead of calling svn_sleep_for_timestamps().
> >   (do_directory_merge, do_merge,
> merge_cousins_and_supplement_mergeinfo):
> >     Add use_sleep parameter and pass it to functions now requiring
> this
> >     parameter.
> >   (svn_client_merge3, svn_client_merge_reintegrate,
> svn_client_merge_peg3):
> >     Pass use_sleep parameters to functions and call
> svn_sleep_for_timestamps() if
> >     this value is TRUE. Also check for use_sleep if the function
> returns
> >     an error.
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org



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