add piece picker test for re-prioritizing downloading/writing/partial pieces (#989)

add piece picker test for re-prioritizing partial pieces and forward port steven's piece picker fix from RC_1_1

include/libtorrent/piece_picker.hpp

@@ -330,8 +330,6 @@ namespace libtorrent
 		void check_peers();
 #endif
 
-//		int get_block_state(piece_block block) const;
-
 		// returns true if any client is currently downloading this
 		// piece-block, or if it's queued for downloading by some client
 		// or if it already has been successfully downloaded
@@ -362,9 +360,6 @@ namespace libtorrent
 
 		void piece_passed(int index);
 
-//		void mark_as_checking(int index);
-//		void mark_as_done_checking(int index);
-
 		// returns information about the given piece
 		void piece_info(int index, piece_picker::downloading_piece& st) const;
 

src/piece_picker.cpp

@@ -356,7 +356,9 @@ namespace libtorrent
 		}
 	}
 
-	void piece_picker::verify_priority(int range_start, int range_end, int prio) const
+	void piece_picker::verify_priority(int const range_start
+		, int const range_end
+		, int const prio) const
 	{
 		TORRENT_ASSERT(range_start <= range_end);
 		TORRENT_ASSERT(range_end <= int(m_pieces.size()));
@@ -616,7 +618,7 @@ namespace libtorrent
 		for (std::vector<piece_pos>::const_iterator i = m_piece_map.begin();
 			i != m_piece_map.end(); ++i)
 		{
-			int index = static_cast<int>(i - m_piece_map.begin());
+			int const index = static_cast<int>(i - m_piece_map.begin());
 			piece_pos const& p = *i;
 
 			if (p.filtered())
@@ -642,7 +644,8 @@ namespace libtorrent
 			if (t != nullptr)
 				TORRENT_ASSERT(!t->have_piece(index));
 
-			int prio = p.priority(this);
+			int const prio = p.priority(this);
+
 #if TORRENT_USE_ASSERTS
 			if (p.downloading())
 			{
@@ -659,7 +662,7 @@ namespace libtorrent
 
 			if (!m_dirty)
 			{
-				TORRENT_ASSERT(prio < int(m_priority_boundaries.size()) || m_dirty);
+				TORRENT_ASSERT(prio < int(m_priority_boundaries.size()));
 				if (prio >= 0)
 				{
 					TORRENT_ASSERT(p.index < m_pieces.size());
@@ -773,7 +776,7 @@ namespace libtorrent
 		return std::make_pair(min_availability + m_seeds, fraction_part * 1000 / num_pieces);
 	}
 
-	std::pair<int, int> piece_picker::priority_range(int prio)
+	std::pair<int, int> piece_picker::priority_range(int const prio)
 	{
 		TORRENT_ASSERT(prio >= 0);
 		TORRENT_ASSERT(prio < int(m_priority_boundaries.size()) || m_dirty);
@@ -789,7 +792,7 @@ namespace libtorrent
 		TORRENT_ASSERT(!m_dirty);
 		TORRENT_ASSERT(index >= 0);
 		TORRENT_ASSERT(index < int(m_piece_map.size()));
-		piece_pos& p = m_piece_map[index];
+		piece_pos const& p = m_piece_map[index];
 		TORRENT_ASSERT(!p.filtered());
 		TORRENT_ASSERT(!p.have());
 
@@ -802,7 +805,7 @@ namespace libtorrent
 
 		TORRENT_ASSERT(int(m_priority_boundaries.size()) >= priority);
 
-		auto range = priority_range(priority);
+		auto const range = priority_range(priority);
 		int new_index = (range.second == range.first)
 			? range.first
 			: random(range.second - range.first) + range.first;
@@ -918,7 +921,7 @@ namespace libtorrent
 		piece_pos& p = m_piece_map[index];
 		TORRENT_ASSERT(int(p.index) == elem_index || p.have());
 
-		int new_priority = p.priority(this);
+		int const new_priority = p.priority(this);
 
 		if (new_priority == priority) return;
 
@@ -1069,9 +1072,9 @@ namespace libtorrent
 		i->locked = false;
 
 		piece_pos& p = m_piece_map[index];
-		int prev_priority = p.priority(this);
+		int const prev_priority = p.priority(this);
 		erase_download_piece(i);
-		int new_priority = p.priority(this);
+		int const new_priority = p.priority(this);
 
 #if TORRENT_USE_INVARIANT_CHECKS
 		check_piece_state();
@@ -1502,9 +1505,9 @@ namespace libtorrent
 			, end(m_piece_map.end()); i != end; ++i, ++index)
 		{
 			piece_pos& p = *i;
-			int prio = p.priority(this);
+			int const prio = p.priority(this);
 			if (prio == -1) continue;
-			int new_index = (prio == 0 ? 0 : m_priority_boundaries[prio - 1]) + p.index;
+			int const new_index = (prio == 0 ? 0 : m_priority_boundaries[prio - 1]) + p.index;
 			m_pieces[new_index] = index;
 		}
 
@@ -1631,8 +1634,8 @@ namespace libtorrent
 			<< index << ")" << std::endl;
 #endif
 		piece_pos& p = m_piece_map[index];
-		int info_index = p.index;
+		int const info_index = p.index;
-		int priority = p.priority(this);
+		int const priority = p.priority(this);
 		TORRENT_ASSERT(priority < int(m_priority_boundaries.size()) || m_dirty);
 
 		if (p.have()) return;
@@ -1690,11 +1693,9 @@ namespace libtorrent
 		TORRENT_ASSERT(p.priority(this) == -1);
 	}
 
-	bool piece_picker::set_piece_priority(int index, int new_piece_priority)
+	bool piece_picker::set_piece_priority(int const index, int const new_piece_priority)
 	{
-#ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS
 		INVARIANT_CHECK;
-#endif
 
 #ifdef TORRENT_PICKER_LOG
 		std::cerr << "[" << this << "] " << "set_piece_priority(" << index
@@ -1780,7 +1781,13 @@ namespace libtorrent
 		TORRENT_ASSERT(m_num_have_filtered >= 0);
 
 		p.piece_priority = new_piece_priority;
-		int new_priority = p.priority(this);
+		int const new_priority = p.priority(this);
+
+		if (prev_priority != new_priority && !m_dirty)
+		{
+			if (prev_priority == -1) add(index);
+			else update(prev_priority, p.index);
+		}
 
 		if (p.downloading())
 		{
@@ -1790,17 +1797,6 @@ namespace libtorrent
 				update_piece_state(i);
 		}
 
-		if (prev_priority == new_priority) return ret;
-
-		if (m_dirty) return ret;
-		if (prev_priority == -1)
-		{
-			add(index);
-		}
-		else
-		{
-			update(prev_priority, p.index);
-		}
 		return ret;
 	}
 
@@ -2132,8 +2128,8 @@ namespace libtorrent
 			{
 				for (int i = int(m_priority_boundaries.size()) - 1; i >= 0; --i)
 				{
-					int start = (i == 0) ? 0 : m_priority_boundaries[i - 1];
+					int const start = (i == 0) ? 0 : m_priority_boundaries[i - 1];
-					int end = m_priority_boundaries[i];
+					int const end = m_priority_boundaries[i];
 					for (int p = end - 1; p >= start; --p)
 					{
 						pc.inc_stats_counter(counters::piece_picker_reverse_rare_loops);
@@ -2901,9 +2897,9 @@ get_out:
 		std::cerr << "[" << this << "] " << "update_piece_state(" << dp->index << ")" << std::endl;
 #endif
 
-		int num_blocks = blocks_in_piece(dp->index);
+		int const num_blocks = blocks_in_piece(dp->index);
 		piece_pos& p = m_piece_map[dp->index];
-		int current_state = p.download_state;
+		int const current_state = p.download_state;
 		TORRENT_ASSERT(current_state != piece_pos::piece_open);
 		if (current_state == piece_pos::piece_open)
 			return dp;
@@ -2952,7 +2948,9 @@ get_out:
 		downloading_piece dp_info = *dp;
 		m_downloads[p.download_queue()].erase(dp);
 
-		int prio = p.priority(this);
+		int const prio = p.priority(this);
+		TORRENT_ASSERT(prio < int(m_priority_boundaries.size())
+			|| m_dirty);
 		p.download_state = new_state;
 #ifdef TORRENT_PICKER_LOG
 		std::cerr << "[" << this << "] " << " " << dp_info.index << " state (" << current_state << " -> " << new_state << ")" << std::endl;
@@ -2977,29 +2975,7 @@ get_out:
 
 		return i;
 	}
-/*
-	int piece_picker::get_block_state(piece_block block) const
-	{
-		TORRENT_ASSERT(block.block_index != piece_block::invalid.block_index);
-		TORRENT_ASSERT(block.piece_index != piece_block::invalid.piece_index);
-		TORRENT_ASSERT(block.piece_index < m_piece_map.size());
 
-		// if we have the piece, the block state is considered finished
-		if (m_piece_map[block.piece_index].index == piece_pos::we_have_index)
-			return block_info::state_finished;
-
-		int state = m_piece_map[block.piece_index].download_queue();
-		if (state == piece_pos::piece_open) return block_info::state_none;
-		std::vector<downloading_piece>::const_iterator i = find_dl_piece(state
-			, block.piece_index);
-
-		TORRENT_ASSERT(i != m_downloads[state].end());
-
-		block_info const* info = blocks_for_piece(*i);
-		TORRENT_ASSERT(info[block.block_index].piece_index == block.piece_index);
-		return info[block.block_index].state;
-	}
-*/
 	bool piece_picker::is_requested(piece_block block) const
 	{
 		TORRENT_ASSERT(block.block_index != piece_block::invalid.block_index);
@@ -3056,6 +3032,7 @@ get_out:
 	}
 
 	// options may be 0 or piece_picker::reverse
+	// returns false if the block could not be marked as downloading
 	bool piece_picker::mark_as_downloading(piece_block block
 		, torrent_peer* peer, int options)
 	{
@@ -3077,7 +3054,7 @@ get_out:
 #ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS
 			INVARIANT_CHECK;
 #endif
-			int prio = p.priority(this);
+			int const prio = p.priority(this);
 			TORRENT_ASSERT(prio < int(m_priority_boundaries.size())
 				|| m_dirty);
 
@@ -3226,7 +3203,7 @@ get_out:
 			// if we already have this piece, just ignore this
 			if (have_piece(block.piece_index)) return false;
 
-			int prio = p.priority(this);
+			int const prio = p.priority(this);
 			TORRENT_ASSERT(prio < int(m_priority_boundaries.size())
 				|| m_dirty);
 			p.download_state = piece_pos::piece_downloading;
@@ -3378,9 +3355,9 @@ get_out:
 		if (i->finished + i->writing + i->requested == 0)
 		{
 			piece_pos& p = m_piece_map[block.piece_index];
-			int prev_priority = p.priority(this);
+			int const prev_priority = p.priority(this);
 			erase_download_piece(i);
-			int new_priority = p.priority(this);
+			int const new_priority = p.priority(this);
 
 			if (m_dirty) return;
 			if (new_priority == prev_priority) return;
@@ -3432,9 +3409,9 @@ get_out:
 
 			if (i->finished + i->writing + i->requested == 0)
 			{
-				int prev_priority = p.priority(this);
+				int const prev_priority = p.priority(this);
 				erase_download_piece(i);
-				int new_priority = p.priority(this);
+				int const new_priority = p.priority(this);
 
 				if (m_dirty) return;
 				if (new_priority == prev_priority) return;
@@ -3480,7 +3457,7 @@ get_out:
 			INVARIANT_CHECK;
 #endif
 
-			int prio = p.priority(this);
+			int const prio = p.priority(this);
 			TORRENT_ASSERT(prio < int(m_priority_boundaries.size())
 				|| m_dirty);
 			p.download_state = piece_pos::piece_downloading;
@@ -3549,37 +3526,15 @@ get_out:
 #if TORRENT_USE_INVARIANT_CHECKS
 		check_piece_state();
 #endif
-
 	}
 
-/*
-	void piece_picker::mark_as_checking(int index)
-	{
-		int state = m_piece_map[index].download_queue();
-		if (state == piece_pos::piece_open) return;
-		std::vector<downloading_piece>::iterator i = find_dl_piece(state, index);
-		if (i == m_downloads[state].end()) return;
-		TORRENT_ASSERT(i->outstanding_hash_check == false);
-		i->outstanding_hash_check = true;
-	}
-
-	void piece_picker::mark_as_done_checking(int index)
-	{
-		int state = m_piece_map[index].download_queue();
-		if (state == piece_pos::piece_open) return;
-		std::vector<downloading_piece>::iterator i = find_dl_piece(state, index);
-		if (i == m_downloads[state].end()) return;
-		i->outstanding_hash_check = false;
-	}
-*/
-
 	void piece_picker::get_downloaders(std::vector<torrent_peer*>& d, int index) const
 	{
 		TORRENT_ASSERT(index >= 0 && index <= int(m_piece_map.size()));
 
 		d.clear();
-		int state = m_piece_map[index].download_queue();
+		int const state = m_piece_map[index].download_queue();
-		const int num_blocks = blocks_in_piece(index);
+		int const num_blocks = blocks_in_piece(index);
 		d.reserve(num_blocks);
 
 		if (state == piece_pos::piece_open)
@@ -3602,7 +3557,7 @@ get_out:
 
 	torrent_peer* piece_picker::get_downloader(piece_block block) const
 	{
-		int state = m_piece_map[block.piece_index].download_queue();
+		int const state = m_piece_map[block.piece_index].download_queue();
 		if (state == piece_pos::piece_open) return nullptr;
 
 		std::vector<downloading_piece>::const_iterator i = find_dl_piece(state
@@ -3685,7 +3640,7 @@ get_out:
 			TORRENT_ASSERT(prev_prio < int(m_priority_boundaries.size())
 				|| m_dirty);
 			erase_download_piece(i);
-			int prio = p.priority(this);
+			int const prio = p.priority(this);
 			if (!m_dirty)
 			{
 				if (prev_prio == -1 && prio >= 0) add(block.piece_index);

test/test.hpp

@@ -117,6 +117,12 @@ extern int EXPORT _g_test_failures;
 		s__ << "TEST_ERROR: equal check failed:\n" #x ": " << (x) << "\nexpected: " << (y); \
 		TEST_REPORT_AUX(s__.str().c_str(), __FILE__, __LINE__); \
 	}
+#define TEST_NE(x, y) \
+	if ((x) == (y)) { \
+		std::stringstream s__; \
+		s__ << "TEST_ERROR: not equal check failed:\n" #x ": " << (x) << "\nexpected not equal to: " << (y); \
+		TEST_REPORT_AUX(s__.str().c_str(), __FILE__, __LINE__); \
+	}
 #else
 #define TEST_CHECK(x) \
 	try \
@@ -149,6 +155,22 @@ extern int EXPORT _g_test_failures;
 	{ \
 		TEST_ERROR("TEST_ERROR: Exception thrown: " #x); \
 	}
+#define TEST_NE(x, y) \
+	try { \
+		if ((x) == (y)) { \
+			std::stringstream s__; \
+			s__ << "TEST_ERROR: " #x ": " << (x) << " expected not equal to: " << (y); \
+			TEST_REPORT_AUX(s__.str().c_str(), __FILE__, __LINE__); \
+		} \
+	} \
+	catch (std::exception& e) \
+	{ \
+		TEST_ERROR("TEST_ERROR: Exception thrown: " #x " :" + std::string(e.what())); \
+	} \
+	catch (...) \
+	{ \
+		TEST_ERROR("TEST_ERROR: Exception thrown: " #x); \
+	}
 #endif
 
 #define TEST_ERROR(x) \

test/test_piece_picker.cpp

@@ -293,7 +293,7 @@ int test_pick(boost::shared_ptr<piece_picker> const& p
 	const std::vector<int> empty_vector;
 	std::vector<piece_block> picked = pick_pieces(p, "*******", 1, 0, nullptr
 		, options, empty_vector);
-	if (picked.empty()) return -1;
+	if (picked.size() != 1) return -1;
 	return picked[0].piece_index;
 }
 
@@ -1870,5 +1870,118 @@ TORRENT_TEST(time_critical_mode)
 		TEST_EQUAL(picked[0].piece_index, 4);
 }
 
+TORRENT_TEST(reprioritize_downloading)
+{
+	auto p = setup_picker("1111111", "       ", "", "");
+	bool ret;
+
+	ret = p->mark_as_downloading(piece_block(0, 0), tmp_peer);
+	TEST_EQUAL(ret, true);
+	p->mark_as_finished(piece_block(0, 1), tmp_peer);
+	ret = p->mark_as_writing(piece_block(0, 2), tmp_peer);
+	TEST_EQUAL(ret, true);
+
+	// make sure we pick the partial piece (i.e. piece 0)
+	TEST_EQUAL(test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials), 0);
+
+	// set the priority of the piece to 0 (while downloading it)
+	ret = p->set_piece_priority(0, 0);
+	TEST_EQUAL(ret, true);
+
+	// make sure we _DON'T_ pick the partial piece, since it has priority zero
+	int const picked_piece = test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials);
+	TEST_NE(picked_piece, -1);
+	TEST_NE(picked_piece, 0);
+
+	// set the priority of the piece back to 1. It should now be the best pick
+	// again (since it's partial)
+	ret = p->set_piece_priority(0, 1);
+	TEST_EQUAL(ret, true);
+
+	// make sure we pick the partial piece
+	TEST_EQUAL(test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials), 0);
+}
+
+TORRENT_TEST(reprioritize_fully_downloading)
+{
+	auto p = setup_picker("1111111", "       ", "", "");
+	bool ret;
+
+	for (int i = 0; i < blocks_per_piece; ++i)
+	{
+		ret = p->mark_as_downloading(piece_block(0, i), tmp_peer);
+		TEST_EQUAL(ret, true);
+	}
+
+	// make sure we _DON'T_ pick the downloading piece
+	{
+		int const picked_piece = test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials);
+		TEST_NE(picked_piece, -1);
+		TEST_NE(picked_piece, 0);
+	}
+
+	// set the priority of the piece to 0 (while downloading it)
+	ret = p->set_piece_priority(0, 0);
+	TEST_EQUAL(ret, true);
+
+	// make sure we still _DON'T_ pick the downloading piece
+	{
+		int const picked_piece = test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials);
+		TEST_NE(picked_piece, -1);
+		TEST_NE(picked_piece, 0);
+	}
+
+	// set the priority of the piece back to 1. It should now be the best pick
+	// again (since it's partial)
+	ret = p->set_piece_priority(0, 1);
+	TEST_EQUAL(ret, true);
+
+	// make sure we still _DON'T_ pick the downloading piece
+	{
+		int const picked_piece = test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials);
+		TEST_NE(picked_piece, -1);
+		TEST_NE(picked_piece, 0);
+	}
+}
+
+TORRENT_TEST(download_filtered_piece)
+{
+	auto p = setup_picker("1111111", "       ", "", "");
+	bool ret;
+
+	// set the priority of the piece to 0
+	ret = p->set_piece_priority(0, 0);
+	TEST_EQUAL(ret, true);
+
+	// make sure we _DON'T_ pick piece 0
+	{
+		int const picked_piece = test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials);
+		TEST_NE(picked_piece, -1);
+		TEST_NE(picked_piece, 0);
+	}
+
+	// then mark it for downloading
+	ret = p->mark_as_downloading(piece_block(0, 0), tmp_peer);
+	TEST_EQUAL(ret, true);
+	p->mark_as_finished(piece_block(0, 1), tmp_peer);
+	ret = p->mark_as_writing(piece_block(0, 2), tmp_peer);
+	TEST_EQUAL(ret, true);
+
+	{
+		// we still should not pick it
+		int const picked_piece = test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials);
+		TEST_NE(picked_piece, -1);
+		TEST_NE(picked_piece, 0);
+	}
+
+	// set the priority of the piece back to 1. It should now be the best pick
+	// again (since it's partial)
+	ret = p->set_piece_priority(0, 1);
+	TEST_EQUAL(ret, true);
+
+	// make sure we pick piece 0
+	TEST_EQUAL(test_pick(p, piece_picker::rarest_first | piece_picker::prioritize_partials), 0);
+}
+
 //TODO: 2 test picking with partial pieces and other peers present so that both backup_pieces and backup_pieces2 are used