Merge pull request #10463 from liamwhite/this-is-why-we-need-g

vfs_concat: fix time complexity of read

src/core/core.cpp

@@ -117,8 +117,7 @@ FileSys::VirtualFile GetGameFileFromPath(const FileSys::VirtualFilesystem& vfs,
             return nullptr;
         }
 
-        return FileSys::ConcatenatedVfsFile::MakeConcatenatedFile(std::move(concat),
+        return FileSys::ConcatenatedVfsFile::MakeConcatenatedFile(concat, dir->GetName());
-                                                                  dir->GetName());
     }
 
     if (Common::FS::IsDir(path)) {

src/core/file_sys/romfs.cpp

@@ -140,7 +140,8 @@ VirtualFile CreateRomFS(VirtualDir dir, VirtualDir ext) {
         return nullptr;
 
     RomFSBuildContext ctx{dir, ext};
-    return ConcatenatedVfsFile::MakeConcatenatedFile(0, ctx.Build(), dir->GetName());
+    auto file_map = ctx.Build();
+    return ConcatenatedVfsFile::MakeConcatenatedFile(0, file_map, dir->GetName());
 }
 
 } // namespace FileSys

src/core/file_sys/vfs_concat.cpp

@@ -10,84 +10,105 @@
 
 namespace FileSys {
 
-static bool VerifyConcatenationMapContinuity(const std::multimap<u64, VirtualFile>& map) {
+ConcatenatedVfsFile::ConcatenatedVfsFile(ConcatenationMap&& concatenation_map_, std::string&& name_)
-    const auto last_valid = --map.end();
+    : concatenation_map(std::move(concatenation_map_)), name(std::move(name_)) {
-    for (auto iter = map.begin(); iter != last_valid;) {
+    DEBUG_ASSERT(this->VerifyContinuity());
-        const auto old = iter++;
+}
-        if (old->first + old->second->GetSize() != iter->first) {
+
+bool ConcatenatedVfsFile::VerifyContinuity() const {
+    u64 last_offset = 0;
+    for (auto& entry : concatenation_map) {
+        if (entry.offset != last_offset) {
             return false;
         }
+
+        last_offset = entry.offset + entry.file->GetSize();
     }
 
-    return map.begin()->first == 0;
+    return true;
-}
-
-ConcatenatedVfsFile::ConcatenatedVfsFile(std::vector<VirtualFile> files_, std::string name_)
-    : name(std::move(name_)) {
-    std::size_t next_offset = 0;
-    for (const auto& file : files_) {
-        files.emplace(next_offset, file);
-        next_offset += file->GetSize();
-    }
-}
-
-ConcatenatedVfsFile::ConcatenatedVfsFile(std::multimap<u64, VirtualFile> files_, std::string name_)
-    : files(std::move(files_)), name(std::move(name_)) {
-    ASSERT(VerifyConcatenationMapContinuity(files));
 }
 
 ConcatenatedVfsFile::~ConcatenatedVfsFile() = default;
 
-VirtualFile ConcatenatedVfsFile::MakeConcatenatedFile(std::vector<VirtualFile> files,
+VirtualFile ConcatenatedVfsFile::MakeConcatenatedFile(const std::vector<VirtualFile>& files,
-                                                      std::string name) {
+                                                      std::string&& name) {
-    if (files.empty())
+    // Fold trivial cases.
+    if (files.empty()) {
         return nullptr;
-    if (files.size() == 1)
+    }
-        return files[0];
+    if (files.size() == 1) {
+        return files.front();
+    }
 
-    return VirtualFile(new ConcatenatedVfsFile(std::move(files), std::move(name)));
+    // Make the concatenation map from the input.
+    std::vector<ConcatenationEntry> concatenation_map;
+    concatenation_map.reserve(files.size());
+    u64 last_offset = 0;
+
+    for (auto& file : files) {
+        concatenation_map.emplace_back(ConcatenationEntry{
+            .offset = last_offset,
+            .file = file,
+        });
+
+        last_offset += file->GetSize();
+    }
+
+    return VirtualFile(new ConcatenatedVfsFile(std::move(concatenation_map), std::move(name)));
 }
 
 VirtualFile ConcatenatedVfsFile::MakeConcatenatedFile(u8 filler_byte,
-                                                      std::multimap<u64, VirtualFile> files,
+                                                      const std::multimap<u64, VirtualFile>& files,
-                                                      std::string name) {
+                                                      std::string&& name) {
-    if (files.empty())
+    // Fold trivial cases.
+    if (files.empty()) {
         return nullptr;
-    if (files.size() == 1)
+    }
+    if (files.size() == 1) {
         return files.begin()->second;
-
-    const auto last_valid = --files.end();
-    for (auto iter = files.begin(); iter != last_valid;) {
-        const auto old = iter++;
-        if (old->first + old->second->GetSize() != iter->first) {
-            files.emplace(old->first + old->second->GetSize(),
-                          std::make_shared<StaticVfsFile>(filler_byte, iter->first - old->first -
-                                                                           old->second->GetSize()));
-        }
     }
 
-    // Ensure the map starts at offset 0 (start of file), otherwise pad to fill.
+    // Make the concatenation map from the input.
-    if (files.begin()->first != 0)
+    std::vector<ConcatenationEntry> concatenation_map;
-        files.emplace(0, std::make_shared<StaticVfsFile>(filler_byte, files.begin()->first));
 
-    return VirtualFile(new ConcatenatedVfsFile(std::move(files), std::move(name)));
+    concatenation_map.reserve(files.size());
+    u64 last_offset = 0;
+
+    // Iteration of a multimap is ordered, so offset will be strictly non-decreasing.
+    for (auto& [offset, file] : files) {
+        if (offset > last_offset) {
+            concatenation_map.emplace_back(ConcatenationEntry{
+                .offset = last_offset,
+                .file = std::make_shared<StaticVfsFile>(filler_byte, offset - last_offset),
+            });
+        }
+
+        concatenation_map.emplace_back(ConcatenationEntry{
+            .offset = offset,
+            .file = file,
+        });
+
+        last_offset = offset + file->GetSize();
+    }
+
+    return VirtualFile(new ConcatenatedVfsFile(std::move(concatenation_map), std::move(name)));
 }
 
 std::string ConcatenatedVfsFile::GetName() const {
-    if (files.empty()) {
+    if (concatenation_map.empty()) {
         return "";
     }
     if (!name.empty()) {
         return name;
     }
-    return files.begin()->second->GetName();
+    return concatenation_map.front().file->GetName();
 }
 
 std::size_t ConcatenatedVfsFile::GetSize() const {
-    if (files.empty()) {
+    if (concatenation_map.empty()) {
         return 0;
     }
-    return files.rbegin()->first + files.rbegin()->second->GetSize();
+    return concatenation_map.back().offset + concatenation_map.back().file->GetSize();
 }
 
 bool ConcatenatedVfsFile::Resize(std::size_t new_size) {
@@ -95,10 +116,10 @@ bool ConcatenatedVfsFile::Resize(std::size_t new_size) {
 }
 
 VirtualDir ConcatenatedVfsFile::GetContainingDirectory() const {
-    if (files.empty()) {
+    if (concatenation_map.empty()) {
         return nullptr;
     }
-    return files.begin()->second->GetContainingDirectory();
+    return concatenation_map.front().file->GetContainingDirectory();
 }
 
 bool ConcatenatedVfsFile::IsWritable() const {
@@ -110,25 +131,45 @@ bool ConcatenatedVfsFile::IsReadable() const {
 }
 
 std::size_t ConcatenatedVfsFile::Read(u8* data, std::size_t length, std::size_t offset) const {
-    auto entry = --files.end();
+    const ConcatenationEntry key{
-    for (auto iter = files.begin(); iter != files.end(); ++iter) {
+        .offset = offset,
-        if (iter->first > offset) {
+        .file = nullptr,
-            entry = --iter;
+    };
+
+    // Read nothing if the map is empty.
+    if (concatenation_map.empty()) {
+        return 0;
+    }
+
+    // Binary search to find the iterator to the first position we can check.
+    // It must exist, since we are not empty and are comparing unsigned integers.
+    auto it = std::prev(std::upper_bound(concatenation_map.begin(), concatenation_map.end(), key));
+    u64 cur_length = length;
+    u64 cur_offset = offset;
+
+    while (cur_length > 0 && it != concatenation_map.end()) {
+        // Check if we can read the file at this position.
+        const auto& file = it->file;
+        const u64 file_offset = it->offset;
+        const u64 file_size = file->GetSize();
+
+        if (cur_offset >= file_offset + file_size) {
+            // Entirely out of bounds read.
             break;
         }
+
+        // Read the file at this position.
+        const u64 intended_read_size = std::min<u64>(cur_length, file_size);
+        const u64 actual_read_size =
+            file->Read(data + (cur_offset - offset), intended_read_size, cur_offset - file_offset);
+
+        // Update tracking.
+        cur_offset += actual_read_size;
+        cur_length -= actual_read_size;
+        it++;
     }
 
-    if (entry->first + entry->second->GetSize() <= offset)
+    return cur_offset - offset;
-        return 0;
-
-    const auto read_in =
-        std::min<u64>(entry->first + entry->second->GetSize() - offset, entry->second->GetSize());
-    if (length > read_in) {
-        return entry->second->Read(data, read_in, offset - entry->first) +
-               Read(data + read_in, length - read_in, offset + read_in);
-    }
-
-    return entry->second->Read(data, std::min<u64>(read_in, length), offset - entry->first);
 }
 
 std::size_t ConcatenatedVfsFile::Write(const u8* data, std::size_t length, std::size_t offset) {

src/core/file_sys/vfs_concat.h

@@ -3,6 +3,7 @@
 
 #pragma once
 
+#include <compare>
 #include <map>
 #include <memory>
 #include "core/file_sys/vfs.h"
@@ -12,19 +13,33 @@ namespace FileSys {
 // Class that wraps multiple vfs files and concatenates them, making reads seamless. Currently
 // read-only.
 class ConcatenatedVfsFile : public VfsFile {
-    explicit ConcatenatedVfsFile(std::vector<VirtualFile> files, std::string name_);
+private:
-    explicit ConcatenatedVfsFile(std::multimap<u64, VirtualFile> files, std::string name_);
+    struct ConcatenationEntry {
+        u64 offset;
+        VirtualFile file;
+
+        auto operator<=>(const ConcatenationEntry& other) const {
+            return this->offset <=> other.offset;
+        }
+    };
+    using ConcatenationMap = std::vector<ConcatenationEntry>;
+
+    explicit ConcatenatedVfsFile(std::vector<ConcatenationEntry>&& concatenation_map,
+                                 std::string&& name);
+    bool VerifyContinuity() const;
 
 public:
     ~ConcatenatedVfsFile() override;
 
     /// Wrapper function to allow for more efficient handling of files.size() == 0, 1 cases.
-    static VirtualFile MakeConcatenatedFile(std::vector<VirtualFile> files, std::string name);
+    static VirtualFile MakeConcatenatedFile(const std::vector<VirtualFile>& files,
+                                            std::string&& name);
 
     /// Convenience function that turns a map of offsets to files into a concatenated file, filling
     /// gaps with a given filler byte.
-    static VirtualFile MakeConcatenatedFile(u8 filler_byte, std::multimap<u64, VirtualFile> files,
+    static VirtualFile MakeConcatenatedFile(u8 filler_byte,
-                                            std::string name);
+                                            const std::multimap<u64, VirtualFile>& files,
+                                            std::string&& name);
 
     std::string GetName() const override;
     std::size_t GetSize() const override;
@@ -37,8 +52,7 @@ public:
     bool Rename(std::string_view new_name) override;
 
 private:
-    // Maps starting offset to file -- more efficient.
+    ConcatenationMap concatenation_map;
-    std::multimap<u64, VirtualFile> files;
     std::string name;
 };