[Merge] Slow socket

[Carl Friedrich Bolz]

I updated the slow socket patch to fix the problems that John and Robert
pointed out, see attachement.

John Arbash Meinel wrote:
> Out of curiosity, what bzr did you use to generate the diff? We haven't
> used '/dev/null' for a newly added file in a while.

I used the one in dapper, whichever that is. From now on I am properly
dogfooding on bzr.dev, though :-).

[snip]
>> +        try:
>> +            tree.commit('initial commit')
>> +            for i in range(num_commits):
>> +                fn = files[i % len(files)]
>> +                f = open(fn, "w")
>> +                f.write("\n".join([str(j) for j in (range(i) + [i,
i, i])]))
>> +                f.close()
>> +                tree.commit("changing file %s" % fn)
>
> ^- We almost always use "wb" to ensure the contents of the file are
> consistent across platforms.
> I also don't understand what you are accomplishing with (range(i) +
> [i,i,i]). I suppose you just want to always create a file with 'i+3'
lines.

The intention was to have at least _some_ lines which get removed
when the file is changed again, and not just more and more added lines.

> Also, what you have written will create a file with no trailing newline.
> What you probably want is:
>
> f.write('\n'.join([str(j) for j in (range(i) + [i,i,i,''])])
>
>> +        finally:
>> +            try:
>> +                try:
>> +                    tree.branch.repository.unlock()
>> +                finally:
>> +                    tree.branch.unlock()
>> +            finally:
>> +                tree.unlock()
>> +        return tree, files
>
> Why is this part of SFTPBenchmark, instead of Benchmark or some helper
> functions?

Moved this. I was not so sure whether it is of general use, I guess in
the end more and more benchmarks will use the same convenience functions
to create trees with certain properties.

In addition I think the tree creation function should be updated to use
John's new caching mechanism. Should I update the patch again or do it
with an extra merge? These particular trees are not very big, so maybe
it is ok not cache.

> Did we decide that simulated time was better than using real time? (For
> one thing, async improvements would not show up properly here).

Right. Although the benchmarks are now even slower, of course.

[snip]

>> +
>> +    def test_bandwidth(self):
>> +        from bzrlib.transport.sftp import SocketDelay
>> +        sending = FakeSocket()
>> +        receiving = SocketDelay(sending, 0, bandwidth=8.0/(1024*1024))
>> +        # check that simulated time is charged only per round-trip:
>> +        t1 = SocketDelay.simulated_time
>> +        receiving.send("connect")
>> +        self.assertEqual(sending.recv(1024), "connect")
>> +        sending.send("a" * 100)
>> +        self.assertEqual(receiving.recv(1024), "a" * 100)
>> +        t2 = SocketDelay.simulated_time
>> +        self.assertAlmostEqual(t2 - t1, 100 + 7)
>> +
>> +
>
> So is this setting the bandwidth to 8 millibytes/sec?
>

I hope it sets the bandwidth to 1 byte/sec, but I might have
mis-calculated :-). The bandwidth parameter accepts Mbit, which is
commonly used for internet access and such.

>
>> === modified file 'bzrlib/transport/sftp.py'
>> --- bzrlib/transport/sftp.py	
>> +++ bzrlib/transport/sftp.py	
>> @@ -1065,6 +1065,99 @@
>>                          x)
>>
>>
>> +class SocketDelay(object):
>> +    """A socket decorator to make TCP appear slower.
>> +
>> +    This changes recv, send, and sendall to add a fixed latency to
each python
>> +    call if a new roundtrip is detected. That is, when a recv is
called and the
>> +    flag new_roundtrip is set, latency is charged. Every send and
send_all
>> +    sets this flag.
>> +
>> +    In addition every send, sendall and recv sleeps a bit per
character send to
>> +    simulate bandwidth.
>> +
>> +    The function used to sleep moves a counter forwards to not make
the tests
>> +    slower. It could be made more clever, by adding the time that
was passing
>> +    between sleep calls to the simulated time too.
>
> ^- the english here isn't very clear.
>
>> +
>> +    Not all methods are implemented, this is deliberate as this
class is not a
>> +    replacement for the builtin sockets layer. fileno is not
implemented to
>> +    prevent the proxy being bypassed.
>> +    """
>> +
>> +    simulated_time = 0
>
>
> v- You could probably do this easier with a __getattr__() that thunked
> to self.sock. And explicitly blocked fileno. (My understanding is that
> __getattr__() is only called when the attribute in question fails to be
> found by the default object implementation)
>

A __getattr__() is indeed only called when the normal way to lookup a an
attribute fails. I decided to use a __getattr__ but to route only
certain attributes through to the underlying socket (see patch). I think
it is a compromise between having repeated code and the risk of routing
too much.

>> +
>> +    def __init__(self, sock, latency, bandwidth=1.0,
>> +                 really_sleep=False):
>> +        """
>> +        :param bandwith: simulated bandwith (MegaBit)
>> +        """
>> +        self.sock = sock
>> +        self.latency = latency
>> +        self.really_sleep = really_sleep
>> +        self.time_per_byte = 1 / (bandwidth / 8.0 * 1024 * 1024)
>> +        self.new_roundtrip = False
>> +
>> +    def sleep(self, s):
>> +        if self.really_sleep:
>> +            time.sleep(s)
>> +        SocketDelay.simulated_time += s
>
> Shouldn't this be:
>
> if self.really_sleep:
>   time.sleep(s)
> else:
>   SocketDelay.simulated_time += s
>

Fixed, thanks.

Cheers,

Carl Friedrich
-------------- next part --------------
A non-text attachment was scrubbed...
Name: slowsocket.patch
Type: text/x-patch
Size: 15495 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060816/788226a0/attachment.bin