diff --git a/ChangeLog b/ChangeLog index 1b2d97333..8893b4fde 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ + * improve file layout optimization when creating torrents with padfiles * remove remote_dl_rate feature * source code migration from boost::shared_ptr to std::shared_ptr * storage_interface API changed to use span and references diff --git a/src/file_storage.cpp b/src/file_storage.cpp index 1cc75a029..0dde7d7d7 100644 --- a/src/file_storage.cpp +++ b/src/file_storage.cpp @@ -96,18 +96,6 @@ namespace libtorrent namespace { - bool compare_string(char const* str, int len, std::string const& str2) - { - if (str2.size() != len) return false; - return memcmp(str2.c_str(), str, len) == 0; - } - - bool compare_file_entry_size(internal_file_entry const& fe1 - , internal_file_entry const& fe2) - { - return fe1.size < fe2.size; - } - bool compare_file_offset(internal_file_entry const& lhs , internal_file_entry const& rhs) { @@ -156,7 +144,7 @@ namespace libtorrent && std::memcmp(branch_path, m_name.c_str(), m_name.size()) == 0) { // the +1 is to skip the trailing '/' (or '\') - int offset = int(m_name.size()) + int const offset = int(m_name.size()) + (m_name.size() == branch_len?0:1); branch_path += offset; branch_len -= offset; @@ -168,9 +156,12 @@ namespace libtorrent } // do we already have this path in the path list? - std::vector::reverse_iterator p - = std::find_if(m_paths.rbegin(), m_paths.rend() - , std::bind(&compare_string, branch_path, branch_len, _1)); + auto p = std::find_if(m_paths.rbegin(), m_paths.rend() + , [&] (std::string const& str) + { + if (str.size() != branch_len) return false; + return memcmp(str.c_str(), branch_path, branch_len) == 0; + }); if (p == m_paths.rend()) { @@ -403,7 +394,7 @@ namespace libtorrent target.offset = offset; TORRENT_ASSERT(!compare_file_offset(target, m_files.front())); - std::vector::const_iterator file_iter = std::upper_bound( + auto file_iter = std::upper_bound( begin_deprecated(), end_deprecated(), target, compare_file_offset); TORRENT_ASSERT(file_iter != begin_deprecated()); @@ -424,7 +415,7 @@ namespace libtorrent target.offset = offset; TORRENT_ASSERT(!compare_file_offset(target, m_files.front())); - std::vector::const_iterator file_iter = std::upper_bound( + auto file_iter = std::upper_bound( m_files.begin(), m_files.end(), target, compare_file_offset); TORRENT_ASSERT(file_iter != m_files.begin()); @@ -463,7 +454,7 @@ namespace libtorrent if (std::int64_t(target.offset + size) > m_total_size) size = m_total_size - target.offset; - std::vector::const_iterator file_iter = std::upper_bound( + auto file_iter = std::upper_bound( m_files.begin(), m_files.end(), target, compare_file_offset); TORRENT_ASSERT(file_iter != m_files.begin()); @@ -969,12 +960,14 @@ namespace libtorrent #endif // TORRENT_DEPRECATED } - void file_storage::optimize(int pad_file_limit, int alignment - , bool tail_padding) + void file_storage::optimize(int const pad_file_limit, int alignment + , bool const tail_padding) { if (alignment == -1) alignment = m_piece_length; + // TODO: padfiles should be removed + std::int64_t off = 0; int padding_file = 0; for (std::vector::iterator i = m_files.begin(); @@ -983,10 +976,24 @@ namespace libtorrent if ((off % alignment) == 0) { // this file position is aligned, pick the largest - // available file to put here - std::vector::iterator best_match - = std::max_element(i, m_files.end() - , &compare_file_entry_size); + // available file to put here. If we encounter a file whose size is + // divisible by `alignment`, we pick that immediately, since that + // will not affect whether we're at an aligned position and will + // improve packing of files + std::vector::iterator best_match = i; + for (auto k = i; k != m_files.end(); ++k) + { + // a file whose size fits the alignment always takes priority, + // since it will let us keep placing aligned files + if ((k->size % alignment) == 0) + { + best_match = k; + break; + } + // otherwise, pick the largest file, to have as many bytes be + // aligned. + if (best_match->size < k->size) best_match = k; + } if (best_match != i) { @@ -1062,14 +1069,15 @@ namespace libtorrent // skip the file we just put in place, so we put the pad // file after it ++i; - if (i == m_files.end()) break; // tail-padding is enabled, and the offset after this file is not - // aligned and it's not the last file. The last file must be padded - // too, in order to match an equivalent tail-padded file. + // aligned. The last file must be padded too, in order to match an + // equivalent tail-padded file. add_pad_file(alignment - (off % alignment), i, off, padding_file); TORRENT_ASSERT((off % alignment) == 0); + + if (i == m_files.end()) break; } } m_total_size = off; @@ -1080,8 +1088,8 @@ namespace libtorrent , std::int64_t& offset , int& pad_file_counter) { - int cur_index = i - m_files.begin(); - int index = int(m_files.size()); + int const cur_index = i - m_files.begin(); + int const index = int(m_files.size()); m_files.push_back(internal_file_entry()); ++m_num_files; internal_file_entry& e = m_files.back(); @@ -1104,7 +1112,7 @@ namespace libtorrent if (!m_file_base.empty()) m_file_base.resize(index + 1, 0); #endif - reorder_file(index, cur_index); + if (index != cur_index) reorder_file(index, cur_index); } void file_storage::unload() diff --git a/test/test_file_storage.cpp b/test/test_file_storage.cpp index 818617a1b..78e11baa6 100644 --- a/test/test_file_storage.cpp +++ b/test/test_file_storage.cpp @@ -211,7 +211,99 @@ TORRENT_TEST(file_path_hash) TEST_EQUAL(file_hash0, file_hash1); } -// TODO: test file_storage::optimize +// make sure that files whose size is a multiple by the alignment are picked +// first, since that lets us keep placing files aligned +TORRENT_TEST(optimize_aligned_sizes) +{ + file_storage fs; + fs.set_piece_length(512); + fs.add_file(combine_path("s", "1"), 1); + fs.add_file(combine_path("s", "2"), 40000); + fs.add_file(combine_path("s", "3"), 1024); + + fs.optimize(512, 512, false); + + // since the size of file 3 is a multiple of the alignment (512), it should + // be prioritized, to minimize the amount of padding. + // after that, we want to pick the largest file (2), and since file 1 is + // smaller than the pad-file limit (512) we won't pad it. Since tail_padding + // is false, we won't pad the tail of the torrent either + + TEST_EQUAL(fs.num_files(), 3); + + TEST_EQUAL(fs.file_size(0), 1024); + TEST_EQUAL(fs.file_name(0), "3"); + TEST_EQUAL(fs.pad_file_at(0), false); + + TEST_EQUAL(fs.file_size(1), 40000); + TEST_EQUAL(fs.file_name(1), "2"); + TEST_EQUAL(fs.pad_file_at(1), false); + + TEST_EQUAL(fs.file_size(2), 1); + TEST_EQUAL(fs.file_name(2), "1"); + TEST_EQUAL(fs.pad_file_at(2), false); +} + +// make sure we pad the end of the torrent when tail_padding is specified +TORRENT_TEST(optimize_tail_padding) +{ + file_storage fs; + fs.set_piece_length(512); + fs.add_file(combine_path("s", "1"), 700); + + fs.optimize(512, 512, true); + + // since the size of file 3 is a multiple of the alignment (512), it should + // be prioritized, to minimize the amount of padding. + // after that, we want to pick the largest file (2), and since file 1 is + // smaller than the pad-file limit (512) we won't pad it. Since tail_padding + // is false, we won't pad the tail of the torrent either + + TEST_EQUAL(fs.num_files(), 2); + + TEST_EQUAL(fs.file_size(0), 700); + TEST_EQUAL(fs.file_name(0), "1"); + TEST_EQUAL(fs.pad_file_at(0), false); + + TEST_EQUAL(fs.file_size(1), 1024 - 700); + TEST_EQUAL(fs.pad_file_at(1), true); +} + + +// make sure we fill in padding with small files +TORRENT_TEST(optimize_pad_fillers) +{ + file_storage fs; + fs.set_piece_length(512); + fs.add_file(combine_path("s", "1"), 1); + fs.add_file(combine_path("s", "2"), 1000); + fs.add_file(combine_path("s", "3"), 1001); + + fs.optimize(512, 512, false); + + // first we pick the largest file, then we need to add padding, since file 1 + // is smaller than the pad file limit, it won't be aligned anyway, so we + // place that as part of the padding + + TEST_EQUAL(fs.num_files(), 4); + + TEST_EQUAL(fs.file_size(0), 1001); + TEST_EQUAL(fs.file_name(0), "3"); + TEST_EQUAL(fs.pad_file_at(0), false); + + TEST_EQUAL(fs.file_size(1), 1); + TEST_EQUAL(fs.file_name(1), "1"); + TEST_EQUAL(fs.pad_file_at(1), false); + + TEST_EQUAL(fs.file_size(2), 1024 - (1001 + 1)); + TEST_EQUAL(fs.pad_file_at(2), true); + + TEST_EQUAL(fs.file_size(3), 1000); + TEST_EQUAL(fs.file_name(3), "2"); + TEST_EQUAL(fs.pad_file_at(3), false); +} + +// TODO: add more optimize() tests // TODO: test map_block // TODO: test piece_size(int piece) // TODO: test file_index_at_offset