Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#3596 closed Bug (fixed)

optimistic unchoke timer is too short

Reported by: Longinus00 Owned by: charles
Priority: Normal Milestone: 2.11
Component: libtransmission Version: 2.10
Severity: Normal Keywords:
Cc:

Description

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.

Attachments (3)

choke.diff (8.7 KB) - added by charles 11 years ago.
first draft
choke-2.diff (9.3 KB) - added by charles 11 years ago.
choke-3.diff (9.8 KB) - added by charles 11 years ago.
similar to choke-2, but adds a new constraint: after choking a peer, don't unchoke it until MIN_CHOKE_PERIOD_SEC has elapsed

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by charles

  • Milestone changed from None Set to 2.11
  • Status changed from new to assigned

comment:2 Changed 11 years ago by charles

  • Milestone changed from 2.11 to Sometime

comment:3 Changed 11 years ago by 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.

comment:4 Changed 11 years ago by 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.

Last edited 11 years ago by Longinus00 (previous) (diff)

Changed 11 years ago by charles

first draft

comment:5 Changed 11 years ago by 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??

comment:6 Changed 11 years ago by 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.

Last edited 11 years ago by charles (previous) (diff)

Changed 11 years ago by charles

comment:7 Changed 11 years ago by 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++;
}

Changed 11 years ago by 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

comment:8 Changed 11 years ago by charles

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

comment:9 Changed 11 years ago by 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 )
+        --t->optimisticUnchokeTimeScaler;
+    else
+        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;
Last edited 11 years ago by charles (previous) (diff)

comment:10 Changed 11 years ago by 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.

comment:11 Changed 11 years ago by charles

  • Keywords backport-2.0x added
  • Milestone changed from Sometime to 2.11
  • Resolution set to fixed
  • Status changed from assigned to closed

comment:12 Changed 11 years ago by charles

Fixed in trunk by r11307.

comment:13 Changed 11 years ago by charles

  • Keywords backport-2.0x removed
Note: See TracTickets for help on using tickets.