[MERGE] Re: Accelerating push via bzr:// and friends
Andrew Bennetts
andrew at canonical.com
Wed May 21 03:55:43 BST 2008
Martin Pool wrote:
> On Tue, May 20, 2008 at 8:10 PM, Robert Collins
> <robertc at robertcollins.net> wrote:
[...]
> >
> > NODELAY stops the kernel buffering, so you get lots of packets on the
> > wire; otoh it means when we are being chatty we don't have to wait.
> > Perhaps we don't want NODELAY?
>
> SO_NODELAY does not afaik change any behaviour to do with waiting for
> acks, but only as Robert indicates changes transmission buffering. I
> don't think we would want it on in our case.
We originally turned it on because it made things faster. The test suite was
noticeably slower without it, back when we were developing the initial HPSS
code. IIRC, without it we'd write a request to a socket, but then find that the
kernel would hang on to it for some number of milliseconds before sending it,
even though we were just waiting for a response. Our first request is often
quite small (e.g. the initial BzrDir.open request, even with headers etc, is
still only 82 bytes long in v3), so it's understandable that the kernel might
think it's worth waiting for more. This situation is what the NODELAY flag is
for, AIUI.
We aren't ever as pathological as e.g. a user typing in a telnet session. We
sometimes wrote requests/responses in multiple calls because that was the most
convenient thing to do. In theory the network overhead of 3 TCP packets vs 1
packet should be trivial, so the there's no downside to us using NODELAY. So I
think we should keep setting NODELAY.
The problem I've seen with v3 when testing over a 500ms latency connection is
that the very first write to the socket is sent immediately, but then later
writes to send the rest of the request are delayed by 1000ms, because the kernel
waits to see the first ACK before sending any more packets. There's no flag I
can see to change this (and even if there were, I doubt it would be portable).
So I guess the only portable way to perform optimally here is to only call
send(2) once, which at least has the advantage of being a simple to explain
strategy. :)
> I think we are using Python file objects to send and receive, so
> rather than buffering ourself we should make sure their buffering is
> turned on, and then just flush them when we have finished sending the
> message.
We are actually using socket objects directly when talking over TCP.
Also, just flushing at the end of a message isn't sufficient if there's a body
stream. So we need to be flushing after each chunk of a body stream as well as
at the end of a message.
The attached patch implements appropriate buffering, and adds tests for that
buffering. In my testing this brings a push of one revision of bzr.dev over
500ms latency TCP down from 3m 19s to 1m 23s.
-Andrew.
-------------- next part --------------
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: andrew.bennetts at canonical.com-20080521024958-\
# 1b79jyuv3v9qhuq3
# target_branch: http://bazaar-vcs.org/bzr/bzr.dev
# testament_sha1: dae576195a0797da2a554bb47b7f8d226450c259
# timestamp: 2008-05-21 12:50:03 +1000
# source_branch: http://people.ubuntu.com/~andrew/bzr/hpss-buffering
# base_revision_id: pqm at pqm.ubuntu.com-20080520210027-wetfxldz1ggc5u2a
#
# Begin patch
=== modified file 'bzrlib/smart/protocol.py'
--- bzrlib/smart/protocol.py 2008-05-16 07:15:57 +0000
+++ bzrlib/smart/protocol.py 2008-05-21 02:49:58 +0000
@@ -1014,7 +1014,16 @@
response_marker = request_marker = MESSAGE_VERSION_THREE
def __init__(self, write_func):
- self._write_func = write_func
+ self._buf = ''
+ self._real_write_func = write_func
+
+ def _write_func(self, bytes):
+ self._buf += bytes
+
+ def flush(self):
+ if self._buf:
+ self._real_write_func(self._buf)
+ self._buf = ''
def _serialise_offsets(self, offsets):
"""Serialise a readv offset list."""
@@ -1046,6 +1055,7 @@
def _write_end(self):
self._write_func('e')
+ self.flush()
def _write_prefixed_body(self, bytes):
self._write_func('b')
@@ -1101,6 +1111,7 @@
elif response.body_stream is not None:
for chunk in response.body_stream:
self._write_prefixed_body(chunk)
+ self.flush()
self._write_end()
=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py 2008-05-19 13:55:43 +0000
+++ bzrlib/tests/test_smart_transport.py 2008-05-21 02:34:32 +0000
@@ -2566,6 +2566,34 @@
'e', # end
output.getvalue())
+ def test_call_writes_just_once(self):
+ """A bodyless request is written to the medium all at once."""
+ medium_request = StubMediumRequest()
+ encoder = protocol.ProtocolThreeRequester(medium_request)
+ encoder.call('arg1', 'arg2', 'arg3')
+ self.assertEqual(
+ ['accept_bytes', 'finished_writing'], medium_request.calls)
+
+ def test_call_with_body_bytes_writes_just_once(self):
+ """A request with body bytes is written to the medium all at once."""
+ medium_request = StubMediumRequest()
+ encoder = protocol.ProtocolThreeRequester(medium_request)
+ encoder.call_with_body_bytes(('arg', 'arg'), 'body bytes')
+ self.assertEqual(
+ ['accept_bytes', 'finished_writing'], medium_request.calls)
+
+
+class StubMediumRequest(object):
+ """A stub medium request that tracks the number of times accept_bytes is
+ called.
+ """
+ def __init__(self):
+ self.calls = []
+ def accept_bytes(self, bytes):
+ self.calls.append('accept_bytes')
+ def finished_writing(self):
+ self.calls.append('finished_writing')
+
class TestResponseEncodingProtocolThree(tests.TestCase):
@@ -2589,6 +2617,56 @@
'e')
+class TestResponseEncoderBufferingProtocolThree(tests.TestCase):
+ """Tests for buffering of responses.
+
+ We want to avoid doing many small writes when one would do, to avoid
+ unnecessary network overhead.
+ """
+
+ def setUp(self):
+ self.writes = []
+ self.responder = protocol.ProtocolThreeResponder(self.writes.append)
+
+ def assertWriteCount(self, expected_count):
+ self.assertEqual(
+ expected_count, len(self.writes),
+ "Too many writes: %r" % (self.writes,))
+
+ def test_send_error_writes_just_once(self):
+ """An error response is written to the medium all at once."""
+ self.responder.send_error(Exception('An exception string.'))
+ self.assertWriteCount(1)
+
+ def test_send_response_writes_just_once(self):
+ """A normal response with no body is written to the medium all at once.
+ """
+ response = _mod_request.SuccessfulSmartServerResponse(('arg', 'arg'))
+ self.responder.send_response(response)
+ self.assertWriteCount(1)
+
+ def test_send_response_with_body_writes_just_once(self):
+ """A normal response with a monolithic body is written to the medium
+ all at once.
+ """
+ response = _mod_request.SuccessfulSmartServerResponse(
+ ('arg', 'arg'), body='body bytes')
+ self.responder.send_response(response)
+ self.assertWriteCount(1)
+
+ def test_send_response_with_body_stream_writes_once_per_chunk(self):
+ """A normal response with a stream body is written to the medium
+ writes to the medium once per chunk.
+ """
+ # Construct a response with stream with 2 chunks in it.
+ response = _mod_request.SuccessfulSmartServerResponse(
+ ('arg', 'arg'), body_stream=['chunk1', 'chunk2'])
+ self.responder.send_response(response)
+ # We will write 3 times: exactly once for each chunk, plus a final
+ # write to end the response.
+ self.assertWriteCount(3)
+
+
class TestSmartClientUnicode(tests.TestCase):
"""_SmartClient tests for unicode arguments.
# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWXvEOekABsVfgGRUW+///3/3
//q////wYA0+vh49d73Jq3u0K1ZtGteTXo0BqVNNEVNga9NVHDKaU9SeAU9BGyhp6janqNNBppoy
HqABoabUAAklKfkBTxo1NNTSPUNB6mmgNPUBofqg0HqAGgeoJQmpgVPEamU9RtQepoAGgA0AaAAD
TQHMJoyNDQyGEaGQ00aADEZMgGEAwCJIhCaZTRtU2nopmp6j0aT0yJjSaepoNABoNAAkkCABBoAp
6GgaI0FPJoxQ08mk2jTIQyYphEDsAxVadTveujc1lBsNnbo27TkMvr9uduH1KzUly28EUkrQiFwN
uxJ2KqqIuRofCo2WwWl+uZ1YESKk12VoqLKLOB7WSgoEJ+PQh8WI6abT6+3s1CmQwojHec7tW7U6
18IomEWMBvYRyU8IVT7R0xHbM8k+eSmlue+jmSAAoEhGvVjsMq1CLyrr+ROjoViYYZAzL6/qI8R+
8SgYNm12l7jSz20ukPJH0rmdIxemxZrIldOumja6ZJ98RdRgn6TRxHZ5WvE3w4AY9gjlqq01NeLI
r6MytDWFRuqh2COeMNhv/3G8ZHN6l3RzGFw5V3WWpPzmM8Mj99AxL9uTiukWlZra4rhFF+nmp1sx
UmjgqLaotLngiDr5alRbnWaNVk+KuompTMChgYvAuz5quJmKCUyRMMljtxDKV8bCLDgJ3OuUiCqo
1pEsrsNAvgCzPPFetUCYTiV4pdVGzQU44GPKRa5qA+M6ughIlSWA30csfEchguBPTSpmfCe6MfYm
XRrKmteBfi0D1PgO/cnByGVb3PY3m9dGqJnHhdavSN0ZmDm2OorEJIalSMuFTcpWbY2tXTxp4KaU
yT+l3hm5eK5AeHjeHN3eq5ZUpSKI8GGVMTfpITMmIKVJo76PYCD4C+0WcMxgB3lXIYYOKEn0mlxq
KDecg4pLackUWmXWXNBlw7clBT6i2niqlVtEjF9CjHElyGAf4EVKbUQgUlwYxBg4I+pryaNbk5zv
+fADs3UvLZR1Y7J/nckjWMx3TEdooZZeT5MAwI85x0WJAgBnIhAneBQmsIIIjXkBrDCLbfdIUYLk
ESBUzEjYgql+jEidgZSYBTDomQvOcJinslhNISjIZzwur1avBZT+SWUoBipH5DPSTMAz7BwPYmgG
q0kTkVBVvmcglLkkqS3G46y1BogUx8iEC0AphExGU2o23wTVbD14F4wdlO/AfYJyGtMKzUBSWkGs
puIn9B+mJFJRopeUF8NmzMksYCQSJjMH8UQQ9JGXIoQEEBgH2kCoGMhi6SK3XTiN4YFpIheQHkg4
WH/R/+1vlW59nihZAytjwpfIuZ+9kltJ8hYkRwvCGk5hA0LaZaTu5YpIoR2MRNozgdjDINJah5jb
Cwz1TFZrWCSYOqETJmIlhOWcxlM2cqCycBh6JRiQtMD88121JcZfOzXRriGYLi95QmLZjlFCVJoq
z1BpK3Y4mgrHz6Cu0DISRQNAiTMY2kbrN0oKwvpexvFty1jlAgpGL80gpIyYC8TY4aCRUWmKsVLU
0lciZYYuG6YkzaIG1WBG187pKlp11OzhNykIzFTB9RIpW82yA2AFUDwSRzMTlw89vSkqC+V5KZ+m
x4Ft6AJs/oSUiAxJgsKRnEw+24JthshWUwtKwgt0KzAksMLL53lAYDPAfgScOeSJFtHZ01AebqA8
yS9iA5VbubDYfcZB8MHMwmWoNIrkDwzI1CI3B0HrC8ZC/MYPT/HUC+gwXesxRUD/WZsRhmIkE5wy
Gb2V+dd4/CDnIIEhgoKMQGRBJAwnCR2cR6L+sG7xU97DrwtYYBmYo/YPA8+k26Pdq3++VaJzA7jn
I+XA+oxtjOPC88xCsgsqNW3YW8ycRDsCnHC70pzi85TF4vwNAVnq8fn7z4fHAtPcq3hnrDMMDtPO
8X8v/wUimy8RwYZhr8IvgqLpgM1cMfAGjXDDe7Swwm0lPdqgMqX84jEi90NxiwBviDZGfz0XR05P
eDvcIhoayZZa21hkwUi3CPYe5cn4NMHBL/UBmDIzMdfIO1HQJiCanuQ4RUoIe+lSMwTphhRrmCpk
zCYZx0lAxSxF8q6KAmDloBazgPcMbDcVnx8J7R/lcTZP7alK9CaOiqPycZFArM4SENpxXMsEhULx
/taEopHukE6MQqP3P0PQ6POlFgjazWhRxfu0nI8T2wDgUum6Tf9XyOJQldJkc7ghlIHt2GvjlXQa
vSfM55zmOJ75DzrIAewlYnmv0n3Fp9KC8iaUhaMtlefcMHjGYboHJTjxwOrJBcOiRZmGD4uHa8El
9rO5DK0yjGFJ2TEcb5gHrDf551joFTtJakLEiRkWZj/JC+xdWZjX3Xo2KRLSCxGhnR4GdyjPIyZM
DcNGAxsJJbxLMklInLK1PNQ3F0B9jLpJQ+PEtvn2kjSUic4kgeG0nrEu2YiZUwnBVjuKzgajGu/q
PEkrCwZMGZ05xQ65AUkkJPEtAYaauT1BWO1AOYhqIBQyUUVOBwzLvnGabhwy15MBuzQOfzc7nkjx
F50lFN5h5sox2o6wo3GYZhhkOm/XxvUEaFMIqmW7Ya0o85d7dBRXVwPFaVhkA+SYVuc6nBXcJwyD
tnaRHz6NwdVg37YVnaA8ZxqPxazdf1GI7FiPmmSYvUKdtFhFBucOkpNZMCt8Z/RACQH5Kh1w7Xdo
XGF+kKXe3oDGLIGWCdkOETGJ95Slyxa2fYHGK5wB3n58RM5nTjHoIayIOM8LgQvviQ0Gk51l2O4N
Tx7TynZ5PJ3HYXCCJmRoR6f4fM8IdTOpneiG33nWHMnVxhqzCcgEAiZfm3WGkS52xB0x/RiUo8TF
wOGL9UAhASd3jCLskA64OiMfmdqJxf2A5zBwbEKjP53nk5r/PqmSFuMbxyGkFUGs4kaMDPWgxmFV
x8Cf8BEYKDV30p6ZheWcMPb+R2FRYacpUMkD0qcA8qQPd12iFmSMXCJifIgGUIJL9PNkREoA4SYq
TMWhd6voPDoE4DLN8OtIwjNrSjB4TWh42yYGDMTGPgFaCMMvw9KELv6q6xUMCqCgpE+fl6Q8CIU8
Q1fjUbBgqYa1JLuD4PcHQRXvYUJlpyDHameiMURA7jkOHmJQdpB4M3WXHMoTHmRLT2nc8nNhXFaD
JA2KxeaAX3E6TtJdHdMx7ISLkJALjoMFGnviufEDxkR5cFGxYdC50XHDMo0/Ure6pdlFYLSGMRoO
oyuBE4cCtqGA8LerHzm6QlsXJLhyQrwv6+ktmEr2QBgqYnGkSeAa8pio6bqnXz6dWs2UB7hF4joe
UpkeQpCIt4TziXakv2psRlMCxB6GMCkgHI5HJBDwJImDqLcip0zgeieEgAl0Lp/APZYoEAYoTiQY
0HCk7kQeqkCOWnugYhcBxJLpdKB3YHBJDZX+yzyhcoHfgX0wTwlPYOJoNiUZgCGxi7E8zZLPO1TO
4dYU6iDY50XOprMoBpgcr1pAXmV7KmGPABzMQ9qZ0xNBGmmJGBUDLxU2LwK6JgdFd84Kv10c2FrB
kMXKSBclYwrIgWCEUdlDUwG+HwAZRDcloHAr3PWQruVYiFQP+iF3m1btDJl23AVUuVwWjiKN5Mi8
JTEB4tQyFhJcpvoZay/gVtsRdbrK4SDQEcSAHlVgYoKiGU2LwhDOIKSFrEZ6ZHZyGghIZWZGCSF6
Kudmkp9nEFlfSajOFsWR0xM0N5LmhiHkGN8KGNggmk2DnwKPEYQhAJCIN6MBaNbONjCvC8J0h6Q2
S4HieGBYF0EjEWwO8Om+xul52CFBSjopDQBuItBH2LIQkEwcT7wsSVB8yr2qYI7ArXKo0zIDxpfG
KTa54G4gbtJ0j+WC5Bv3A1m25y0HD+KawKzuNi4gfiA+ArcyboNqQXm4chLRVuQyDeFxmeh4be17
kzxDC1e4ynlrPVyvmDZEE7gs4y6bx+3SbUDyUT6WPIDB95gFiS9WgCIfd8rdq6QxMSXAzhD/i7ki
nChIPeIc9IA=
More information about the bazaar
mailing list