On Sat, Oct 16, 2010 at 12:54 PM, Hyrum K. Wright
<hyrum_wright_at_mail.utexas.edu> wrote:
> On Fri, Oct 15, 2010 at 10:27 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Stefan Sperling [mailto:stsp_at_elego.de]
>>> Sent: donderdag 14 oktober 2010 23:39
>>> To: dev_at_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_at_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)
>
> There are parts to the merge which are still useful, so I propose that
> instead of reverting the merge, we make the appropriate reversions on
> the performance branch, and then merge that revision to trunk.
Change made on the branch in r1025660, and merged to trunk in r1025668.
-Hyrum
Received on 2010-10-20 20:42:16 CEST