more piece_picker unit tests and some fixes

Jamfile

@@ -28,14 +28,10 @@ VERSION = 1.1.0 ;
 rule coverage ( properties * )
 {
 	local result ;
-	if ! <test-coverage>on
+	if <test-coverage>on in $(properties)
-	{
+		&& ( <toolset>gcc in $(properties)
-		return $(result) ;
-	}
-
-	if <toolset>gcc in $(properties)
 		|| <toolset>darwin in $(properties)
-		|| <toolset>clang in $(properties)
+		|| <toolset>clang in $(properties) )
 	{
 		result += <cxxflags>-fprofile-arcs <cxxflags>-ftest-coverage ;
 

include/libtorrent/piece_picker.hpp

@@ -173,6 +173,8 @@ namespace libtorrent
 			// info about each block
 			// this is a pointer into the m_block_info
 			// vector owned by the piece_picker
+			// TODO: 2 if we could make this an index into m_block_info instead
+			// we would save at least 4 or 5 bytes
 			block_info* info;
 
 			// the index of the piece
@@ -366,8 +368,8 @@ namespace libtorrent
 
 		void piece_passed(int index);
 
-		void mark_as_checking(int index);
+//		void mark_as_checking(int index);
-		void mark_as_done_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;
@@ -405,10 +407,6 @@ namespace libtorrent
 		// the hash-check yet
 		int unverified_blocks() const;
 
-		// return the peer pointers for all blocks that are currently
-		// in requested state (i.e. requested but not received)
-		void get_requestors(std::vector<void*>& d, int index) const;
-
 		// return the peer pointers to all peers that participated in
 		// this piece
 		void get_downloaders(std::vector<void*>& d, int index) const;

src/peer_connection.cpp

@@ -2790,7 +2790,7 @@ namespace libtorrent
 			if (st.requested > 0 && st.writing + st.finished + st.requested == num_blocks)
 			{
 				std::vector<void*> d;
-				t->picker().get_requestors(d, piece);
+				t->picker().get_downloaders(d, piece);
 				if (d.size() == 1)
 				{
 					// only make predictions if all remaining

src/piece_picker.cpp

@@ -3425,7 +3425,9 @@ get_out:
 	void piece_picker::mark_as_canceled(piece_block block, void* peer)
 	{
 #ifdef TORRENT_PICKER_LOG
-		std::cerr << "[" << this << "] " << "mark_as_cancelled( {" << block.piece_index << ", " << block.block_index << "} )" << std::endl;
+		std::cerr << "[" << this << "] " << "mark_as_cancelled( {"
+			<< block.piece_index << ", " << block.block_index
+			<< "} )" << std::endl;
 #endif
 
 #if TORRENT_USE_INVARIANT_CHECKS
@@ -3458,6 +3460,21 @@ get_out:
 		{
 			--i->writing;
 			info.state = block_info::state_none;
+			// i may be invalid after this call
+			i = update_piece_state(i);
+
+			if (i->finished + i->writing + i->requested == 0)
+			{
+				piece_pos& p = m_piece_map[block.piece_index];
+				int prev_priority = p.priority(this);
+				erase_download_piece(i);
+				int new_priority = p.priority(this);
+
+				if (m_dirty) return;
+				if (new_priority == prev_priority) return;
+				if (prev_priority == -1) add(block.piece_index);
+				else update(prev_priority, p.index);
+			}
 		}
 		else
 		{
@@ -3569,48 +3586,26 @@ get_out:
 
 	}
 
-	// TODO: 3 this doesn't appear to be used for anything. Can it be removed
+/*
-	// or should it be used to plug some race when requesting and checking pieces?
 	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_requestors(std::vector<void*>& d, int index) const
-	{
-		TORRENT_ASSERT(index >= 0 && index <= (int)m_piece_map.size());
-
-		int state = m_piece_map[index].download_queue();
-		TORRENT_ASSERT(state != piece_pos::piece_open);
-		std::vector<downloading_piece>::const_iterator i = find_dl_piece(state, index);
-		TORRENT_ASSERT(i != m_downloads[state].end());
-
-		d.clear();
-		for (int j = 0, end(blocks_in_piece(index)); j != end; ++j)
-		{
-			if (i->info[j].state != block_info::state_requested) continue;
-			if (i->info[j].peer == 0) continue;
-			d.push_back(i->info[j].peer);
-		}
-	}
 
 	void piece_picker::get_downloaders(std::vector<void*>& d, int index) const
 	{
@@ -3618,18 +3613,21 @@ get_out:
 
 		d.clear();
 		int state = m_piece_map[index].download_queue();
+		int num_blocks = blocks_in_piece(index);
+		d.reserve(num_blocks);
+
 		if (state == piece_pos::piece_open)
 		{
-			int num_blocks = blocks_in_piece(index);
 			for (int i = 0; i < num_blocks; ++i) d.push_back(0);
 			return;
 		}
 
-		std::vector<downloading_piece>::const_iterator i = find_dl_piece(state, index);
+		std::vector<downloading_piece>::const_iterator i
+			= find_dl_piece(state, index);
 		TORRENT_ASSERT(i != m_downloads[state].end());
 		TORRENT_ASSERT(i->info >= &m_block_info[0]
 			&& i->info < &m_block_info[0] + m_block_info.size());
-		for (int j = 0, end(blocks_in_piece(index)); j != end; ++j)
+		for (int j = 0; j != num_blocks; ++j)
 		{
 			TORRENT_ASSERT(i->info[j].peer == 0 || static_cast<torrent_peer*>(i->info[j].peer)->in_use);
 			d.push_back(i->info[j].peer);

src/torrent.cpp

@@ -3897,7 +3897,7 @@ namespace libtorrent
 
 		TORRENT_ASSERT(!m_picker->have_piece(j->piece));
 
-		picker().mark_as_done_checking(j->piece);
+//		picker().mark_as_done_checking(j->piece);
 
 		state_updated();
 
@@ -10824,7 +10824,7 @@ namespace libtorrent
 	// adds a piece
 	void torrent::verify_piece(int piece)
 	{
-		picker().mark_as_checking(piece);
+//		picker().mark_as_checking(piece);
 
 		inc_refcount("verify_piece");
 		m_ses.disk_thread().async_hash(m_storage.get(), piece, 0

test/test_piece_picker.cpp

@@ -1550,18 +1550,81 @@ int test_main()
 	TEST_EQUAL(p->has_piece_passed(1), false);
 	TEST_EQUAL(p->have_piece(1), false);
 
-// ========================================================
-
 	// make sure write_failed() and lock_piece() actually
 	// locks the piece, and that it won't be picked.
 	// also make sure restore_piece() unlocks it and makes
 	// it available for picking again.
 
-	// test mark_as_canceled()
+	picked = pick_pieces(p, " *     ", 1, blocks_per_piece, 0);
+	TEST_EQUAL(picked.size(), 0);
 
-	// test get_download_queue()
+	p->restore_piece(1);
 
-	// test get_requestors() (similar to get_downloaders())
+	picked = pick_pieces(p, " *     ", 1, blocks_per_piece, 0);
+	TEST_EQUAL(picked.size(), blocks_per_piece);
+
+	// locking pieces only works on partial pieces
+	p->mark_as_writing(piece_block(1, 0), &tmp1);
+	p->lock_piece(1);
+
+	picked = pick_pieces(p, " *     ", 1, blocks_per_piece, 0);
+	TEST_EQUAL(picked.size(), 0);
+
+// ========================================================
+
+	print_title("test write_failed (clear piece)");
+
+	p = setup_picker("1111111", "* *    ", "1101111", "");
+
+	stat = p->piece_stats(1);
+	TEST_EQUAL(stat.downloading, 0);
+
+	p->mark_as_writing(piece_block(1, 0), &tmp1);
+
+	stat = p->piece_stats(1);
+	TEST_EQUAL(stat.downloading, 1);
+
+	p->write_failed(piece_block(1, 0));
+
+	stat = p->piece_stats(1);
+	TEST_EQUAL(stat.downloading, 0);
+
+// ========================================================
+
+	print_title("test mark_as_canceled");
+
+	p = setup_picker("1111111", "* *    ", "1101111", "");
+
+	stat = p->piece_stats(1);
+	TEST_EQUAL(stat.downloading, 0);
+
+	p->mark_as_writing(piece_block(1, 0), &tmp1);
+
+	stat = p->piece_stats(1);
+	TEST_EQUAL(stat.downloading, 1);
+
+	p->mark_as_canceled(piece_block(1, 0), &tmp1);
+	stat = p->piece_stats(1);
+	TEST_EQUAL(stat.downloading, 0);
+
+// ========================================================
+
+	print_title("test get_download_queue");
+
+	p = setup_picker("1111111", "       ", "1101111", "0327000");
+
+	std::vector<piece_picker::downloading_piece> downloads
+		= p->get_download_queue();
+
+	// the download queue should have piece 1, 2 and 3 in it
+	TEST_EQUAL(downloads.size(), 3);
+
+	TEST_CHECK(std::find_if(downloads.begin(), downloads.end()
+		, boost::bind(&piece_picker::downloading_piece::index, _1) == 1) != downloads.end());
+	TEST_CHECK(std::find_if(downloads.begin(), downloads.end()
+		, boost::bind(&piece_picker::downloading_piece::index, _1) == 2) != downloads.end());
+	TEST_CHECK(std::find_if(downloads.begin(), downloads.end()
+		, boost::bind(&piece_picker::downloading_piece::index, _1) == 3) != downloads.end());
 
 /*