From 4e497e138387f6ba32489e5352bde1f37738adc5 Mon Sep 17 00:00:00 2001 From: arvidn Date: Wed, 9 Aug 2017 14:01:10 +0200 Subject: [PATCH] fix issue where paths were not correctly coalesced when adding files to file_storage (used more memory than necessary) --- include/libtorrent/file_storage.hpp | 2 ++ src/file_storage.cpp | 53 ++++++++++++++++------------- test/test_file_storage.cpp | 30 ++++++++++++++++ 3 files changed, 61 insertions(+), 24 deletions(-) diff --git a/include/libtorrent/file_storage.hpp b/include/libtorrent/file_storage.hpp index cc640991f..d3572303e 100644 --- a/include/libtorrent/file_storage.hpp +++ b/include/libtorrent/file_storage.hpp @@ -583,6 +583,8 @@ namespace libtorrent private: + int get_or_add_path(char const* branch_path, int branch_len); + void add_pad_file(int size , std::vector::iterator& i , boost::int64_t& offset diff --git a/src/file_storage.cpp b/src/file_storage.cpp index 9f1248cbc..765ddcdd7 100644 --- a/src/file_storage.cpp +++ b/src/file_storage.cpp @@ -47,10 +47,8 @@ POSSIBILITY OF SUCH DAMAGE. #if defined(TORRENT_WINDOWS) || defined(TORRENT_OS2) #define TORRENT_SEPARATOR '\\' -#define TORRENT_SEPARATOR_STR "\\" #else #define TORRENT_SEPARATOR '/' -#define TORRENT_SEPARATOR_STR "/" #endif namespace libtorrent @@ -138,13 +136,14 @@ namespace libtorrent return fe1.size < fe2.size; } - bool compare_file_offset(internal_file_entry const& lhs - , internal_file_entry const& rhs) - { - return lhs.offset < rhs.offset; - } + bool compare_file_offset(internal_file_entry const& lhs + , internal_file_entry const& rhs) + { + return lhs.offset < rhs.offset; } +} + // path is not supposed to include the name of the torrent itself. void file_storage::update_path_index(internal_file_entry& e , std::string const& path, bool set_name) @@ -193,6 +192,16 @@ namespace libtorrent e.no_root_dir = true; } + e.path_index = get_or_add_path(branch_path, branch_len); + if (set_name) e.set_name(leaf); + } + + int file_storage::get_or_add_path(char const* branch_path, int branch_len) + { + // trim trailing slashes + if (branch_len > 0 && branch_path[branch_len-1] == TORRENT_SEPARATOR) + --branch_len; + // do we already have this path in the path list? std::vector::reverse_iterator p = std::find_if(m_paths.rbegin(), m_paths.rend() @@ -201,23 +210,16 @@ namespace libtorrent if (p == m_paths.rend()) { // no, we don't. add it - e.path_index = m_paths.size(); - TORRENT_ASSERT(branch_path[0] != '/'); - - // trim trailing slashes - if (branch_len > 0 && branch_path[branch_len-1] == TORRENT_SEPARATOR) - --branch_len; - - // poor man's emplace back - m_paths.resize(m_paths.size() + 1); - m_paths.back().assign(branch_path, branch_len); + int const ret = int(m_paths.size()); + TORRENT_ASSERT(branch_len == 0 || branch_path[0] != TORRENT_SEPARATOR); + m_paths.push_back(std::string(branch_path, branch_len)); + return ret; } else { // yes we do. use it - e.path_index = p.base() - m_paths.begin() - 1; + return int(p.base() - m_paths.begin() - 1); } - if (set_name) e.set_name(leaf); } #ifndef TORRENT_NO_DEPRECATE @@ -396,6 +398,8 @@ namespace libtorrent int file_storage::file_index_at_offset(boost::int64_t offset) const { + TORRENT_ASSERT_PRECOND(offset >= 0); + TORRENT_ASSERT_PRECOND(offset < m_total_size); // find the file iterator and file offset internal_file_entry target; target.offset = offset; @@ -425,6 +429,8 @@ namespace libtorrent , boost::int64_t const offset , int size) const { + TORRENT_ASSERT_PRECOND(piece >= 0); + TORRENT_ASSERT_PRECOND(piece < num_pieces()); TORRENT_ASSERT_PRECOND(num_files() > 0); std::vector ret; @@ -1067,11 +1073,10 @@ namespace libtorrent i = m_files.begin() + cur_index; e.size = size; e.offset = offset; - char name[30]; - snprintf(name, sizeof(name), ".pad" TORRENT_SEPARATOR_STR "%d" - , pad_file_counter); - std::string path = combine_path(m_name, name); - e.set_name(path.c_str()); + e.path_index = get_or_add_path(".pad", 4); + char name[15]; + snprintf(name, sizeof(name), "%d", pad_file_counter); + e.set_name(name); e.pad_file = true; offset += size; ++pad_file_counter; diff --git a/test/test_file_storage.cpp b/test/test_file_storage.cpp index ac8f769d1..127491cc0 100644 --- a/test/test_file_storage.cpp +++ b/test/test_file_storage.cpp @@ -75,6 +75,36 @@ void setup_test_storage(file_storage& st) TEST_EQUAL(st.num_pieces(), (100000 + 0x3fff) / 0x4000); } +TORRENT_TEST(coalesce_path) +{ + file_storage st; + st.add_file(combine_path("test", "a"), 10000); + TEST_EQUAL(st.paths().size(), 1); + TEST_EQUAL(st.paths()[0], ""); + st.add_file(combine_path("test", "b"), 20000); + TEST_EQUAL(st.paths().size(), 1); + TEST_EQUAL(st.paths()[0], ""); + st.add_file(combine_path("test", combine_path("c", "a")), 30000); + TEST_EQUAL(st.paths().size(), 2); + TEST_EQUAL(st.paths()[0], ""); + TEST_EQUAL(st.paths()[1], "c"); + + // make sure that two files with the same path shares the path entry + st.add_file(combine_path("test", combine_path("c", "b")), 40000); + TEST_EQUAL(st.paths().size(), 2); + TEST_EQUAL(st.paths()[0], ""); + TEST_EQUAL(st.paths()[1], "c"); + + // cause pad files to be created, to make sure the pad files also share the + // same path entries + st.optimize(0, 1024, true); + + TEST_EQUAL(st.paths().size(), 3); + TEST_EQUAL(st.paths()[0], ""); + TEST_EQUAL(st.paths()[1], "c"); + TEST_EQUAL(st.paths()[2], ".pad"); +} + TORRENT_TEST(rename_file) { // test rename_file