Opened 12 years ago

Closed 12 years ago

#3325 closed Bug (duplicate)

rechokeUploads is all wrong

Reported by: amarokuser Owned by: charles
Priority: Highest Milestone: None Set
Component: libtransmission Version: 2.00
Severity: Normal Keywords:
Cc:

Description

it unchokes leecher instead uploaders!

The patch changes it to what it is supposed to do - in a quite sophisticated manner.

Attachments (2)

patch (2.6 KB) - added by amarokuser 12 years ago.
patch.diff (2.9 KB) - added by amarokuser 12 years ago.

Download all attachments as: .zip

Change History (6)

Changed 12 years ago by amarokuser

comment:1 Changed 12 years ago by charles

A few thoughts on this patch:

  • It's clear that you've read and understood large pieces of libtransmission. That's much more than most bug reporters do, and it is greatly appreciated. I hope you'll keep looking for ways to improve the code. Don't let the rest of this list scare you off. :)
  • Under this patch, a Transmission seed would consistently and deliberately disconnect from its best peers. This seems counterproductive.
  • Even when not seeding, this patch will cause extreme oscillation in choking/unchoking because it prefers to upload to whoever it's not currently uploading to.
  • This seems to hurt Transmission users' odds of keeping their share ratios high.
  • It's hard to imagine how a swarm of only Transmission peers with this patch would be able to achieve good speeds with all the choking and unchoking.
  • No rationale is given for the patch's choice to disable the optimistic unchoke.
  • Changing MAX_BAD_PIECES_PER_PEER may or may not be good, but it's off-topic to peer choking.
  • Giving IPv6 peers a higher weight than IPv4 may or may not be good, but it's off-topic to peer choking.
  • The one-line description of this ticket is vague. That's a *very* minor point, and is intended more as a hint for future patches. Descriptive ticket subjects make the ticket system much nicer to use.
Last edited 12 years ago by charles (previous) (diff)

comment:2 follow-up: Changed 12 years ago by amarokuser

  • Component changed from Transmission to libtransmission
  • Owner set to charles

Thank you for the welcome flowers ;-) I assume you wrote (parts of) peer-mgr and you are not a mathematician.

  • In seed mode it won't "consistently and deliberately disconnect from its best peers", on the contrary it slides through all peers and distributes the goodness equally among them. What is a good peer in seed mode anyway?
  • When in download mode it reciprocates with its peers. Let me quote:
Reciprocation and number of uploads capping is managed by unchoking
the N peers which have the best upload rate and are interested.
This maximizes the client's download rate. These N peers are
referred to as downloaders, because they are interested in downloading
from the client.
  • The sliding window is large enough to avoid unnecessary oscillation. It does not "prefers to upload to whoever it's not currently uploading to" also on the contrary, it prefers peers that currently uploading over others. Look at the weight of up and down. I conducted tests with adding some noise. It turned out this is not necessary - see optimistic unchoking part. For stability reasons it is a good idea to add some jitter at "sub atomic" level (the effect is virtually not visible) see attached file.
This seems to hurt Transmission users' odds of keeping their share ratios high. 
  • Sorry, I don't get the point. What is the purpose of running file sharing programs - exchanging files. I see no point, if in download mode, to blast out huge amounts and getting nothing or little in return. File sharing is "tit-for-tat". So my goal is to stay at 1:1, the other question is - can I archive that? Some altruism is part of the game, so if you can manage 1.1 up for 1.0 down you are pretty fly.
  • I spend some time thinking about a swarm acting on this patch. The only drawback I see so far is that peers with small upload get a lower chance to download. This only applys in download mode, seed mode is not affected in any way (an equal distribution is almost the optimum to think of).
  • Actually I thing optimistic unchoke adds extreme oscillation in choking/unchoking. Apart from the isNew rule it is complete random/chaotic. In seed mode, as described above it is completely pointless, it simply add one upload slot. In download mode it is always "looking" for new good peers, even if we are maxed out. The patch on the other hand only "looks" for better peers if the currently upload set is no longer reciprocate with us. It also has some 5 per cent altruism build in. So I think optimistic unchoke is completely unnecessary.
  • The other things are more like hints and truly off-topic to peer choking.
  • being vague is one of my virtues ;-)
  • edit: btw what do you think stands TR_CLIENT_TO_PEER for? hint: its not our download :-)
Last edited 12 years ago by amarokuser (previous) (diff)

Changed 12 years ago by amarokuser

comment:3 in reply to: ↑ 2 Changed 12 years ago by charles

Replying to amarokuser:

Thank you for the welcome flowers ;-) I assume you wrote (parts of) peer-mgr and you are not a mathematician.

Getting your patches accepted into Transmission will be easier if you can control yourself to not sound like a troll. I'm glad that your patch works in "quite a sophisticated manner" but I would like to see more descriptive evidence that it "distributes the goodness equally" among the peers.

Here, specifically, is what your patch does for a seeding Transmission client: all peers will have a nonpositive ChokeData?.rate. Idle peers will have a ChokeData?.rate of 0. A peer that got one block from us recently will have a rate of -1000. Two blocks --> -2000, and so on.

rechokeUpload() then calls qsort() to sort the peers from fastest to slowest. Under your patch the "fastest" peers will be the idle peers with a value of 0, since 0 is larger than -1000, -2000, etc. So the peers are sorted from slowest to fastest.

Then compareChoke() unchokes the first N peers in the array (the slowest peers) and chokes the rest (the fastest peers).

I suppose this could be described as equal distribution, in the sense that we distribute equally poorly to all peers. Choking a peer after we've been seeding to it for only 10 seconds still seems unproductive to me.

comment:4 Changed 12 years ago by charles

  • Resolution set to duplicate
  • Status changed from new to closed

I notice you've created a new ticket #3334 whose patch seems to duplicate and supersede this patch, without addressing my comments here.

Since you've moved the contents of this patch into a new ticket, I don't see any reason to leave #3325 open any longer. If there is some aspect of this patch not covered by #3334, please reopen this ticket with an explanation.

Note: See TracTickets for help on using tickets.