Opened 4 weeks ago

Closed 8 days ago

#2574 closed enhancement (fixed)

Reduce ACKSender.ACK_FREQUENCY

Reported by: Zlatin Balevsky Owned by: zzz
Priority: minor Milestone: 0.9.42
Component: router/transport Version: 0.9.41
Keywords: ssu Cc: jogger
Parent Tickets: Sensitive: no

Description (last modified by Zlatin Balevsky)

The current logic in determining how often to send SSU ACKs is the following:

return Math.max(rtt/2, ACK_FREQUENCY);

With the current value of 250 this assumes that the rtt to a given peer is at least 500ms.

I believe that value is too high. If the transfer is unidirectional as can be the case during streaming, there will be no data to piggyback acks in response, so this limits the effective throughput of the SSU connection as if its rtt was 500ms.

Update: the constant is used in a few other places, so instead of changing it maybe change the logic to only rely on take rtt into account?

Subtickets

Attachments (2)

ACK_FREQUENCY_analysis.ods (16.0 KB) - added by Zlatin Balevsky 3 weeks ago.
ACK_FREQUENCY_analysis2.ods (16.1 KB) - added by Zlatin Balevsky 2 weeks ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 weeks ago by Zlatin Balevsky

Cc: jogger added
Description: modified (diff)

comment:2 Changed 3 weeks ago by zzz

Milestone: undecided0.9.42
Status: newaccepted

I agree with your analysis, need to disentangle where else this constant may be used (or not).

It's a good point that this mainly affects one-way data transfer, but that is likely the norm given our unidirectional tunnels.

I skimmed through the other open SSU tickets including #2412 and it does not appear that this is covered in any other ticker or pending changes.

My instinct would be to cut it in half and see how it goes, possibly with further reductions in future releases.

comment:3 Changed 3 weeks ago by zzz

increased from 100 to 200, May 2006
increased from 200 to 500, June 2006
reduced from 500 to 350, Oct. 2012
reduced from 350 to 250, Jan. 2015

comment:4 Changed 3 weeks ago by Zlatin Balevsky

I'll benchmark all values from 250 down to 50 in 50ms steps on the testnet and upload the results in a spreadsheet. Will take me about a day because I'm getting 20 seamples at each step.

comment:5 Changed 3 weeks ago by zzz

The CLOCK_SKEW_FUDGE calculation in PeerState? should continue to be based on ACK_FREQUENCY.

Not so sure about PeerState? line 1891, that might be better based on the FIFOBandwidthRefiller frequency.

I'm going to do some testing with 125 out on the real net.

comment:6 Changed 3 weeks ago by zzz

looking fine so far, average RTT has dropped from high 200's to low 200's

Changed 3 weeks ago by Zlatin Balevsky

Attachment: ACK_FREQUENCY_analysis.ods added

comment:7 Changed 3 weeks ago by Zlatin Balevsky

No difference to overall throughput when tuning this value against revision 2ec35a4caf7e2622fd5aa9e8c4edc6c3aff9c80f but it may be worth trying again if any of the other ssu tickets land, more specifically #2576 and #2586

comment:8 Changed 2 weeks ago by zzz

PeerState? line 1891 (as referenced in comment 5) is being addressed in #2506

Changed 2 weeks ago by Zlatin Balevsky

Attachment: ACK_FREQUENCY_analysis2.ods added

comment:9 Changed 2 weeks ago by Zlatin Balevsky

I repeated the experiment with delay of 75ms at each container and there is an improvement in overall throughput when reducing the value, up to a point. Please see ACK_FREQUENCY_analysis2.ods

comment:10 Changed 2 weeks ago by jogger

good catch. Using really low values is somewhat pointless because of

Thread.sleep(5 + (ACK_FREQUENCY / 3));

which statistically further adds to the delay.

Instead of sleeping this thread could be notified just before packet pusher goes into wait(). This way both threads stop competing for the CPU and ACKsender runs more frequently when higher traffic.

comment:11 Changed 2 weeks ago by jogger

Just researching delays caused by context switches on my ARM32. Currently the above sleep parameter is 88ms, actually sleeping less on my systems and running 12-13 loops/sec. Is preempted multiple times during each run (>4 typical) which adds further delay. This would suggest using a sleep of 15-20 ms.

comment:12 Changed 8 days ago by zzz

Resolution: fixed
Status: acceptedclosed

Based on 2nd spreadsheet showing 150 is best (readings are bandwidth, higher is better), and my testing showing no ill effects with a 125 setting, changing to 150, in f1a75ea05319a92d8bc0c8bab819f54f9d54d22c to be 0.9.41-9

Note: See TracTickets for help on using tickets.