subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
Subject RE: svn commit: r1022707 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Date Fri, 15 Oct 2010 15:27:26 GMT


> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: donderdag 14 oktober 2010 23:39
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1022707 - in /subversion/trunk: ./
> subversion/libsvn_subr/io.c
> 
> On Thu, Oct 14, 2010 at 09:00:44PM -0000, hwright@apache.org wrote:
> > Author: hwright
> > Date: Thu Oct 14 21:00:43 2010
> > New Revision: 1022707
> >
> > URL: http://svn.apache.org/viewvc?rev=1022707&view=rev
> > Log:
> > Merge r985472 from the performance branch, which see for more
> information.
> >
> > This revision randomizes the numerical suffix of temporary files, in an
> effort
> > to make finding one easier.


> > @@ -384,17 +393,35 @@ svn_io_open_uniquely_named(apr_file_t **
> >           This is good, since "1" would misleadingly imply that
> >           the second attempt was actually the first... and if someone's
> >           got conflicts on their conflicts, we probably don't want to
> > -         add to their confusion :-). */
> > +         add to their confusion :-).
> > +
> > +         Also, the randomization used to minimize the number of re-try
> > +         cycles will interfere with certain tests that compare working
> > +         copies etc.
> > +       */
> >        if (i == 1)
> >          unique_name = apr_psprintf(scratch_pool, "%s%s", path, suffix);
> >        else
> > -        unique_name = apr_psprintf(scratch_pool, "%s.%u%s", path, i,
suffix);
> > +        unique_name = apr_psprintf(scratch_pool, "%s.%u_%x%s", path, i,
> rand(), suffix);
> 
> -1 on using rand() here, for several reasons:
> 
> The intent of svn_io_open_uniquely_named() is to provide user-facing
> temporary files. The order of numbers actually carry meaning in some
cases.
> With files named svn-commit.tmp, svn-commit.2.tmp, svn-commit.3.tmp,
> users
> can easily tell which one is the newest by looking at the number.
> Names with random numbers don't provide this information.
> 
> We already have a different function to create truely randomly-named
> temporary files, svn_io_open_unique_file3(). It should be used instead
> of svn_io_open_uniquely_named() wherever doing so doesn't hurt user
> convenience (i.e. for any tempfile a user will never directly work with).
> 
> Also, rand() isn't thread-safe, and this is library code.

+1 on this reasoning and the -1.
 This merge should be reverted and performance critical code should use
svn_io_open_unique_file3() instead of this function.
(I think we already did this in most cases. In 1.6 this function was a
severe performance problem)

	Bert


Mime
View raw message