[MERGE] sftp readv performance tweaks (2x faster)

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> On 17 Jul 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:
> 
>> Well, I ran scripts all weekend to figure out how fast I could make sftp
>> with my changes. And the attached image shows how it went.
> 
> Very nice.  Given the improvement it would be good for 0.9 - do you have
> any reservations?

No specific reservations. I'm pretty happy with it overall. And we might
be able to tweak a little performance improvement for local access as well.

The only thing I really have left to test is whether we need to work
around how paramiko does prefetch and/or readv. If paramiko directly
implements readv() we might end up implementing a different
SFTPTransport.readv(). I don't know how it is implemented though, and
these changes should also benefit local requests and ftp requests.

> 
>> +    @staticmethod
>> +    def _coalesce_offsets(offsets, limit, fudge_factor):
>> +        """Yield coalesced offsets.
>> +
>> +        With a long list of neighboring requests, combine them
>> +        into a single large request, while retaining the original
>> +        offsets.
>> +        Turns  [(15, 10), (25, 10)] => [(15, 20, [(0, 10), (10, 10)])]
>> +
>> +        :param offsets: A list of (start, length) pairs
>> +        :param limit: Only combine a maximum of this many pairs
>> +                      Some transports penalize multiple reads more than
>> +                      others, and sometimes it is better to return early.
>> +                      0 means no limit
>> +        :param fudge_factor: All transports have some level of 'it is
>> +                better to read some more data and throw it away rather 
>> +                than seek', so collapse if we are 'close enough'
>> +        :return: a generator of [(start, total_length, 
>> +                                  [(inner_offset, length)])]
>> +                (start, total_length) is where to start, and how much to read
>> +                and (inner_offset, length) is how those chunks should be
>> +                split up.
> 
> These lists are getting to a size/complexity where they might be better
> as trivial objects rather than a list - with say .start, .total_length,
> .ranges.  Unless there's a compelling peformance reason it's better not
> to store objects with multiple fields inside a list or tuple.  It might
> make the tests a bit more reasonable too.
> 
> This couldn't be a generator then, but does it get any value from being
> so?  Since you sort the input it can't really stream.

Well, it actually could still be a generator. Just returning objects
that look like:

class Chunk(object):
  __slots__ = ['start', 'length', 'inner_offsets']
  def __init__(self):
    self.start = None
    self.length = None
    self.inner_offsets = []

etc.

Actually, I thought I mentioned this, but I do stream. I sort the
requested input to make the actual read requests, but then I yield
everything I can as I go along. And if I yield it, I pop() the data
buffer. So I only buffer whatever was requested out of order.

In my testing, we usually read *almost* in order. Which means my buffer
stays small, and we stream fairly well. But that little bit out of order
does mess up performance. (Doing larger combination, and doing fewer
reads if the hunks are close also helps)

> 
> Just a thought, if you think it's better this way you can leave it.

I agree that it is cleaner that way. I wasn't real happy with the
complexity of the tuples.


>> === modified file 'bzrlib/transport/sftp.py'
>> --- bzrlib/transport/sftp.py	2006-06-30 15:13:19 +0000
>> +++ bzrlib/transport/sftp.py	2006-07-17 18:14:33 +0000
>> @@ -426,25 +434,6 @@
>>          except (IOError, paramiko.SSHException), e:
>>              self._translate_io_exception(e, path, ': error retrieving')
>>  
>> -    def get_partial(self, relpath, start, length=None):
>> -        """
>> -        Get just part of a file.
>> -
>> -        :param relpath: Path to the file, relative to base
>> -        :param start: The starting position to read from
>> -        :param length: The length to read. A length of None indicates
>> -                       read to the end of the file.
>> -        :return: A file-like object containing at least the specified bytes.
>> -                 Some implementations may return objects which can be read
>> -                 past this length, but this is not guaranteed.
>> -        """
>> -        # TODO: implement get_partial_multi to help with knit support
>> -        f = self.get(relpath)
>> -        f.seek(start)
>> -        if self._do_prefetch and hasattr(f, 'prefetch'):
>> -            f.prefetch()
>> -        return f
>> -
> 
> Was this just no longer used?
> 

'get_partial' is no longer part of the Transport api. It was supplanted
by readv(). I think when get_partial was removed, nobody looked around
to see that SFTP had also implemented it.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEwBjqJdeBCYSNAAMRAsj1AKCozetK2AMxOiNAixzsDULWCVWd7QCdGczX
cthBrYX05XJwVrVRe/tt3sA=
=w0Wa
-----END PGP SIGNATURE-----