[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r995478 [1/2] - in /subversion/branches/performance: ./ subversion/bindings/javahl/native/ subversion/bindings/javahl/src/org/apache/subversion/javahl/ subversion/bindings/javahl/src/org/tigris/subversion/javahl/ subversion/bindings/j

From: Konstantin Kolinko <knst.kolinko_at_gmail.com>
Date: Sat, 11 Sep 2010 09:13:09 +0400

2010/9/10 Hyrum Wright <hwright_at_apache.org>:
> On Thu, Sep 9, 2010 at 4:34 PM, Stefan Fuhrmann
> <stefanfuhrmann_at_alice-dsl.de> wrote:
>> Hyrum Wright wrote:
>>>>  *identical_p = TRUE;  /* assume TRUE, until disproved below */
>>>> -  do
>>>> +  while (! (done1 || done2))
>>>>    {
>>>> -      SVN_ERR(svn_io_file_read_full2(file1_h, buf1,
>>>> -                                     SVN__STREAM_CHUNK_SIZE,
>>>> &bytes_read1,
>>>> -                                     TRUE, pool));
>>>> -      SVN_ERR(svn_io_file_read_full2(file2_h, buf2,
>>>> -                                     SVN__STREAM_CHUNK_SIZE,
>>>> &bytes_read2,
>>>> -                                     TRUE, pool));
>>>> +      err = svn_io_file_read_full(file1_h, buf1,
>>>> +                                  SVN__STREAM_CHUNK_SIZE, &bytes_read1,
>>>> pool);
>>>>
>>>
>>> Stefan,
>>> This call, and the one following, needed to revert back to
>>> svn_io_file_read_full(), since read_full2() was being stuck in an
>>> infinite loop.  Perhaps you can investigate?
>>>
>>
>> Hm. That doesn't make any sense. As soon as it tries to read beyond EOF,
>> the while (bytes_read1==SVN__STREAM_CHUNK_SIZE) condition
>> should be triggered. Looking through the respective APR code for Windows
>> and Unix, they always seem to set the number of bytes read even if errors
>> were encountered.
>>
>> How did you trigger the infinite loop?
>
> Change those two function calls, and then run 'make check'.  It
> stalled infinitely[1], with both cores pegged.  Using gdb to attach to
> the running process showed that it was svn_io_file_read_full2(),
> invoked from that location.
>
> -Hyrum
>
> [1] Well, I've no way of knowing if it was infinite or just taking a
> long time, but I'll defer a solution to the Halting Problem as an
> exercise for the reader. :)
>

Hyrum,
you cannot just replace svn_io_file_read_full() with
svn_io_file_read_full2() in io.c_at_995478 without adding additional code
there.

svn_io_file_read_full2() swallows end-of-file, thus done1 and done2 in
that loop will never become TRUE.

I think that it could be written like the following:
[[[
    err = svn_io_file_read_full2(file1_h, buf1,
        SVN__STREAM_CHUNK_SIZE, &bytes_read1, TRUE, pool);
    if (err) {
       break;
    } else if (bytes_read1 != SVN__STREAM_CHUNK_SIZE) {
          done1 = TRUE;
    }
]]]
and the same for the second file.

My understanding is that
- you tried to backport rev.993308 from trunk
- the advantage of svn_io_file_read_full2() is that it does not waste
time creating detailed svn_error_t object when EOF is met.

I think that svn_io_file_read_full2() could be rewritten to accept
pointer to a boolean,
like the following: (warning: I did not try to compile or run this)

[[[
* /subversion/libsvn_subr/io.c
  (svn_io_file_read_full2): Assume that eof_is_ok argument is always TRUE
     and use a pointer to a boolean variable to pass back information about
     detected EOF.
  (contents_identical_p): use *_read_full2() to minimize EOF detection
     overhead, like r985488 did but was reverted in r995478.

Index: io.c
===================================================================
--- io.c (revision 996063)
+++ io.c (working copy)
@@ -2875,12 +2875,14 @@
 svn_error_t *
 svn_io_file_read_full2(apr_file_t *file, void *buf,
                         apr_size_t nbytes, apr_size_t *bytes_read,
- svn_boolean_t eof_is_ok,
+ svn_boolean_t* eof_flag,
                         apr_pool_t *pool)
 {
   apr_status_t status = apr_file_read_full(file, buf, nbytes, bytes_read);
- if (APR_STATUS_IS_EOF(status) && eof_is_ok)
+ if (APR_STATUS_IS_EOF(status)) {
+ *eof_flag = TRUE;
     return SVN_NO_ERROR;
+ }

   return do_io_file_wrapper_cleanup
     (file, status,
@@ -3607,26 +3609,14 @@
   *identical_p = TRUE; /* assume TRUE, until disproved below */
   while (! (done1 || done2))
     {
- err = svn_io_file_read_full(file1_h, buf1,
- SVN__STREAM_CHUNK_SIZE, &bytes_read1, pool);
- if (err && APR_STATUS_IS_EOF(err->apr_err))
- {
- svn_error_clear(err);
- err = NULL;
- done1 = TRUE;
- }
- else if (err)
+ err = svn_io_file_read_full2(file1_h, buf1,
+ SVN__STREAM_CHUNK_SIZE,
&bytes_read1, &done1, pool);
+ if (err)
         break;

- err = svn_io_file_read_full(file2_h, buf2,
- SVN__STREAM_CHUNK_SIZE, &bytes_read2, pool);
- if (err && APR_STATUS_IS_EOF(err->apr_err))
- {
- svn_error_clear(err);
- err = NULL;
- done2 = TRUE;
- }
- else if (err)
+ err = svn_io_file_read_full2(file2_h, buf2,
+ SVN__STREAM_CHUNK_SIZE,
&bytes_read2, &done2, pool);
+ if (err)
         break;

       if ((bytes_read1 != bytes_read2)
]]]

Best regards,
Konstantin Kolinko

Received on 2010-09-11 07:14:08 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.