request partial pieces in rarest-first order

include/libtorrent/piece_picker.hpp

@@ -480,6 +480,8 @@ namespace libtorrent
 
 		std::pair<int, int> distributed_copies() const;
 
+		void set_num_pad_files(int n) { m_num_pad_files = n; }
+
 	private:
 
 		friend struct piece_pos;
@@ -705,8 +707,6 @@ namespace libtorrent
 			{ return index == p.index && peer_count == p.peer_count; }
 		};
 
-		void set_num_pad_files(int n) { m_num_pad_files = n; }
-
 	private:
 
 #ifndef TORRENT_DEBUG_REFCOUNTS
@@ -717,6 +717,9 @@ namespace libtorrent
 #endif
 #endif
 
+		bool partial_compare_rarest_first(downloading_piece const* lhs
+		, downloading_piece const* rhs) const;
+
 		void break_one_seed();
 
 		void update_pieces() const;

src/piece_picker.cpp

@@ -1888,6 +1888,28 @@ namespace libtorrent
 			src.clear();
 			return num_blocks - to_copy;
 		}
+
+	}
+
+	// lower availability comes first. This is a less-than comparison, it returns
+	// true if lhs has lower availability than rhs
+	bool piece_picker::partial_compare_rarest_first(downloading_piece const* lhs
+		, downloading_piece const* rhs) const
+	{
+		int lhs_availability = m_piece_map[lhs->index].peer_count;
+		int rhs_availability = m_piece_map[rhs->index].peer_count;
+		if (lhs_availability != rhs_availability)
+			return lhs_availability < rhs_availability;
+
+		// if the availability is the same, prefer the piece that's closest to
+		// being complete.
+		int lhs_blocks_left = m_blocks_per_piece - lhs->finished - lhs->writing
+			- lhs->requested;
+		TORRENT_ASSERT(lhs_blocks_left > 0);
+		int rhs_blocks_left = m_blocks_per_piece - rhs->finished - rhs->writing
+			- rhs->requested;
+		TORRENT_ASSERT(rhs_blocks_left > 0);
+		return lhs_blocks_left < rhs_blocks_left;
 	}
 
 	// pieces describes which pieces the peer we're requesting from has.
@@ -1983,29 +2005,58 @@ namespace libtorrent
 
 		if (options & prioritize_partials)
 		{
-			// TODO: 3 prioritize partials correctly. either by rarity or by which
-			// one is closest to being complete
+			// first, allocate a small array on the stack of all the partial
+			// pieces (downloading_piece). We'll then sort this list by
+			// availability or by some other condition. The list of partial pieces
+			// in m_downloads is ordered by piece index, this is to have O(log n)
+			// lookups when finding a downloading_piece for a specific piece index.
+			// this is important and needs to stay sorted that way, that's why
+			// we're copying it here
+			downloading_piece const** ordered_partials = TORRENT_ALLOCA(
+				downloading_piece const*, m_downloads[piece_pos::piece_downloading].size());
+			int num_ordered_partials = 0;
+
+			// now, copy over the pointers. We also apply a filter here to not
+			// include ineligible pieces in certain modes. For instance, a piece
+			// that the current peer doesn't have is not included.
 			for (std::vector<downloading_piece>::const_iterator i
 				= m_downloads[piece_pos::piece_downloading].begin()
 				, end(m_downloads[piece_pos::piece_downloading].end()); i != end; ++i)
 			{
+				pc.inc_stats_counter(counters::piece_picker_partial_loops);
+
 				// in time critical mode, only pick high priority pieces
 				if ((options & time_critical_mode)
 					&& piece_priority(i->index) != priority_levels - 1)
 					continue;
 
-				pc.inc_stats_counter(counters::piece_picker_partial_loops);
 				if (!is_piece_free(i->index, pieces)) continue;
+
 				TORRENT_ASSERT(m_piece_map[i->index].download_queue()
 					== piece_pos::piece_downloading);
-				if (int(backup_blocks.size()) >= num_blocks
-					&& int(backup_blocks2.size()) >= num_blocks)
-					break;
 
-				num_blocks = add_blocks_downloading(*i, pieces
+				ordered_partials[num_ordered_partials++] = &*i;
+			}
+
+			// now, sort the list.
+			// TODO: this could probably be optimized by incrementally
+			// calling partial_sort to sort one more element in the list. Because
+			// chances are that we'll just need a single piece, and once we've
+			// picked from it we're done. Sorting the rest of the list in that
+			// case is a waste of time.
+			std::sort(ordered_partials, ordered_partials + num_ordered_partials
+				, boost::bind(&piece_picker::partial_compare_rarest_first, this
+					, _1, _2));
+
+			for (int i = 0; i < num_ordered_partials; ++i)
+			{
+				num_blocks = add_blocks_downloading(*ordered_partials[i], pieces
 					, interesting_blocks, backup_blocks, backup_blocks2
 					, num_blocks, prefer_contiguous_blocks, peer, speed, options);
 				if (num_blocks <= 0) return;
+				if (int(backup_blocks.size()) >= num_blocks
+					&& int(backup_blocks2.size()) >= num_blocks)
+					break;
 			}
 
 			num_blocks = append_blocks(interesting_blocks, backup_blocks

test/test_piece_picker.cpp

@@ -611,14 +611,36 @@ int test_main()
 
 // ========================================================
 
-	// make sure downloading pieces closer to completion have higher priority
-	// piece 3 has only 1 block from being completed, and should be picked
-//	print_title("test downloading piece order");
-//	p = setup_picker("1111111", "       ", "", "013700f");
-//	picked = pick_pieces(p, "*******", 1, 0, 0, piece_picker::fast
-//		, options | piece_picker::prioritize_partials, empty_vector);
-//	TEST_CHECK(int(picked.size()) > 0);
-//	TEST_CHECK(picked.front() == piece_block(3, 3));
+	// when we're prioritizing partial pieces, make sure to first pick the
+	// rarest of them. The blocks in this test are:
+	// 0: [    ] avail: 1
+	// 1: [x   ] avail: 1
+	// 2: [xx  ] avail: 1
+	// 3: [xxx ] avail: 2
+	// 4: [    ] avail: 1
+	// 5: [    ] avail: 1
+	// 6: [xxxx] avail: 1
+	// piece 6 does not have any blocks left to pick, even though piece 3 only
+	// has a single block left before it completes, it is less rare than piece
+	// 2. Piece 2 is the best pick in this case.
+	print_title("test partial piece order (rarest first)");
+	p = setup_picker("1112111", "       ", "", "013700f");
+	picked = pick_pieces(p, "*******", 1, 0, 0, piece_picker::fast
+		, options | piece_picker::prioritize_partials, empty_vector);
+	TEST_CHECK(int(picked.size()) > 0);
+	TEST_CHECK(picked.front() == piece_block(2, 2)
+		|| picked.front() == piece_block(2, 3));
+
+	// as a tie breaker, make sure downloading pieces closer to completion have
+	// higher priority. piece 3 is only 1 block from being completed, and should
+	// be picked
+
+	print_title("test partial piece order (most complete)");
+	p = setup_picker("1111111", "       ", "", "013700f");
+	picked = pick_pieces(p, "*******", 1, 0, 0, piece_picker::fast
+		, options | piece_picker::prioritize_partials, empty_vector);
+	TEST_CHECK(int(picked.size()) > 0);
+	TEST_CHECK(picked.front() == piece_block(3, 3));
 
 // ========================================================