Cc and optimization pr #37

pietdevaere · 2017-11-30T07:59:27Z

this is #30 and #33 on top of #36

When selecting the number of frames and acks to be transmitted, the header size and AEAD overhead were not considered when determining the space left in the packet.

Problem was that it was decied to generate an ack frame BEFORE it was known that there was space to put acks in the packet. This resulted in an empty rs slice, causing errors

should now be able to handle ack rangesx

This reverts commit f5d96d2.

When selecting the number of frames and acks to be transmitted, the header size and AEAD overhead were not considered when determining the space left in the packet.

Problem was that it was decied to generate an ack frame BEFORE it was known that there was space to put acks in the packet. This resulted in an empty rs slice, causing errors

should now be able to handle ack rangesx

This reverts commit f5d96d2.

ekr · 2017-11-30T13:54:45Z

@pietdevaere: the integration pieces look basically sound. I have some minor comments as you can see. It's going to take me some time to review the congestion piece, and I see it doesn't have any tests so I suggest we land that separately. Specifically, I suggest:

Clean up the issues I noted.
Add a dummy congestion controller that just allows sending at all times. Thanks for doing an interface there, that makes things much easier.
Fix the merge issues (I may be able to help with that, as it looks like in part you just have duplicate commits)
Land all the code except for congestion.go

We should be able to do that today or tomorrow depending on your availability.

Separately, we can:

Write tests for congestion.go
Review congestion.go
Land it

I don't plan to work on that file myself (modulo review), so it shouldn't cause any merge/rebase issues.

Does that work for you?

pietdevaere · 2017-11-30T14:51:23Z

That should work for me. I can make time for this tomorrow (and maybe tonight). I was planning to work on the congestion controller soon anyway (adding RTT estimation, not to big of a change).

Three remarks:

I can not see your comments. Can you check what's up with that?
That the congestion controller is easy to swap out is by design. I thought it would be useful to make it easy to experiment with different congestion controllers.
congestion.go is a pretty direct implementation of draft-ietf-quic-recovery-06. That probably makes it easier to review.

ekr · 2017-11-30T13:04:15Z

frame.go

@@ -118,7 +119,7 @@ func decodeFrame(data []byte) (uintptr, *frame, error) {
 		return 0, nil, err
 	}

-	return n, &frame{0, inner, data[:n], nil, time.Now()}, nil
+	return n, &frame{0, inner, data[:n], nil, time.Now(), true}, nil


Why is this not false? I am assuming we're not going to transmit this frame.

agreed, done.

ekr · 2017-11-30T13:04:49Z

stream.go

+	/* First check if we can append the new slice at the end */
+	if l := len(h.chunks); l == 0 || offset > h.chunks[l - 1].offset {
+		h.chunks = append(h.chunks, c)
+	/* Otherwise find out where it should go */


Please put this comment after the opening brace.

ekr · 2017-11-30T13:06:56Z

stream.go

-		if offset < h.chunks[i].offset {
-			break
+	/* First check if we can append the new slice at the end */
+	if l := len(h.chunks); l == 0 || offset > h.chunks[l - 1].offset {


Given that we're going to likely need len(chunks) repeatedly, let's instead do

var nchunks = len(h.chunks)

and then use nchunks repeatedly below

ekr · 2017-11-30T13:08:41Z

tracking.go

-	pk.acked2 = true
+	if pn >= p.minNotAcked2 {
+		pk, ok := p.packets[pn]
+		//assert(ok)


Should this packet always be in the array? If so, you want the assert and not the if.

no, it doesn't have to. If a packet has been acked2ed before it might have been removed from the map before. The assert was probably still there because removing the packets is something I added. I removed the assert.

ekr · 2017-11-30T13:16:14Z

connection.go

@@ -393,7 +401,7 @@ func (c *Connection) determineAead(pt uint8) cipher.AEAD {
 	return aead
 }

-func (c *Connection) sendPacketRaw(pt uint8, connId ConnectionId, pn uint64, version VersionNumber, payload []byte) error {
+func (c *Connection) sendPacketRaw(pt uint8, connId ConnectionId, pn uint64, version VersionNumber, payload []byte, onlyAcks bool) error {


does onlyAcks here mean "i am only acks" or "send only acks"? Please rename accordinly

ekr · 2017-11-30T13:29:34Z

connection.go

 		// c.log(logTypeStream, "Examining frame=%v", f)
-		l, err := f.length()
+
+		frameLenght, err := f.length()


typo: frameLength

ekr · 2017-11-30T13:30:11Z

connection.go

 		if err != nil {
 			return 0, err
 		}

+		// if there is no more space in the congestion window, stop
+		// trying to send stuff
+		if (spaceInCongestionWindow < frameLenght){


This test seems like it should go after needsTransmit and the too recent test.

agreed & done

ekr · 2017-11-30T13:39:59Z

connection.go

+		for _, frame := range queue {
+			for _, pn := range frame.pns {
+				if pn == lostPn {
+					frame.needsTransmit = true


This looks wrong to me. Say we have a frame which appears in packet 1 and packet 2 and we get an ack for 2, but 1 is lost, why are we marking it for needsTransmit? Perhaps the frame has already been removed? At minimum you're gonna need a comment explaining why this is cool.

if this logic is right, you want a break at line 843 because you don't need to look at the rest of the pns

agreed, changed and commented.

ekr · 2017-11-30T13:40:22Z

connection.go

@@ -759,6 +834,19 @@ func (c *Connection) queueStreamFrames(pt uint8, protected bool, bareAcks bool)
 	return sent, nil
 }

+func (c *Connection) handleLostPackets(lostPn uint64){


"handleLostPacket" because there is only one,

ekr · 2017-11-30T13:48:47Z

connection.go

 	// TODO([email protected]): Process the ACK timestamps.
+
+	//TODO([email protected]) add timestamping stuff


We can remove comments about timestamps because we're not supporting them

Hand-merge + squash of Piet De Vaere's giant PR. Fixes #37.

pietdevaere added 30 commits November 2, 2017 10:13

Fixing bug with oversized packets

f7e1322

When selecting the number of frames and acks to be transmitted, the header size and AEAD overhead were not considered when determining the space left in the packet.

requested changes

010eae9

Fixed index_out_of_bound error when generating ack frames

75fea45

Problem was that it was decied to generate an ack frame BEFORE it was known that there was space to put acks in the packet. This resulted in an empty rs slice, causing errors

Redid ACK generation / processing. UNTESTED

375e1cf

should now be able to handle ack rangesx

I am probably going to hell for this...

0a0671b

first setup for congestion controller

30a2e31

splitting enqueueing and transmitting of frames

ea41938

first very rudementary static window congestion control

1adf58e

basic loss detection framework in place

c55c3a0

not counting ack only packets in congestion control

f80c845

debugging of basic congestion control

02bf40a

logging, loss detection, dynamic window size

a438bea

fix bug in loss detection

432cd7f

minor changes preparing PR

fc062da

second pass to prepare for PR

1244cb3

larger UDP buffer sizes

876dc0c

optimizing prepareAckRange

d4f2479

optimizing setAcked2

ce1796e

unifying and nicyfing setting buffer size

485558d

Revert "I am probably going to hell for this..."

9db8f10

This reverts commit f5d96d2.

optimizing insertSortedChunk for writes

f9bb6f2

Fixing bug with oversized packets

1dc70a4

When selecting the number of frames and acks to be transmitted, the header size and AEAD overhead were not considered when determining the space left in the packet.

Fixed index_out_of_bound error when generating ack frames

e8b024b

Problem was that it was decied to generate an ack frame BEFORE it was known that there was space to put acks in the packet. This resulted in an empty rs slice, causing errors

Redid ACK generation / processing. UNTESTED

f32c183

should now be able to handle ack rangesx

I am probably going to hell for this...

3760b35

first setup for congestion controller

85e7c70

splitting enqueueing and transmitting of frames

1476d85

first very rudementary static window congestion control

6a49cb5

basic loss detection framework in place

bbdd48d

not counting ack only packets in congestion control

f032710

pietdevaere added 15 commits November 11, 2017 20:19

debugging of basic congestion control

a50d189

logging, loss detection, dynamic window size

b6f43a1

fix bug in loss detection

612e067

minor changes preparing PR

e2cb92c

second pass to prepare for PR

da9f75e

Revert "I am probably going to hell for this..."

9a41771

This reverts commit f5d96d2.

fixing br0ken rebase

de29a57

c++ style comments

b680780

Merge branch 'congestion_control_PR' into buffer_ack_optimization_PR

9c9ea72

call StreamReadable handler after processing all frames

e1bdbd2

remove commented code

ad3b982

only flush queues when needed

beb1d02

requested changes

e142c0b

Merge branch 'StreamReadable_PR' into HEAD

51277b3

Merge branch 'buffer_ack_optimization_PR' into ekr_master

3cab416

This was referenced Nov 30, 2017

Better acks, better chunk inserts & larger buffers #33

Closed

Basic congestion Control #30

Closed

minor changes

5c18d18

ekr requested changes Nov 30, 2017

View reviewed changes

pietdevaere added 2 commits December 1, 2017 10:57

requested changes

198869f

dummy congestion controller

3d8a9b3

ekr closed this in 1b6d92b Dec 2, 2017

ekr added a commit that referenced this pull request Dec 2, 2017

Merge pull request #38 from ekr/pietdv_cc_pr

3d31ff2

Hand-merge + squash of Piet De Vaere's giant PR. Fixes #37.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cc and optimization pr #37

Cc and optimization pr #37

pietdevaere commented Nov 30, 2017

ekr commented Nov 30, 2017

pietdevaere commented Nov 30, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

pietdevaere Dec 1, 2017

ekr Nov 30, 2017

		// TODO([email protected]): Process the ACK timestamps.

		//TODO([email protected]) add timestamping stuff

Cc and optimization pr #37

Cc and optimization pr #37

Conversation

pietdevaere commented Nov 30, 2017

ekr commented Nov 30, 2017

pietdevaere commented Nov 30, 2017

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

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