optimistic unchoke timer is too short

[Longinus00]

Optimistic unchokes currently get recalculated each call of rechokeUploads, every 10 seconds. This doesn't give much, if any, time for the other peers tit-for-tat strategy to kick in.

[charles]

How other FOSS clients handle it:

• By default, libtorrent-rb leaves a torrent optimistically unchoked for four rechoke intervals.

• rtorrent's approach seems to be random

• monotorrent's interval is 30 seconds. As a side note, monotorrent's source code is pretty easy on the eyes.

• ktorrent's interval is 30 seconds.

[Longinus00]

30 seconds is what is specified on theory.org.

• vuze is 30 seconds.

• libtorrent-rb(0.15.3) goes a bit further and adjusts the interval to allow the peer to download a whole piece.

If you're working on #2997 then this could be rolled into that.

[charles]

first draft

[Longinus00]

I'm a little confused by your diff. Won't this in effect make every peer have a 30 second rechoke interval and not just the optimistic peer? Also, won't have_optimistic_unchoke get overwritten a bunch of times and cause it to skip over unchoked peers leading to it unchoking more than it should??

[charles]

Also, won't have_optimistic_unchoke get overwritten a bunch of times and cause it to skip over unchoked peers leading to it unchoking more than it should??

Oops, that should have been a "|=" instead of "="

Won't this in effect make every peer have a 30 second rechoke interval and not just the optimistic peer?

Yes. Previously peer-msgs.c had an implicit interval that silently ignored requests that came too frequently from peer-mgr.c. The periods can be revised but the larger point is to remove the implicit interval and consolidate all the logic into the peer-mgr unchoke timer.

Now, about revising the periods -- maybe 30 seconds is too long for non-optimistic unchokes. And chokes could be handled separately from both kinds of unchokes -- possibly with no minimum choke interval at all.

Attached is a revised patch which enforces no interval on choked peers, a 30 second minimum for optimistic unchokes, and a 20 second minimum on non-optimistic unchokes.

[Longinus00]

theory.org states the following about rechoking intervals.

The currently deployed choking algorithm avoids fibrillation by only changing choked peers once every ten seconds.

I'm not sure this needs to be as complicated as your patch. How about this: Skip over the current optimistic unchoke in the first for loop.

else if( t->optimistic != peer )
{
/* stuff choke array with peer */
}

To prevent a peer from never losing the optimistic status if peer counts drop too low, clear the optimistic peer so that it is released back into the choke array next time around.

if( unchokedInterested < session->uploadSlotsPerTorrent )
    t->optimistic = NULL;

Then run the optimistic unchoke selection once every 3 times through rechokeUploads and have it choke the previous optimistic unchoke.

if( t->optimisticCounter > 3 && !isMaxedOut && (i<size) )
{
/* snip */
    if(( n = tr_ptrArraySize( &randPool ) ))
        t->optimisiticCounter = 0;
        if( t->optimistic )
            tr_peerMsgsSetChoke( t->optimistic->msgs, TRUE );
/* snip */
}
else
{
    t->optimisticCounter++;
}

[charles]

similar to choke-2, but adds a new constraint: after choking a peer, don't unchoke it until MIN_CHOKE_PERIOD_SEC has elapsed

[charles]

I didn't see your comment before uploading choke-3.diff. Not a bad idea.

[charles]

I think we can make it yet simpler by moving some of the multiplier code and "t->optimistic = NULL" code outside of other conditionals.

We add a new constant:

+    /* an optimistically unchoked peer is immune from rechoking
+       for this many calls to rechokeUploads(). */
+    OPTIMISTIC_UNCHOKE_MULTIPLIER = 4,

At the beginning of rechokeUploads(), before the first for loop:

+    /* optimistic unchoke peers lose their "optimistic" state
+     * (and therefore become candidates for rechoking)
+     * after N calls to rechokeUploads() */
+    if( t->optimisticUnchokeTimeScaler > 0 )
+        if( !--t->optimisticUnchokeTimeScaler )
+            t->optimistic = NULL;

The choke-array-building-loop is modified as you suggested:

-        else
+        else if( t->optimistic != peer )
         {
             struct ChokeData * n = &choke[size++];
             n->peer         = peer;

Lastly, the optimistic-unchoke section takes two one-liners:

-    if( !isMaxedOut && (i<size) )
+    if( !t->optimistic && !isMaxedOut && (i<size) )
     {
         int n;
         struct ChokeData * c;
@@ -2787,6 +2798,7 @@
             c = tr_ptrArrayNth( &randPool, tr_cryptoWeakRandInt( n ));
             c->isChoked = FALSE;
             t->optimistic = c->peer;
+            t->optimisticUnchokeTimeScaler = OPTIMISTIC_UNCHOKE_MULTIPLIER;

[Longinus00]

Looks good. I would like to suggest changing my

+        else if( t->optimistic != peer )

to

+        else if( peer != t->optimistic )

as that reads a bit better.

[charles]

Fixed in trunk by r11307.