Make deadlines interrupt and coalesce writes #28

Stebalien · 2019-05-21T02:16:43Z

(sorry for the monster PR, these changes turned into a bit of a rabbit hole)

First up, this patch correctly implements deadlines:

Deadlines now apply to in-progress operations, allowing us to cancel writes/reads with deadlines. I need this to correctly implement Close() in the circuit transport.
The ConnectionWriteTimeout now actually does what it says it does: When we fail to write fast enough, it kills the entire connection.
All writes are now async.
a. We can't abort after a partial write anyways but we shouldn't block the stream write until we finish writing to the underlying connection. The simple solution is to claim that the write succeeds when we enqueue it. This is fine as network writes are buffered anyways.
b. The network is buffered so it doesn't matter.
c. We need this to coalesce/buffer writes to reduce packet count/secio overhead.

Next, this PR coalesces writes from any number of streams.

Finally, this PR breaks large writes into 16KiB chunks to:

Ensure fairness between streams.
Ensure that one large write doesn't block the send loop and make us think we've timed out.

Breaking:

Probably changes a few errors.
Requires net.Conn (for deadlines). Shouldn't break our code as our stream muxer interfaces use net.Conn anyways.
Anything expecting writes to be synchronous will break. That is, nothing that actually uses the network.

Tradeoffs:

This patch splits up writes into small (64KiB) buffers and copies them into temporary buffers (using a buffer pool). We have to do this as we now write async.

This impacts the built-in benchmarks quite a bit but, IMO, those are rather synthetic and don't reflect actual network conditions.

Coalesce Delays:

I've played with introducing an additional delay to try to coalesce writes a bit more. To test this, I tried connecting two IPFS nodes and counted the number of packets sent by both nodes. My results are:

103 - base (current go-ipfs)
64 - no delay (this patch)
46 - 1us delay
45 - 10us delay
36 - 1ms delay

Given this, I feel like we can mess with delays later if necessary but this is a good start (and the patch is already way too large).

These are really small so there's no point in using slices. These slices are currently amortized by reusing them but we can't _always_ do that.

Write always writes everything

Creating a new byte reader is a waste of an allocation.

1. Deadlines now apply to in-progress operations. 2. The ConnectionWriteTimeout now actually does what it says it does: When we fail to write fast enough, it kills the entire connection. 3. Make all writes async. a. We can't abort after a partial write anyways. b. The network is buffered so it doesn't matter. c. This will help us coalesce writes.

Stebalien · 2019-05-21T02:17:17Z

deadline.go

+//
+// Copyright 2010 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE-BSD file.


Stolen from the standard library, no need to review much.

Stebalien · 2019-05-21T02:17:39Z

mux.go

@@ -69,7 +70,7 @@ func VerifyConfig(config *Config) error {
 // Server is used to initialize a new server-side connection.
 // There must be at most one server-side connection. If a nil config is
 // provided, the DefaultConfiguration will be used.
-func Server(conn io.ReadWriteCloser, config *Config) (*Session, error) {
+func Server(conn net.Conn, config *Config) (*Session, error) {


A breaking change for yamux but libp2p doesn't care.

Stebalien · 2019-05-21T02:19:38Z

session.go

@@ -169,7 +161,7 @@ func (s *Session) Open() (net.Conn, error) {
 // OpenStream is used to create a new stream
 func (s *Session) OpenStream() (*Stream, error) {
 	if s.IsClosed() {
-		return nil, ErrSessionShutdown
+		return nil, s.shutdownErr


This exposes the connection timeout error. I'm happy to revert this but a bunch of tests were complaining that Write was returning ErrSessionShutdown instead of ErrConnectionWriteTimeout so I just fixed this everywhere.

Stebalien · 2019-05-21T02:20:01Z

session.go

@@ -275,14 +267,13 @@ func (s *Session) exitErr(err error) {
 // GoAway can be used to prevent accepting further
 // connections. It does not close the underlying conn.
 func (s *Session) GoAway() error {
-	return s.waitForSend(s.goAway(goAwayNormal), nil)
+	return s.sendMsg(s.goAway(goAwayNormal), nil, nil)


There is now only one way to send things (all sends are async, like the network, dammit!).

Stebalien · 2019-05-21T02:29:15Z

session.go

+	// duplicate as we're sending this async.
+	buf := pool.Get(headerSize + len(body))
+	copy(buf[:headerSize], hdr[:])
+	copy(buf[headerSize:], body)


This is the most annoying part of this patch for me. However:

To implement per-stream deadlines correctly, we need to copy the data before writing. Otherwise, we can't return while the write is still in-progress (we don't want to just kill the entire connection).

The buffer pool is actually pretty fast.

yeah, it's ok.

Stebalien · 2019-05-21T02:34:07Z

session_test.go

-		writer:       write1,
-		writeBlocker: make(chan struct{}, 1),
-		closeCh:      make(chan struct{}, 1),
+func testConn() (conn1, conn2 net.Conn) {


These tests made a lot of assumptions about the synchronous nature of this connection. I've had to rewrork most of them to handle the fact that all writes are now async.

Stebalien · 2019-05-21T02:34:42Z

stream.go

@@ -360,6 +319,16 @@ func (s *Stream) forceClose() {
 	}
 	s.stateLock.Unlock()
 	s.notifyWaiting()
+
+	s.readDeadline.set(time.Time{})
+	s.readDeadline.set(time.Time{})


Avoid leaking timers.

Stebalien · 2019-05-21T02:34:49Z

stream.go

+func (s *Stream) cleanup() {
+	s.session.closeStream(s.id)
+	s.readDeadline.set(time.Time{})
+	s.readDeadline.set(time.Time{})


Also avoid leaking timers.

Stebalien · 2019-05-21T02:35:00Z

stream.go

+	defer s.stateLock.Unlock()
+	switch s.state {
+	case streamClosed, streamRemoteClose, streamReset:
+		return nil


Really, don't leak timers.

* Ensures one stream doesn't completely starve the others when writing a large blob. * Ensures we don't think we're stalling on write when we're actually writing a ton of data. * Ensures we don't queue more than 1MiB in the write channel.

1. Bump to 64KiB. That's large enough to not be a performance issue but still small enough that not writing 64KiB within the connection write timeout means something is very wrong. 2. Remove the header size from this max message size so we get a nice round number (see the next patch).

1. This allows us to combine the header and body before writing and helps us avoid really tiny packets. 2. We _need_ to copy as we're now sending entirely async. The buffer pool should pretty much erase the overhead of this copy.

Stebalien · 2019-05-21T02:51:04Z

cc @vyzo.

vyzo · 2019-05-21T08:47:55Z

It seems that a little delay does help, I would say 1ms is not a big deal and goes a long way towards the goal of reducing packet counts.
We can couple this with libp2p/go-mplex#54 and ship write coalescing already!

Stebalien · 2019-05-21T16:16:12Z

It seems that a little delay does help, I would say 1ms is not a big deal and goes a long way towards the goal of reducing packet counts.

1ms is a pretty big deal on a local network. I'd prefer to ship something simpler first and then play with configurable delays so we have a better base to test against.

This significantly impacts local benchmarks but those are pretty synthetic anyways. This shouldn't make any difference on an actual network.

vyzo

LGTM overall, a couple of nits.

vyzo · 2019-05-21T16:46:52Z

go.mod

@@ -0,0 +1,5 @@
+module github.com/whyrusleeping/yamux
+
+go 1.12


that was probably auto-added when you built, but I don't think we mandate it yet.

It doesn't really make a difference. It just means go will warn the user if the compile fails.

The go directive in a go.mod file now indicates the version of the language used by the files within that module. It will be set to the current release (go 1.12) if no existing version is present. If the go directive for a module specifies a version newer than the toolchain in use, the go command will attempt to build the packages regardless, and will note the mismatch only if that build fails.

vyzo · 2019-05-21T16:48:53Z

session.go

+	// duplicate as we're sending this async.
+	buf := pool.Get(headerSize + len(body))
+	copy(buf[:headerSize], hdr[:])
+	copy(buf[headerSize:], body)


yeah, it's ok.

vyzo · 2019-05-21T16:50:37Z

session.go

 				}
+
+				writer = getBuffer(s.conn)
+				writeTimeout.Reset(100 * time.Microsecond)


shouldn't we Stop the timer and possibly drain the channel before resetting?

The timer has to have fired for us to get to this point.

Stebalien added 6 commits May 19, 2019 23:26

implement net.Error for yamux errors

51b721f

return shutdown error in more places

2f357ff

send headers by value

b8cd2b4

These are really small so there's no point in using slices. These slices are currently amortized by reusing them but we can't _always_ do that.

simplify sendLoop

7009fac

Write always writes everything

use raw byte buffers for writes

b371c3a

Creating a new byte reader is a waste of an allocation.

Stebalien commented May 21, 2019

View reviewed changes

Stebalien added 7 commits May 20, 2019 19:45

coalesce writes

4aa320f

set a maximum message size

7bf61d6

* Ensures one stream doesn't completely starve the others when writing a large blob. * Ensures we don't think we're stalling on write when we're actually writing a ton of data. * Ensures we don't queue more than 1MiB in the write channel.

fix benchmarks

b87f504

copy writes into a temporary buffer on send

901fb4b

1. This allows us to combine the header and body before writing and helps us avoid really tiny packets. 2. We _need_ to copy as we're now sending entirely async. The buffer pool should pretty much erase the overhead of this copy.

chore: remove dead code

355555e

doc: add a few comments

118deb1

Stebalien force-pushed the feat/deadline branch from 2b461b2 to 118deb1 Compare May 21, 2019 02:45

fix: catch timeouts on flush

9c050b1

Stebalien mentioned this pull request May 21, 2019

call Stream.Reset instead of Stream.Close libp2p/go-libp2p-circuit#76

Merged

vyzo mentioned this pull request May 21, 2019

coalesce writes libp2p/go-mplex#54

Merged

Stebalien added 2 commits May 21, 2019 09:44

introduce a 100us delay when coalescing writes

7c36f6a

This significantly impacts local benchmarks but those are pretty synthetic anyways. This shouldn't make any difference on an actual network.

dep: add go module support

66954cb

vyzo approved these changes May 21, 2019

View reviewed changes

make MaxMessageSize and WriteCoalesceDelay configurable.

5936fdb

Stebalien merged commit f2ffcc7 into master May 21, 2019

Stebalien deleted the feat/deadline branch May 21, 2019 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make deadlines interrupt and coalesce writes #28

Make deadlines interrupt and coalesce writes #28

Stebalien commented May 21, 2019

Stebalien May 21, 2019

Stebalien May 21, 2019

Stebalien May 21, 2019

Stebalien May 21, 2019

Stebalien May 21, 2019

vyzo May 21, 2019

Stebalien May 21, 2019

Stebalien May 21, 2019

Stebalien May 21, 2019

Stebalien May 21, 2019

Stebalien commented May 21, 2019

vyzo commented May 21, 2019

Stebalien commented May 21, 2019

vyzo left a comment

vyzo May 21, 2019

Stebalien May 21, 2019

vyzo May 21, 2019

vyzo May 21, 2019

Stebalien May 21, 2019

		@@ -0,0 +1,5 @@
		module github.com/whyrusleeping/yamux

		go 1.12

Make deadlines interrupt and coalesce writes #28

Make deadlines interrupt and coalesce writes #28

Conversation

Stebalien commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented May 21, 2019

vyzo commented May 21, 2019

Stebalien commented May 21, 2019

vyzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment