BDP estimation and window update. #1310
Conversation
transport/bdp_estimator.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| bdpPing = &ping{data: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}} |
There was a problem hiding this comment.
Half-serious comment:
How about:
2,4,16,16,9,14,7,7 ?
98,100,112,80,105,110,103,33 ?
103,82,80,67,45,103,111,0 ?
transport/bdp_estimator.go
Outdated
|
|
||
| const ( | ||
| limit = (1 << 20) * 4 | ||
| alpha = 0.9 |
There was a problem hiding this comment.
"Almost completely opaque"? :)
Please document these constants.
transport/bdp_estimator.go
Outdated
| ) | ||
|
|
||
| type bdpEstimator struct { | ||
| side string |
There was a problem hiding this comment.
Is this only for debugging? Maybe remove it to avoid the extra memory...
transport/bdp_estimator.go
Outdated
|
|
||
| mu sync.Mutex | ||
| bdp uint32 | ||
| sample uint32 // Current bdp sample. |
There was a problem hiding this comment.
Doc the units, please. Maybe:
// Bytes received during this sample. ?
transport/bdp_estimator.go
Outdated
| bdp uint32 | ||
| sample uint32 // Current bdp sample. | ||
| sentAt time.Time // Time when the ping was sent. | ||
| bwMax float64 |
| if opts.InitialConnWindowSize >= defaultWindowSize { | ||
| icwz = opts.InitialConnWindowSize | ||
| dynamicWindow = false |
There was a problem hiding this comment.
Can this not be false?
If I see an option called "Initial ______", I don't assume that will disable dynamic updates to ____. Ideally there should be a different option to set to prevent BDP calculation from affecting the window sizes.
There was a problem hiding this comment.
Some follow-up work needed for this.
transport/bdp_estimator.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| limit = (1 << 20) * 4 |
There was a problem hiding this comment.
Nit: prefix consts with "bdp" to avoid name collisions.
transport/http2_client.go
Outdated
| if dynamicWindow { | ||
| t.bdpEst = &bdpEstimator{ | ||
| bdp: initialWindowSize, | ||
| updateFlowControl: func(n uint32) { |
There was a problem hiding this comment.
Can you add a method to http2Client to do this instead of inlining it, please? So this would be:
updateFlowControl: t.updateFlowControl| if sendBDPPing { | ||
| t.controlBuf.put(&windowUpdate{0, uint32(size), false}) | ||
| t.controlBuf.put(bdpPing) | ||
| } else { |
There was a problem hiding this comment.
Is it not a problem to skip the onData/onRead calls into the flow control?
For a later discussion, not this PR: should the flow control be aware of the BDP estimator?
There was a problem hiding this comment.
Resolved after offline discussion
transport/http2_client.go
Outdated
| @@ -927,6 +968,10 @@ func (t *http2Client) handleSettings(f *http2.SettingsFrame) { | |||
|
|
|||
| func (t *http2Client) handlePing(f *http2.PingFrame) { | |||
| if f.IsAck() { // Do nothing. | |||
transport/bdp_estimator.go
Outdated
| @@ -89,13 +106,14 @@ func (b *bdpEstimator) calculate(d [8]byte) { | |||
| // If the current sample (which is smaller than or equal to the 1.5 times the real BDP) is | |||
| // greater than or equal to 2/3rd our perceived bdp AND this is the maximum bandwidth seen so far, we | |||
| // should update our perception of the network BDP. | |||
| if float64(b.sample) >= float64(0.66)*float64(b.bdp) && bwCurrent == b.bwMax { | |||
| if float64(b.sample) >= float64(beta)*float64(b.bdp) && bwCurrent == b.bwMax && b.bdp != bdpLimit { | |||
transport/bdp_estimator.go
Outdated
| @@ -89,13 +106,14 @@ func (b *bdpEstimator) calculate(d [8]byte) { | |||
| // If the current sample (which is smaller than or equal to the 1.5 times the real BDP) is | |||
| // greater than or equal to 2/3rd our perceived bdp AND this is the maximum bandwidth seen so far, we | |||
| // should update our perception of the network BDP. | |||
| if float64(b.sample) >= float64(0.66)*float64(b.bdp) && bwCurrent == b.bwMax { | |||
| if float64(b.sample) >= float64(beta)*float64(b.bdp) && bwCurrent == b.bwMax && b.bdp != bdpLimit { | |||
| // Put our bdp to be smaller than or equal to twice the real BDP. | |||
| // We really should multiply with 4/3, however to round things out | |||
There was a problem hiding this comment.
Move this comment to where gamma is defined. Maybe add something like:
"If gamma were 4/3, we would set our new BDP estimate to double the measured BDP".
Alternatively, express gamma as that scaling factor and have another const of 1.5 for the 1.5 round trips covered by the sample. Or even gamma = 3/1.5?
menghanl
added
1.5
Type: Performance
#1280
This is the first iteration for BDP estimation based on the algorithm proposed by @ejona86.
With this we get significant performance boost on high latency networks with varied message sizes.
Following are some benchmark results:
Cross-continental network (RTT: ~152ms)
Benchmark scenario:
Unmerged benchmark code
The same benchmark was run locally for different latency values. Latency was simulated using @dfawley 's implementation.
Following are the results: