Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

replace random nonces with message sequence numbers #617

Merged
merged 5 commits into from
Apr 30, 2015

Conversation

rade
Copy link
Member

@rade rade commented Apr 28, 2015

Fixes #564.

@rade
Copy link
Member Author

rade commented Apr 29, 2015

I did some benchmarking of this today. Running iperf I observed a ~1% drop compared to master, based on the highest throughput figure from 10 runs. The variance is much greater than 1% though, so I'm not going to read much into that result, and in any case a 1% drop would be perfectly acceptable. It is certainly possible that there might be a slight performance decrease with this change, since we have increased the encapsulation overhead by 6 bytes (2 byte offset in the old code vs 8 byte message sequence number here).

I also ran multiweave, attempting to determine how many weaves I can get to fully establish connections. On master I get as far as 32. On this branch to 50. The reason is likely twofold. Firstly, on master we run into #564, which this fixes. Secondly, crypto on the tcp connection should perform much better on this branch, and since connection establishment is mostly about the tcp connections, that likely makes a difference in the test.

@dpw
Copy link
Contributor

dpw commented Apr 29, 2015

Ok, I'm going to add a few specific comments about the current advanceState implementation. And then explain how the current windowing approach offends my sense of micro-optimization, and should all be different.

@rade
Copy link
Member Author

rade commented Apr 29, 2015

We never seem to actually use di.highestSeqNoSeen except to calculate its window number here.

Not quite. We have the following in the default case:

        if seqNo > di.highestSeqNoSeen {
            di.highestSeqNoSeen = seqNo
        }

@rade
Copy link
Member Author

rade commented Apr 29, 2015

Almost all fixed in the above commit. I will squash that, but figured it would be better to have it around temporarily so it's obvious what i've changed.

Good call on the naming and dispatching on the delta. And good catch on the overflow.

Re di.currentWindow - see comment above

Re ordering... I thought about this a fair bit when I wrote the code and experimented with a few variations. In the end I settled on the current version because the default case is special. It sticks out visually, due to the keyword and coming last. I therefore chose to make it the logically special case of the five. And that is the 0 case, which a) is the common case, b) the only case with a delta comparison not involving +/-1, and c) the only case where we have a nested conditional.

@dpw
Copy link
Contributor

dpw commented Apr 29, 2015

Not quite. We have the following in the default case:

Yes. But why can't that be turned into di.currentWindow = window in the appropriate places?

@rade
Copy link
Member Author

rade commented Apr 29, 2015

ah true. Just realised that myself :)

rade added 5 commits April 30, 2015 14:47
Since TCP is reliable, we do not need to transmit the nonce itself;
the recipient can simply construct the nonce from its own message
counter. Hence there is no message envelope and we can thus remove a
whole layer of encoding.
we only needed it because of the nonce channels
@dpw dpw merged commit cdf7542 into weaveworks:master Apr 30, 2015
dpw added a commit that referenced this pull request Apr 30, 2015
replace random nonces with message sequence numbers

Fixes #564.
@rade rade modified the milestone: 0.11.0 May 12, 2015
@awh awh mentioned this pull request Jun 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto errors can stall udp receiver loop
2 participants