coalesce writes #54

vyzo · 2019-05-11T12:46:11Z

This is an experiment in adding a write coalescer to mplex. Although I do think we need a generic one that sites between the security module and the multiplexer, perhaps instantiated at user's choice.

vyzo · 2019-05-11T12:48:18Z

test failures are #47

marten-seemann · 2019-05-11T12:52:37Z

I’m not sure if this belongs in a stream multiplexer. The stream multiplexer’s responsibility is to, well, multiplex streams.
Having a 100ms delay for sending a small message doesn’t seem like something every application protocol would want. That’s why I’d argue that this belongs in a layer above the stream multiplexer.

vyzo · 2019-05-11T12:56:35Z

Maybe so, but we do have a problem with doing small writes all over the place.

vyzo · 2019-05-11T13:10:59Z

An alternative is to have a generic coalescer (not a protocol specific one, like here), and put it under the multiplexer, perhaps at a user's preference.

marten-seemann · 2019-05-11T13:12:37Z

Then let’s fix it there. Small writes are not an mplex-specific problem, we have the same problem when using QUIC.

…

On Sat, May 11, 2019 at 21:56 vyzo ***@***.***> wrote: Maybe so, but we do have a problem with doing small writes all over the place. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#54 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALI6V3K5XZ35BHZNOLNN33PU27AJANCNFSM4HMIBMUQ> .

raulk

This is an interesting finding and experiment, but we really don’t want this at this layer.

I agree that this doesn’t belong in a multiplexer. Even if we pushed it down the stack, it would belong in the transport.

But let’s not add a delay to everything. There’s a reason why people disable Nagle in most backend processes.

The application protocol is the one that knows its write pattern, and whether its use case is latency-sensitive or not. Ideally we should expose metadata about the transport at play: is fragmentation costly or not? In UDP the answer might be no, in TCP it would be yes, all of this omitting the cost of the syscall.

Providing a toolbox utility that protocols can opt in, like Marten suggests, is a reasonable tradeoff.

I’m also worried we’re fine-tuning a lot for the specific usecase of relay infrastructure.

vyzo · 2019-05-11T16:07:52Z

@raulk I wasn't really intending to merge this in mplex, it was an experiment to open the conversation.
I would like this to be a generic option, which the user can configure, and would be turned on in relays.

vyzo · 2019-05-11T16:09:02Z

Marked as DO NOT MERGE.

vyzo · 2019-05-11T16:23:49Z

Here is a proposal: We can add the coaslescer under the multiplexer and also extend our NewStream interface to provide options. One such option would be for a real-time stream, which instructs the coalescer to send immediately (together with what data it might have accumulated).
We are going to have to extend our stream interface in order to handle the closure/reset mess, so we might as well take the opportunity to do this there.

vyzo · 2019-05-11T17:09:09Z

Continuation of discussion in libp2p/go-libp2p#633

Will close it now, but do not delete the branch as it is in use in relays.

vyzo · 2019-05-13T08:46:50Z

reopened this so that we avoid deleting the branch by accident.

vyzo · 2019-05-21T09:07:50Z

@raulk would you be comfortable merging with a configurable coalesce delay, that defaults to 1ms?
The relays can set it to 100ms.

Note that we are adding write coalescing to yamux as well in whyrusleeping/yamux#28

marten-seemann · 2019-05-21T09:16:27Z

Where is the main source of small writes? I’d still prefer to fix it there.
Introducing artificial delays might break applications, most obviously the ping protocol, but also other real time protocols.

vyzo · 2019-05-21T09:17:26Z

1ms delay will hardly break anything.
The writes are coming from multiple streams, so they need to be combined in the multiplexer or under it.

vyzo · 2019-05-21T09:18:14Z

In short, we can't fix it above the multiplexer, it either has to be in the multiplexer or below it.

Stebalien

I think we can simplify this a bit but it should work. The only thing we really need to change is the default delay.

Stebalien · 2019-05-21T16:55:00Z

multiplex.go

@@ -37,6 +37,8 @@ var ErrInvalidState = errors.New("received an unexpected message from the peer")
 var (
 	NewStreamTimeout   = time.Minute
 	ResetStreamTimeout = 2 * time.Minute
+
+	WriteCoalesceDelay = 1 * time.Millisecond


Let's default to 100us for now.

ok, is that the default in yamux too?

ah it's 10us in yamux.

Stebalien · 2019-05-21T18:48:50Z

multiplex.go

+	n := copy(buf, data)
+	pool.Put(data)
+
+	if !mp.writeTimerFired {


I'd just combine this with handleOutgoing and avoid storing the timer on the instance (makes it clear that it's local only).

Then we'd have to explicitly pass it to writeMsg and mutate in there, which makes it a little messy.
It seemed nicer to just have the timer in the instance.

My suggestion was to ditch writeMsg and combine everything. But we can do that later.

Stebalien · 2019-05-21T18:52:38Z

multiplex.go

+	defer pool.Put(buf)
+
+	n := copy(buf, data)
+	pool.Put(data)


Wouldn't it be simpler to just use a pool of buffered writers?

hrm, maybe. I find it simpler to reason with the buffer actually.

Stebalien · 2019-05-21T18:54:08Z

Well, except that the tests are failing.

Stebalien · 2019-05-21T18:55:00Z

multiplex.go

+		case data := <-mp.writeCh:
+			err := mp.writeMsg(data)
+			if err != nil {
+				log.Warningf("Error writing data: %s", err.Error())


Shouldn't we return here?

We probably should. There is an implicit return because the write failure closes the connection, which triggers the shutdown channel, but better to be explicit.

vyzo · 2019-05-21T19:03:43Z

The test failure is #47 -- this test has been failing for a while.

vyzo · 2019-05-21T19:12:52Z

The test passes if I add a small delay.

vyzo · 2019-05-21T19:13:24Z

Fixed the test as well.

main objection (delay too large) has been lifted, raul unavailable.

raulk

Only got to it after the fact.

raulk · 2019-05-23T11:16:07Z

multiplex.go

-		// only return this error if we don't *already* have an error from the write.
-		if err2 := mp.con.SetWriteDeadline(time.Time{}); err == nil && err2 != nil {
-			return err2
+	buf := pool.Get(4096)


Can we hold on to a static byte slice to avoid the pool operation? This method is not called concurrently, so it should be safe.

sure, that's probably a worthwhile optimization.

That'll take 4KiB when idle which can add up pretty quickly.

vyzo added 3 commits May 11, 2019 14:35

implement write coalescing

2c1c36f

fix tests

95dcec5

use timer instead of context for coalescing

9c074ae

vyzo requested a review from Stebalien May 11, 2019 12:46

ghost assigned vyzo May 11, 2019

ghost added the status/in-progress In progress label May 11, 2019

raulk previously requested changes May 11, 2019

View reviewed changes

vyzo changed the title ~~coalesce writes~~ [DO NOT MERGE] coalesce writes May 11, 2019

vyzo mentioned this pull request May 11, 2019

Write coalescing libp2p/go-libp2p#633

Closed

vyzo closed this May 11, 2019

ghost removed the status/in-progress In progress label May 11, 2019

vyzo reopened this May 13, 2019

ghost added the status/in-progress In progress label May 13, 2019

vyzo force-pushed the feat/coalesce-write branch from 0ecbf59 to 9c074ae Compare May 13, 2019 09:07

pre-allocate timer for write coalescing

0465341

This was referenced May 21, 2019

Make deadlines interrupt and coalesce writes whyrusleeping/yamux#28

Merged

Correctly implement write deadlines #55

Open

make write coalesce delay configurable, default to 1ms

6cf6509

vyzo changed the title ~~[DO NOT MERGE] coalesce writes~~ coalesce writes May 21, 2019

vyzo requested a review from raulk May 21, 2019 09:12

Stebalien approved these changes May 21, 2019

View reviewed changes

Stebalien requested changes May 21, 2019

View reviewed changes

vyzo added 2 commits May 21, 2019 22:06

return on write error

c0e7a8d

reduce default write coalesce delay to 100us

7a55767

vyzo requested a review from Stebalien May 21, 2019 19:10

fix test

03d48f4

Stebalien approved these changes May 21, 2019

View reviewed changes

vyzo merged commit aaf9be9 into master May 21, 2019

vyzo deleted the feat/coalesce-write branch May 21, 2019 21:17

raulk reviewed May 23, 2019

View reviewed changes

coalesce writes #54

coalesce writes #54

Conversation

vyzo commented May 11, 2019 • edited Loading

vyzo commented May 11, 2019

marten-seemann commented May 11, 2019

vyzo commented May 11, 2019

vyzo commented May 11, 2019

marten-seemann commented May 11, 2019 via email

raulk left a comment

Choose a reason for hiding this comment

vyzo commented May 11, 2019

vyzo commented May 11, 2019

vyzo commented May 11, 2019

vyzo commented May 11, 2019

vyzo commented May 13, 2019

vyzo commented May 21, 2019

marten-seemann commented May 21, 2019

vyzo commented May 21, 2019

vyzo commented May 21, 2019

Stebalien 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented May 21, 2019

vyzo commented May 21, 2019

vyzo commented May 21, 2019

raulk 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

vyzo commented May 11, 2019 •

edited

Loading