You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gavin Beau Baumanis <ga...@thespidernet.com> on 2010/11/24 11:43:34 UTC
Re: [PATCH] extend svn_subst_translate_string() to record whether re-encoding and/or line ending translation were performed (v. 2)
Hi Daniel (T),
Since in your earlier post you mentioned that you didn't mind a friendly reminder...
I thought I would return this thread to the top of the list - Just i case you had missed Daniel (Shahaf) 's comments.
Gavin "Beau" Baumanis
On 02/11/2010, at 12:35 AM, Daniel Shahaf wrote:
> Gavin Beau Baumanis wrote on Mon, Oct 18, 2010 at 21:06:31 +1100:
>> Ping. This patch submission has received no comments.
>>
>
> Thanks, Gavin. @Daniel, sorry for the delay.
>
>> On 04/10/2010, at 5:55 AM, Daniel Trebbien wrote:
>>
>>> On Fri, Oct 1, 2010 at 3:57 AM, Julian Foad <ju...@wandisco.com> wrote:
>>>>> Adds a public API function, svn_subst_translate_string2(), an extension of
>>>>> svn_subst_translate_string(), that has two, additional output parameters for
>>>>> determining whether re-encoding and/or line ending translation were performed.
>>>>
>>>> If we're going to add this functionality to _translate_string(),
>>>> shouldn't we also add it to the other variants - _detranslate_string,
>>>> _translate_cstring2, _stream_translated, _copy_and_translate4?
>>>>
>>>> I see you're adding the infrastructure into the (new) bodies of
>>>> _translate_cstring2 and _stream_translated, just not (yey) exposing it
>>>> in their APIs.
>>>>
>
> If we can provide the information cheaply, I see no reason not to let
> the APIs provide it. As some of these APIs are already revved for 1.7,
> we might as well add the new information to them now.
>
>>>> I'm not sure. The set of 'translation' functions is already messy and
>>>> it's not clear to me how useful this new functionality will be.
>>>
>>> I originally began working on svn_subst_translate_string2() because I
>>> could not find a combination of public API functions that would allow
>>> me to determine whether the line endings changed when a string was
>>> both re-encoded to UTF-8 and normalized to LF line endings. The most
>>> applicable, svn_subst_translate_string(), performs both translations
>>> without indicating whether it re-encoded the string or normalized a
>>> line ending.
>>>
>
> And all of this is necessary just so svnsync can /report/ which of the
> two fixes it made, right? If so, we might move on with teaching svnsync
> to fix newlines too, and add the "Report back how many properties had
> their newlines changed" functionality separately. (easier to review,
> and I don't think "512 properties were re-newlined" is /that/ useful to
> know for most people)
>
>>> An alternative to extending svn_subst_translate_string() is to add a
>>> public API function that does the re-encoding and another that
>>> normalizes line endings. I think, however, that extending
>>> svn_subst_translate_string() is better because though the current
>>> implementation of svn_subst_translate_string() re-encodes, then
>>> normalizes line endings, the single API function allows for the
>>> possibility of a future improvement in efficiency as a result of
>>> performing both translations simultaneously.
>>>
>>>
>>>
>>> Attached are a revised patch and log message.
>>> <log message.txt><patch.txt>
>
>> [[[
>> Adds a public API function, svn_subst_translate_string2(), an extension of
>> svn_subst_translate_string(), that has two, additional output parameters for
>> determining whether re-encoding and/or line ending translation were performed.
>>
>
> Grammar police: No comma after "two".
>
>>
>> As discussed at:
>> http://thread.gmane.org/gmane.comp.version-control.subversion.devel/122550
>> http://thread.gmane.org/gmane.comp.version-control.subversion.devel/123020
>>
>>
>> * subversion/include/svn_subst.h
>> (svn_subst_translate_string2): New function.
>> (svn_subst_translate_string): Deprecated in favor of
>> svn_subst_translate_string2().
>>
>> * subversion/libsvn_subr/subst.c
>
> What's happening in this file is essentially "Add the parameters to
> <this> public API, punch them through everything, and finally in
> translate_newline() make use of them". Could you add something
> (a paragraph?) to the log message that makes that clear?
>
> Or, another option is two have two subst.c entries in the log message
> --- one for the 'punching through' and one for the interesting changes
> --- though we have less precedent for that.
>
> I'll leave it to you to find a way to make the overall picture clear :)
>
>> (translate_newline): Added the new parameter TRANSLATED_EOL, the value at
>> which is set to TRUE if the newline string in EOL_STR is different than
>> the newline string in NEWLINE_BUF.
>> (translation_baton): Added the TRANSLATED_EOL field.
>> (create_translation_baton): Added a new parameter TRANSLATED_EOL that is
>> passed to the resulting translation_baton.
>> (translate_chunk): Modified the three calls to translate_newline() to pass
>> the TRANSLATED_EOL pointer from the translation baton.
>> (stream_translated): New static function. Its implementation is the old
>> implementation of svn_subst_stream_translated(), but accepting another
>> parameter, TRANSLATED_EOL, that is passed to the in/out translation batons
>> that it creates.
>> (svn_subst_stream_translated): Now a wrapper for stream_translated().
>> (translate_cstring): New static function. Its implementation is the old
>> implementation of svn_subst_translate_cstring2(), but modified to accept
>> another parameter, TRANSLATED_EOL, that is passed to stream_translated().
>> (svn_subst_translate_cstring2): Now a wrapper for translate_cstring().
>> (svn_subst_translate_string2): New function. The task of recording whether
>> it translated a line ending is delegated to translate_cstring().
>> ]]]
>
>> Index: subversion/include/svn_subst.h
>> ===================================================================
>> --- subversion/include/svn_subst.h (revision 1004022)
>> +++ subversion/include/svn_subst.h (working copy)
>> @@ -588,16 +588,41 @@ svn_subst_stream_detranslated(svn_stream_t **strea
>>
>> /* EOL conversion and character encodings */
>>
>> -/** Translate the data in @a value (assumed to be in encoded in charset
>> +/** @deprecated Provided for backward compatibility with the 1.6 API. Callers
>> + * should use svn_subst_translate_string2().
>> + *
>> + * Translate the data in @a value (assumed to be encoded in charset
>> * @a encoding) to UTF8 and LF line-endings. If @a encoding is @c NULL,
>> * then assume that @a value is in the system-default language encoding.
>> * Return the translated data in @a *new_value, allocated in @a pool.
>> */
>
> Please make the old docstring only describe the differences from the new
> version of the function, and delete the rest of the docstring here.
> Follow the examples in the other header files.
>
>> +SVN_DEPRECATED
>> svn_error_t *svn_subst_translate_string(svn_string_t **new_value,
>> const svn_string_t *value,
>> const char *encoding,
>> apr_pool_t *pool);
>>
>> +/** Translate the data in @a value (assumed to be encoded in charset
>> + * @a encoding) to UTF8 and LF line-endings. If @a encoding is @c NULL,
>> + * then assume that @a value is in the system-default character encoding.
>
> You changed 'language encoding' to 'character encoding'.
>
>> + * If @a translated_to_utf8 is not @c NULL, then
>> + * @code *translated_to_utf8 @endcode is set to @c TRUE if at least one
>> + * character of @a value in the source charset was translated to UTF-8; to
>> + * @c FALSE otherwise. If @a translated_line_endings is not @c NULL, then
>> + * @code *translated_line_endings @endcode is set to @c TRUE if at least one
>> + * line ending was changed to LF; to @c FALSE otherwise.
>
> Use paragraphs please.
>
>> + * Return the translated
>> + * data in @a *new_value, allocated in @a pool.
>> + *
>> + * @since New in 1.7
>
> Trailing period missing.
>
> (Vim tip: ^X^L completes a line.)
>
>> + */
>> +svn_error_t *
>> +svn_subst_translate_string2(svn_string_t **new_value,
>> + svn_boolean_t *translated_to_utf8,
>> + svn_boolean_t *translated_line_endings,
>> + const svn_string_t *value,
>> + const char *encoding,
>> + apr_pool_t *pool);
>
> Can you switch this function to the two-pools paradigm
> (result_pool/scratch_pool) while here? There are examples in
> libsvn_wc/wc_db.h (and elsewhere).
>
> It already uses scratch_pool internally.
>
>> +
>> /** Translate the data in @a value from UTF8 and LF line-endings into
>> * native locale and native line-endings, or to the output locale if
>> * @a for_output is TRUE. Return the translated data in @a
>> Index: subversion/libsvn_subr/subst.c
>> ===================================================================
>> --- subversion/libsvn_subr/subst.c (revision 1004022)
>> +++ subversion/libsvn_subr/subst.c (working copy)
>> @@ -620,6 +620,10 @@ translate_keyword(char *buf,
>> newline in the file, and copy it to {SRC_FORMAT, *SRC_FORMAT_LEN} to
>> use for later consistency checks.
>>
>> + Sets *TRANSLATED_EOL to TRUE if TRANSLATED_EOL is not NULL and the newline
>> + string that was written (EOL_STR) is not the same as the newline string
>> + that was translated (NEWLINE_BUF).
>> +
>> Note: all parameters are required even if REPAIR is TRUE.
>> ### We could require that REPAIR must not change across a sequence of
>> calls, and could then optimize by not using SRC_FORMAT at all if
>> @@ -633,7 +637,8 @@ translate_newline(const char *eol_str,
>> const char *newline_buf,
>> apr_size_t newline_len,
>> svn_stream_t *dst,
>> - svn_boolean_t repair)
>> + svn_boolean_t repair,
>> + svn_boolean_t *translated_eol)
>> {
>> /* If we've seen a newline before, compare it with our cache to
>> check for consistency, else cache it for future comparisons. */
>> @@ -653,8 +658,17 @@ translate_newline(const char *eol_str,
>> strncpy(src_format, newline_buf, newline_len);
>> *src_format_len = newline_len;
>> }
>> - /* Write the desired newline */
>> - return translate_write(dst, eol_str, eol_str_len);
>> +
>> + /* Translate the newline */
>> + SVN_ERR(translate_write(dst, eol_str, eol_str_len));
>> +
>> + if (translated_eol)
>> + {
>
> Right now, this if() will be done every time translate_newline() is
> called. Could you rework the logic to check it fewer times?
>
> e.g., to only check it once per translation_baton if possible, or to
> skip checking it if it had been set already, or if REPAIR is false and
> a newline had been seen already, etc. (These are just ideas, you don't
> have to implement all of them!)
>
> Stefan2 indicates that the subst() code is a rather expensive part of
> 'checkout' and 'export', so doing the check once-per-file (instead of
> once-per-line) will be nice. (However, I realize that right now you
> only expose translated_eol for the svn_string_t API.)
>
>> + if (newline_len != eol_str_len ||
>> + strncmp(newline_buf, eol_str, newline_len))
>> + *translated_eol = TRUE;
>> + }
>> + return SVN_NO_ERROR;
>> }
>>
>>
>> @@ -1683,6 +1734,19 @@ svn_subst_translate_string(svn_string_t **new_valu
>> const char *encoding,
>> apr_pool_t *pool)
>> {
>> + return svn_subst_translate_string2(new_value, NULL, NULL, value, encoding,
>> + pool);
>> +}
>> +
>
> Please move the above wrapper to deprecated.c.
>
>> +
>> +svn_error_t *
>> +svn_subst_translate_string2(svn_string_t **new_value,
>> + svn_boolean_t *translated_to_utf8,
>> + svn_boolean_t *translated_line_endings,
>> + const svn_string_t *value,
>> + const char *encoding,
>> + apr_pool_t *pool)
>> +{
>> const char *val_utf8;
>> const char *val_utf8_lf;
>> apr_pool_t *scratch_pool = svn_pool_create(pool);
>> @@ -1703,14 +1767,18 @@ svn_subst_translate_string(svn_string_t **new_valu
>> SVN_ERR(svn_utf_cstring_to_utf8(&val_utf8, value->data, scratch_pool));
>> }
>>
>
> Not related to your patch, but the above if/else block calls the UTF-8
> translation routines on value->data. Since this is specifically the
> translate_string() API (and there is a separate translate_cstring()
> API), I think either we should fix this (the whole reason svn_string_t
> exists is to allow embedded NULs) or at least document this limitation
> in the API docs.
>
>> - SVN_ERR(svn_subst_translate_cstring2(val_utf8,
>> - &val_utf8_lf,
>> - "\n", /* translate to LF */
>> - FALSE, /* no repair */
>> - NULL, /* no keywords */
>> - FALSE, /* no expansion */
>> - scratch_pool));
>> + if (translated_to_utf8)
>> + *translated_to_utf8 = (strcmp(value->data, val_utf8) != 0);
>>
>> + SVN_ERR(translate_cstring(&val_utf8_lf,
>> + translated_line_endings,
>> + val_utf8,
>> + "\n", /* translate to LF */
>> + FALSE, /* no repair */
>> + NULL, /* no keywords */
>> + FALSE, /* no expansion */
>> + scratch_pool));
>> +
>> *new_value = svn_string_create(val_utf8_lf, pool);
>> svn_pool_destroy(scratch_pool);
>> return SVN_NO_ERROR;
>
> Lots of plumbing, but looks good :-)
>
> Thanks,
>
> Daniel
>
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Tue, Nov 30, 2010 at 10:40:59 +0000:
> On Mon, 2010-11-29, Danny Trebbien wrote:
> [...]
> > My conclusion from all of this is that regardless of the value of
> > `repair`, my changes do not appear to decrease the performance of
> > svn_subst_translate_string() as long as svn_subst_translate_string2()
> > is called directly.
>
> Hi Danny. (I notice you changed your email "From" name to "Danny".)
>
> Statistics was never my strength so I'll just look to your conclusion.
> It sounds like it doesn't need any optimization, certainly nothing
> major. Therefore we should definitely make the functional change first.
>
> I just looked back at the previous emails and had a chat with Daniel
> Sh., and he agrees.
It would be more accurate to say that I don't know yet whether I agree
or not, since I haven't had the chance to digest dtrebbien's t-test
results yet.
> Would you like to re-post your patch, when you're ready, without any
> of this optimization but with any other changes that are still needed?
>
+1.
(I don't need to be convinced myself of the statistics to be convinced
that having the patch proceed without the optimization is the way to go
here.)
> Daniel Shahaf wrote:
> > As I don't recall (m)any other issues with the patch, I think it's
> > a short distance from resolving this issue to committing the patch.
>
> Yup, a short distance now.
But once there, Danny has another patch in the pipeline...
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Re-reading my original review comment, I think it sounds more requiring
than I intended it to be. My fault.
Also, this thread (and in particular Danny's last mail) nicely demonstrate
another aspect of our patch submission process: namely, that patch reviews
are nothing but the start of a discussion about the point they highlight,
and it's fine and healthy to present a disagreeing opinion upon receiving
a patch review.
Daniel
Julian Foad wrote on Tue, Nov 30, 2010 at 10:40:59 +0000:
> On Mon, 2010-11-29, Danny Trebbien wrote:
> [...]
> > My conclusion from all of this is that regardless of the value of
> > `repair`, my changes do not appear to decrease the performance of
> > svn_subst_translate_string() as long as svn_subst_translate_string2()
> > is called directly.
>
> Hi Danny. (I notice you changed your email "From" name to "Danny".)
>
> Statistics was never my strength so I'll just look to your conclusion.
> It sounds like it doesn't need any optimization, certainly nothing
> major. Therefore we should definitely make the functional change first.
>
> I just looked back at the previous emails and had a chat with Daniel
> Sh., and he agrees. Would you like to re-post your patch, when you're
> ready, without any of this optimization but with any other changes that
> are still needed?
>
> Daniel Shahaf wrote:
> > As I don't recall (m)any other issues with the patch, I think it's
> > a short distance from resolving this issue to committing the patch.
>
> Yup, a short distance now.
>
> Thanks.
>
> - Julian
>
>
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-11-29, Danny Trebbien wrote:
[...]
> My conclusion from all of this is that regardless of the value of
> `repair`, my changes do not appear to decrease the performance of
> svn_subst_translate_string() as long as svn_subst_translate_string2()
> is called directly.
Hi Danny. (I notice you changed your email "From" name to "Danny".)
Statistics was never my strength so I'll just look to your conclusion.
It sounds like it doesn't need any optimization, certainly nothing
major. Therefore we should definitely make the functional change first.
I just looked back at the previous emails and had a chat with Daniel
Sh., and he agrees. Would you like to re-post your patch, when you're
ready, without any of this optimization but with any other changes that
are still needed?
Daniel Shahaf wrote:
> As I don't recall (m)any other issues with the patch, I think it's
> a short distance from resolving this issue to committing the patch.
Yup, a short distance now.
Thanks.
- Julian
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Danny Trebbien <dt...@gmail.com>.
Attached is a benchmark and Makefile that I used to test the speed of
svn_subst_translate_string() from trunk versus the new
svn_subst_translate_string2(). The program reads a text file named
`2600.txt` in the current working directory and repeatedly calls
svn_subst_translate_string() on the contents. For `2600.txt`, I used
the plain text version of War and Peace from Project Gutenberg
(http://www.gutenberg.org/ebooks/2600.txt.utf8).
The data that I generated for trunk@1040115 were:
trunk_at_1040115 <- c(7780000, 7910000, 7870000, 7660000, 7840000,
7760000, 7620000, 7500000, 7860000, 7800000, 7640000, 7740000,
7760000, 7850000, 8010000, 7800000, 7730000, 7700000, 7900000,
7760000, 7790000, 7970000, 7700000, 7710000, 7990000, 7830000,
7780000, 7810000, 7730000, 7600000)
The data for the "HEAD" sources (commit
6f828b0a4e07d1e14189b9b8c84bd0f884c59164 from my repo;
https://github.com/dtrebbien/subversion/tree/6f828b0a4e07d1e14189b9b8c84bd0f884c59164)
were:
HEAD <- c(8050000, 8230000, 7980000, 8150000, 7950000, 8600000,
8080000, 8420000, 8000000, 8020000, 8420000, 7960000, 8010000,
8200000, 8080000, 8490000, 8190000, 7920000, 7820000, 7780000,
7880000, 8540000, 7970000, 8250000, 8830000, 8540000, 8310000,
8270000, 8010000, 7990000)
Note: This is not "version 3" of the patch. It is essentially
trunk@1040115 plus "version 3" plus this changeset:
https://github.com/dtrebbien/subversion/commit/d22329a54dcf58cddc2b618f913597c6defbcb2d
The t-test allows us to conclude with high confidence that the mean
time to run the benchmark with libsvn_subr-1 compiled from
trunk@1040115 is less than the mean time to run the benchmark with
libsvn_subr-1 compiled from the HEAD sources:
> t.test(trunk_at_1040115, HEAD, alternative = "less", var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: trunk_at_1040115 and HEAD
t = -7.473, df = 58, p-value = 2.350e-10
alternative hypothesis: true difference in means is less than 0
90 percent confidence interval:
-Inf -317939.7
sample estimates:
mean of x mean of y
7780000 8164667
I realized, however, that this is not a fair comparison because the
HEAD sources simply call svn_subst_translate_string2() within
svn_subst_translate_string(), meaning that there is an extra layer of
indirection. After modifying the benchmark to call
svn_subst_translate_string2() directly, I generated these timings:
HEAD_new <- c(7850000, 7890000, 8080000, 7980000, 7820000, 7880000,
7850000, 7540000, 8470000, 8230000, 8410000, 7880000, 7410000,
7490000, 7420000, 7650000, 7430000, 7430000, 7530000, 7720000,
7940000, 7780000, 8070000, 7840000, 7870000, 7970000, 7690000,
7910000, 7860000, 7620000)
Now we cannot reject the null hypothesis that the mean time to run the
benchmark with libsvn_subr-1 compiled from trunk@1040115 is greater
than or equal to the mean time to run the modified benchmark with
libsvn_subr-1 compiled from the HEAD sources:
> t.test(trunk_at_1040115, HEAD_new, alternative = "less", var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: trunk_at_1040115 and HEAD_new
t = -0.6839, df = 58, p-value = 0.2484
alternative hypothesis: true difference in means is less than 0
90 percent confidence interval:
-Inf 33129.55
sample estimates:
mean of x mean of y
7780000 7817000
One other set of timings that I generated were for the modified
benchmark running with libsvn_subr-1 compiled from the HEAD sources,
slightly modified to set `repair` to TRUE:
HEAD_new_repair <- c(7660000, 7560000, 7570000, 7540000, 7670000,
7790000, 7460000, 7840000, 8060000, 7790000, 8000000, 7830000,
8370000, 8010000, 7730000, 7800000, 7900000, 7730000, 7730000,
7790000, 7750000, 7930000, 7860000, 7810000, 7930000, 7840000,
7890000, 7460000, 7790000, 7730000)
We cannot reject the null hypothesis that the mean time to run the
modified benchmark with libsvn_subr-1 compiled from the HEAD sources
is the same as the mean time to run the modified benchmark with
libsvn_subr-1 compiled from slightly-modified HEAD sources (`repair`
is set to TRUE):
> t.test(HEAD_new, HEAD_new_repair, var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: HEAD_new and HEAD_new_repair
t = 0.3815, df = 58, p-value = 0.7042
alternative hypothesis: true difference in means is not equal to 0
90 percent confidence interval:
-77774.74 123774.74
sample estimates:
mean of x mean of y
7817000 7794000
> t.test(trunk_at_1040115, HEAD_new_repair, alternative = "less", var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: trunk_at_1040115 and HEAD_new_repair
t = -0.3501, df = 58, p-value = 0.3638
alternative hypothesis: true difference in means is less than 0
90 percent confidence interval:
-Inf 37836.36
sample estimates:
mean of x mean of y
7780000 7794000
Therefore, I do not have evidence to support my earlier claim: "3.)
This penalizes repair translations."
My conclusion from all of this is that regardless of the value of
`repair`, my changes do not appear to decrease the performance of
svn_subst_translate_string() as long as svn_subst_translate_string2()
is called directly.
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Danny Trebbien <dt...@gmail.com>.
Attached is a benchmark and Makefile that I used to test the speed of
svn_subst_translate_string() from trunk versus the new
svn_subst_translate_string2(). The program reads a text file named
`2600.txt` in the current working directory and repeatedly calls
svn_subst_translate_string() on the contents. For `2600.txt`, I used
the plain text version of War and Peace from Project Gutenberg
(http://www.gutenberg.org/ebooks/2600.txt.utf8).
The data that I generated for trunk@1040115 were:
trunk_at_1040115 <- c(7780000, 7910000, 7870000, 7660000, 7840000,
7760000, 7620000, 7500000, 7860000, 7800000, 7640000, 7740000,
7760000, 7850000, 8010000, 7800000, 7730000, 7700000, 7900000,
7760000, 7790000, 7970000, 7700000, 7710000, 7990000, 7830000,
7780000, 7810000, 7730000, 7600000)
The data for the "HEAD" sources (commit
6f828b0a4e07d1e14189b9b8c84bd0f884c59164 from my repo;
https://github.com/dtrebbien/subversion/tree/6f828b0a4e07d1e14189b9b8c84bd0f884c59164)
were:
HEAD <- c(8050000, 8230000, 7980000, 8150000, 7950000, 8600000,
8080000, 8420000, 8000000, 8020000, 8420000, 7960000, 8010000,
8200000, 8080000, 8490000, 8190000, 7920000, 7820000, 7780000,
7880000, 8540000, 7970000, 8250000, 8830000, 8540000, 8310000,
8270000, 8010000, 7990000)
Note: This is not "version 3" of the patch. It is essentially
trunk@1040115 plus "version 3" plus this changeset:
https://github.com/dtrebbien/subversion/commit/d22329a54dcf58cddc2b618f913597c6defbcb2d
The t-test allows us to conclude with high confidence that the mean
time to run the benchmark with libsvn_subr-1 compiled from
trunk@1040115 is less than the mean time to run the benchmark with
libsvn_subr-1 compiled from the HEAD sources:
> t.test(trunk_at_1040115, HEAD, alternative = "less", var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: trunk_at_1040115 and HEAD
t = -7.473, df = 58, p-value = 2.350e-10
alternative hypothesis: true difference in means is less than 0
90 percent confidence interval:
-Inf -317939.7
sample estimates:
mean of x mean of y
7780000 8164667
I realized, however, that this is not a fair comparison because the
HEAD sources simply call svn_subst_translate_string2() within
svn_subst_translate_string(), meaning that there is an extra layer of
indirection. After modifying the benchmark to call
svn_subst_translate_string2() directly, I generated these timings:
HEAD_new <- c(7850000, 7890000, 8080000, 7980000, 7820000, 7880000,
7850000, 7540000, 8470000, 8230000, 8410000, 7880000, 7410000,
7490000, 7420000, 7650000, 7430000, 7430000, 7530000, 7720000,
7940000, 7780000, 8070000, 7840000, 7870000, 7970000, 7690000,
7910000, 7860000, 7620000)
Now we cannot reject the null hypothesis that the mean time to run the
benchmark with libsvn_subr-1 compiled from trunk@1040115 is greater
than or equal to the mean time to run the modified benchmark with
libsvn_subr-1 compiled from the HEAD sources:
> t.test(trunk_at_1040115, HEAD_new, alternative = "less", var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: trunk_at_1040115 and HEAD_new
t = -0.6839, df = 58, p-value = 0.2484
alternative hypothesis: true difference in means is less than 0
90 percent confidence interval:
-Inf 33129.55
sample estimates:
mean of x mean of y
7780000 7817000
One other set of timings that I generated were for the modified
benchmark running with libsvn_subr-1 compiled from the HEAD sources,
slightly modified to set `repair` to TRUE:
HEAD_new_repair <- c(7660000, 7560000, 7570000, 7540000, 7670000,
7790000, 7460000, 7840000, 8060000, 7790000, 8000000, 7830000,
8370000, 8010000, 7730000, 7800000, 7900000, 7730000, 7730000,
7790000, 7750000, 7930000, 7860000, 7810000, 7930000, 7840000,
7890000, 7460000, 7790000, 7730000)
We cannot reject the null hypothesis that the mean time to run the
modified benchmark with libsvn_subr-1 compiled from the HEAD sources
is the same as the mean time to run the modified benchmark with
libsvn_subr-1 compiled from slightly-modified HEAD sources (`repair`
is set to TRUE):
> t.test(HEAD_new, HEAD_new_repair, var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: HEAD_new and HEAD_new_repair
t = 0.3815, df = 58, p-value = 0.7042
alternative hypothesis: true difference in means is not equal to 0
90 percent confidence interval:
-77774.74 123774.74
sample estimates:
mean of x mean of y
7817000 7794000
> t.test(trunk_at_1040115, HEAD_new_repair, alternative = "less", var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: trunk_at_1040115 and HEAD_new_repair
t = -0.3501, df = 58, p-value = 0.3638
alternative hypothesis: true difference in means is less than 0
90 percent confidence interval:
-Inf 37836.36
sample estimates:
mean of x mean of y
7780000 7794000
Therefore, I do not have evidence to support my earlier claim: "3.)
This penalizes repair translations."
My conclusion from all of this is that regardless of the value of
`repair`, my changes do not appear to decrease the performance of
svn_subst_translate_string() as long as svn_subst_translate_string2()
is called directly.
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Danny Trebbien <dt...@gmail.com>.
> I timed 'svn export' of a 146MB subdirectory of a format-21 working
> copy to a linux tmpfs. Below, 't1' is pristine trunk@HEAD and 't2' is
> trunk@HEAD plus the patch at the top of this thread:
>
> % cd /tmp/ram
> % mount | grep $PWD
> tmpfs on /tmp/ram type tmpfs (rw,nosuid,nodev,noatime)
> % rm -f /tmp/five-{t1,t2}
> % for j in $(seq 5); do
> for ii in t1 t2; do
> .svn $ii;
> rm -rf ./$ii;
> (time $svn export -q ~/src/asf/infra/trunk/ ./$ii) 2>&1 | tee -a ../five-$ii;
> rm -rf ./$ii;
> done;
> done
> % cat /tmp/five-t1
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.26s user 2.36s system 37% cpu 15.035 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.29s user 2.58s system 29% cpu 20.046 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.41s user 2.36s system 29% cpu 19.664 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.36s user 2.45s system 30% cpu 19.222 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.32s user 2.39s system 29% cpu 19.156 total
> % cat /tmp/five-t2
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.40s user 2.35s system 32% cpu 17.823 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.51s user 2.64s system 26% cpu 22.859 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.26s user 2.49s system 29% cpu 19.313 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.28s user 2.37s system 29% cpu 19.312 total
> $svn export -q ~/src/asf/infra/trunk/ ./$ii 3.24s user 2.30s system 28% cpu 19.329 total
> %
>
> It seems that the second set of results is slightly slower?
>
> Daniel
Using R 2.12.0 I ran a two-sample unpaired t-test of the totals. The
alternative hypothesis is that the mean time to run `five-t1` (μ_t1)
is less than the mean time to run `five-t2` (μ_t2):
> x <- c(15.035, 20.046, 19.664, 19.222, 19.156)
> y <- c(17.823, 22.859, 19.313, 19.312, 19.329)
> t.test(x, y, alternative = "less", var.equal = TRUE, conf.level = 0.90)
Two Sample t-test
data: x and y
t = -0.892, df = 8, p-value = 0.1992
alternative hypothesis: true difference in means is less than 0 (μ_t1
- μ_t2 < 0)
90 percent confidence interval:
-Inf 0.624089
sample estimates:
mean of x mean of y
18.6246 19.7272
So actually we "fail to reject the null hypothesis", meaning that we
cannot reject the claim that μ_t1 >= μ_t2.
`five-t2` should be taking longer than `five-t1` because we know that
`five-t2` does more work than `five-t1`. However, the difference is
so small that we have trouble telling the timings apart.
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I timed 'svn export' of a 146MB subdirectory of a format-21 working
copy to a linux tmpfs. Below, 't1' is pristine trunk@HEAD and 't2' is
trunk@HEAD plus the patch at the top of this thread:
% cd /tmp/ram
% mount | grep $PWD
tmpfs on /tmp/ram type tmpfs (rw,nosuid,nodev,noatime)
% rm -f /tmp/five-{t1,t2}
% for j in $(seq 5); do
for ii in t1 t2; do
.svn $ii;
rm -rf ./$ii;
(time $svn export -q ~/src/asf/infra/trunk/ ./$ii) 2>&1 | tee -a ../five-$ii;
rm -rf ./$ii;
done;
done
% cat /tmp/five-t1
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.26s user 2.36s system 37% cpu 15.035 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.29s user 2.58s system 29% cpu 20.046 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.41s user 2.36s system 29% cpu 19.664 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.36s user 2.45s system 30% cpu 19.222 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.32s user 2.39s system 29% cpu 19.156 total
% cat /tmp/five-t2
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.40s user 2.35s system 32% cpu 17.823 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.51s user 2.64s system 26% cpu 22.859 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.26s user 2.49s system 29% cpu 19.313 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.28s user 2.37s system 29% cpu 19.312 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.24s user 2.30s system 28% cpu 19.329 total
%
It seems that the second set of results is slightly slower?
Daniel
Danny Trebbien wrote on Sat, Nov 27, 2010 at 09:46:34 -0800:
> > To the point, I originally asked if your changes affected the performance
> > of checkout/export. That is not a reason to stop the patch in its tracks;
> > it's a question that should be answered (either way) and the patch then
> > proceed. So, firstly, do your changes have any noticeable performance
> > effect, or is the effect of the added per-line condition simply not
> > noticeable?
>
> I don't have benchmarks, but it does not seem that the performance of
> my tests are noticeably degraded. These tests aren't extensive,
> however, so I am not sure whether a much larger Subversion operation
> that uses the changes (such as running the patched `svnsync` to sync
> the GNU Nano repository) is impacted.
>
> Note that when EOL_TRANSLATED is NULL, the overhead is a single NULL
> check for each line.
>
> > If the latter, then I apologize (to Daniel) for your having spent time
> > writing patches (in various "creative" ways :)) that address what is
> > a non-problem.
>
> It's not a big deal, and I think that it helped me to understand the
> code more fully. Besides, I may have another option.
>
> This other idea is based on knowledge that b->repair is usually FALSE.
> Given this, I examined one of the if statements:
>
> if ((! b->repair) && DIFFERENT_EOL_STRINGS(src_format,
> *src_format_len, newline_buf, newline_len))
>
> The `DIFFERENT_EOL_STRINGS` check will run whenever b->repair is
> FALSE. So, I check `DIFFERENT_EOL_STRINGS` first and if that check is
> TRUE, I then check !b->repair, else check to see whether
> *b->eol_translated needs to be set to TRUE:
> <https://github.com/dtrebbien/subversion/commit/d22329a54dcf58cddc2b618f913597c6defbcb2d>.
> What do you all think?
>
> The upside is that unnecessary NULL checks of TRANSLATED_EOL are
> dynamically elided under normal conditions.
>
> The downsides are:
>
> 1.) Another assumption must be made (namely, that the EOL_STR string
> is not varied between calls to translate_newline() using the same
> translation baton).
>
> 2.) It complicates the logic.
>
> 3.) This penalizes repair translations.
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I timed 'svn export' of a 146MB subdirectory of a format-21 working
copy to a linux tmpfs. Below, 't1' is pristine trunk@HEAD and 't2' is
trunk@HEAD plus the patch at the top of this thread:
% cd /tmp/ram
% mount | grep $PWD
tmpfs on /tmp/ram type tmpfs (rw,nosuid,nodev,noatime)
% rm -f /tmp/five-{t1,t2}
% for j in $(seq 5); do
for ii in t1 t2; do
.svn $ii;
rm -rf ./$ii;
(time $svn export -q ~/src/asf/infra/trunk/ ./$ii) 2>&1 | tee -a ../five-$ii;
rm -rf ./$ii;
done;
done
% cat /tmp/five-t1
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.26s user 2.36s system 37% cpu 15.035 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.29s user 2.58s system 29% cpu 20.046 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.41s user 2.36s system 29% cpu 19.664 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.36s user 2.45s system 30% cpu 19.222 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.32s user 2.39s system 29% cpu 19.156 total
% cat /tmp/five-t2
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.40s user 2.35s system 32% cpu 17.823 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.51s user 2.64s system 26% cpu 22.859 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.26s user 2.49s system 29% cpu 19.313 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.28s user 2.37s system 29% cpu 19.312 total
$svn export -q ~/src/asf/infra/trunk/ ./$ii 3.24s user 2.30s system 28% cpu 19.329 total
%
It seems that the second set of results is slightly slower?
Daniel
Danny Trebbien wrote on Sat, Nov 27, 2010 at 09:46:34 -0800:
> > To the point, I originally asked if your changes affected the performance
> > of checkout/export. That is not a reason to stop the patch in its tracks;
> > it's a question that should be answered (either way) and the patch then
> > proceed. So, firstly, do your changes have any noticeable performance
> > effect, or is the effect of the added per-line condition simply not
> > noticeable?
>
> I don't have benchmarks, but it does not seem that the performance of
> my tests are noticeably degraded. These tests aren't extensive,
> however, so I am not sure whether a much larger Subversion operation
> that uses the changes (such as running the patched `svnsync` to sync
> the GNU Nano repository) is impacted.
>
> Note that when EOL_TRANSLATED is NULL, the overhead is a single NULL
> check for each line.
>
> > If the latter, then I apologize (to Daniel) for your having spent time
> > writing patches (in various "creative" ways :)) that address what is
> > a non-problem.
>
> It's not a big deal, and I think that it helped me to understand the
> code more fully. Besides, I may have another option.
>
> This other idea is based on knowledge that b->repair is usually FALSE.
> Given this, I examined one of the if statements:
>
> if ((! b->repair) && DIFFERENT_EOL_STRINGS(src_format,
> *src_format_len, newline_buf, newline_len))
>
> The `DIFFERENT_EOL_STRINGS` check will run whenever b->repair is
> FALSE. So, I check `DIFFERENT_EOL_STRINGS` first and if that check is
> TRUE, I then check !b->repair, else check to see whether
> *b->eol_translated needs to be set to TRUE:
> <https://github.com/dtrebbien/subversion/commit/d22329a54dcf58cddc2b618f913597c6defbcb2d>.
> What do you all think?
>
> The upside is that unnecessary NULL checks of TRANSLATED_EOL are
> dynamically elided under normal conditions.
>
> The downsides are:
>
> 1.) Another assumption must be made (namely, that the EOL_STR string
> is not varied between calls to translate_newline() using the same
> translation baton).
>
> 2.) It complicates the logic.
>
> 3.) This penalizes repair translations.
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> > (btw, the firstnames distribution in COMMITTERS is... interesting.)
>
> You piqued my curiosity. Here is a frequency table of first names of
> the active full committers:
...
> That's a lot of Daniels.
Also, many names start with J --- and not one of the J's repeats :-).
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Danny Trebbien <dt...@gmail.com>.
> I hope you aren't discouraged from working on the patch.
Nah :)
> To the point, I originally asked if your changes affected the performance
> of checkout/export. That is not a reason to stop the patch in its tracks;
> it's a question that should be answered (either way) and the patch then
> proceed. So, firstly, do your changes have any noticeable performance
> effect, or is the effect of the added per-line condition simply not
> noticeable?
I don't have benchmarks, but it does not seem that the performance of
my tests are noticeably degraded. These tests aren't extensive,
however, so I am not sure whether a much larger Subversion operation
that uses the changes (such as running the patched `svnsync` to sync
the GNU Nano repository) is impacted.
Note that when EOL_TRANSLATED is NULL, the overhead is a single NULL
check for each line.
> If the latter, then I apologize (to Daniel) for your having spent time
> writing patches (in various "creative" ways :)) that address what is
> a non-problem.
It's not a big deal, and I think that it helped me to understand the
code more fully. Besides, I may have another option.
This other idea is based on knowledge that b->repair is usually FALSE.
Given this, I examined one of the if statements:
if ((! b->repair) && DIFFERENT_EOL_STRINGS(src_format,
*src_format_len, newline_buf, newline_len))
The `DIFFERENT_EOL_STRINGS` check will run whenever b->repair is
FALSE. So, I check `DIFFERENT_EOL_STRINGS` first and if that check is
TRUE, I then check !b->repair, else check to see whether
*b->eol_translated needs to be set to TRUE:
<https://github.com/dtrebbien/subversion/commit/d22329a54dcf58cddc2b618f913597c6defbcb2d>.
What do you all think?
The upside is that unnecessary NULL checks of TRANSLATED_EOL are
dynamically elided under normal conditions.
The downsides are:
1.) Another assumption must be made (namely, that the EOL_STR string
is not varied between calls to translate_newline() using the same
translation baton).
2.) It complicates the logic.
3.) This penalizes repair translations.
> > p.s. To avoid confusion, you can call me Danny if you would like. I
> > like that name, and it comes in handy when working with other Daniels
> > :)
>
> And I'm danielsh, if it helps.
>
> (btw, the firstnames distribution in COMMITTERS is... interesting.)
You piqued my curiosity. Here is a frequency table of first names of
the active full committers:
5|Daniel
3|David
3|Stefan
2|Ben
2|Greg
2|Mark
1|Arfrever
1|Bert
1|Blair
1|Branko
1|Brian
1|C
1|Erik
1|Garrett
1|Hyrum
1|Ivan
1|Jani
1|Jeremy
1|Jim
1|Joe
1|John
1|Julian
1|Justin
1|Kamesh
1|Karl
1|Kouhei
1|Lieven
1|Max
1|Neels
1|Paul
1|Peter
1|Philip
1|Sander
1|Senthil
1|Stephen
1|Vlad
That's a lot of Daniels.
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Thu, Nov 25, 2010 at 3:21 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>
> (btw, the firstnames distribution in COMMITTERS is... interesting.)
It will probably be a long time before 'Hyrum' isn't an outlier. :)
-Hyrum
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Trebbien wrote on Wed, Nov 24, 2010 at 14:43:24 -0800:
> On Wed, Nov 24, 2010 at 3:43 AM, Gavin Beau Baumanis
> <ga...@thespidernet.com> wrote:
> > Hi Daniel (T),
> >
> > Since in your earlier post you mentioned that you didn't mind a friendly reminder...
> >
> > I thought I would return this thread to the top of the list - Just i case you had missed Daniel (Shahaf) 's comments.
> >
> >
> > Gavin "Beau" Baumanis
>
>
> Hi Gavin,
>
> I posted a new version of the patch on November 8
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123657)
> which addressed Daniel Shahaf's feedback from November 1. Daniel S.
> responded the next day with more feedback
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123719).
> I then suggested a macro solution
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123817)
> and Julian commented that a macro solution is not preferred
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123820).
>
> Julian is right that I can "feel" why not, so now I am trying to think
> of another approach to avoid the `if (translated_eol)` check from
> being executed unnecessarily. I feel stuck, though :(
>
I hope you aren't discouraged from working on the patch.
To the point, I originally asked if your changes affected the performance
of checkout/export. That is not a reason to stop the patch in its tracks;
it's a question that should be answered (either way) and the patch then
proceed. So, firstly, do your changes have any noticeable performance
effect, or is the effect of the added per-line condition simply not
noticeable?
If the latter, then I apologize (to Daniel) for your having spent time
writing patches (in various "creative" ways :)) that address what is
a non-problem.
As I don't recall (m)any other issues with the patch, I think it's
a short distance from resolving this issue to committing the patch.
>
> p.s. To avoid confusion, you can call me Danny if you would like. I
> like that name, and it comes in handy when working with other Daniels
> :)
And I'm danielsh, if it helps.
(btw, the firstnames distribution in COMMITTERS is... interesting.)
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Trebbien wrote on Wed, Nov 24, 2010 at 14:43:24 -0800:
> On Wed, Nov 24, 2010 at 3:43 AM, Gavin Beau Baumanis
> <ga...@thespidernet.com> wrote:
> > Hi Daniel (T),
> >
> > Since in your earlier post you mentioned that you didn't mind a friendly reminder...
> >
> > I thought I would return this thread to the top of the list - Just i case you had missed Daniel (Shahaf) 's comments.
> >
> >
> > Gavin "Beau" Baumanis
>
>
> Hi Gavin,
>
> I posted a new version of the patch on November 8
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123657)
> which addressed Daniel Shahaf's feedback from November 1. Daniel S.
> responded the next day with more feedback
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123719).
> I then suggested a macro solution
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123817)
> and Julian commented that a macro solution is not preferred
> (http://article.gmane.org/gmane.comp.version-control.subversion.devel/123820).
>
> Julian is right that I can "feel" why not, so now I am trying to think
> of another approach to avoid the `if (translated_eol)` check from
> being executed unnecessarily. I feel stuck, though :(
>
I hope you aren't discouraged from working on the patch.
To the point, I originally asked if your changes affected the performance
of checkout/export. That is not a reason to stop the patch in its tracks;
it's a question that should be answered (either way) and the patch then
proceed. So, firstly, do your changes have any noticeable performance
effect, or is the effect of the added per-line condition simply not
noticeable?
If the latter, then I apologize (to Daniel) for your having spent time
writing patches (in various "creative" ways :)) that address what is
a non-problem.
As I don't recall (m)any other issues with the patch, I think it's
a short distance from resolving this issue to committing the patch.
>
> p.s. To avoid confusion, you can call me Danny if you would like. I
> like that name, and it comes in handy when working with other Daniels
> :)
And I'm danielsh, if it helps.
(btw, the firstnames distribution in COMMITTERS is... interesting.)
Re: [PATCH] extend svn_subst_translate_string() to record whether
re-encoding and/or line ending translation were performed (v. 2)
Posted by Daniel Trebbien <dt...@gmail.com>.
On Wed, Nov 24, 2010 at 3:43 AM, Gavin Beau Baumanis
<ga...@thespidernet.com> wrote:
> Hi Daniel (T),
>
> Since in your earlier post you mentioned that you didn't mind a friendly reminder...
>
> I thought I would return this thread to the top of the list - Just i case you had missed Daniel (Shahaf) 's comments.
>
>
> Gavin "Beau" Baumanis
Hi Gavin,
I posted a new version of the patch on November 8
(http://article.gmane.org/gmane.comp.version-control.subversion.devel/123657)
which addressed Daniel Shahaf's feedback from November 1. Daniel S.
responded the next day with more feedback
(http://article.gmane.org/gmane.comp.version-control.subversion.devel/123719).
I then suggested a macro solution
(http://article.gmane.org/gmane.comp.version-control.subversion.devel/123817)
and Julian commented that a macro solution is not preferred
(http://article.gmane.org/gmane.comp.version-control.subversion.devel/123820).
Julian is right that I can "feel" why not, so now I am trying to think
of another approach to avoid the `if (translated_eol)` check from
being executed unnecessarily. I feel stuck, though :(
p.s. To avoid confusion, you can call me Danny if you would like. I
like that name, and it comes in handy when working with other Daniels
:)