From 3c3596dbf22e79a75b51fb6b125b0cda579924cb Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Fri, 23 Jun 2017 17:23:43 +0300 Subject: [PATCH] Make the Loader API thread-safe Since the majority of the code is using ReadAt API already, map this to a `readp` "syscall" which does not mutate any state about the file descriptor therefore making it fairly safe multi-threading wise. This allows to get rid of read-time mutexes in RamCachedFileLoader and therefore fixes #9803 --- Core/FileLoaders/CachingFileLoader.cpp | 7 +------ Core/FileLoaders/CachingFileLoader.h | 8 -------- Core/FileLoaders/DiskCachingFileLoader.cpp | 7 +------ Core/FileLoaders/DiskCachingFileLoader.h | 8 -------- Core/FileLoaders/HTTPFileLoader.cpp | 4 ---- Core/FileLoaders/HTTPFileLoader.h | 7 ------- Core/FileLoaders/LocalFileLoader.cpp | 24 +++------------------- Core/FileLoaders/LocalFileLoader.h | 3 --- Core/FileLoaders/RamCachingFileLoader.cpp | 15 +------------- Core/FileLoaders/RamCachingFileLoader.h | 9 -------- Core/FileLoaders/RetryingFileLoader.cpp | 7 +------ Core/FileLoaders/RetryingFileLoader.h | 8 -------- Core/FileSystems/BlockDevices.cpp | 1 - Core/Loaders.h | 6 ------ 14 files changed, 7 insertions(+), 107 deletions(-) diff --git a/Core/FileLoaders/CachingFileLoader.cpp b/Core/FileLoaders/CachingFileLoader.cpp index b9a43e195..6c9dd206c 100644 --- a/Core/FileLoaders/CachingFileLoader.cpp +++ b/Core/FileLoaders/CachingFileLoader.cpp @@ -25,7 +25,7 @@ // Takes ownership of backend. CachingFileLoader::CachingFileLoader(FileLoader *backend) - : filesize_(0), filepos_(0), backend_(backend), exists_(-1), isDirectory_(-1), aheadThread_(false), prepared_(false) { + : filesize_(0), backend_(backend), exists_(-1), isDirectory_(-1), aheadThread_(false), prepared_(false) { } void CachingFileLoader::Prepare() { @@ -82,10 +82,6 @@ std::string CachingFileLoader::Path() const { return backend_->Path(); } -void CachingFileLoader::Seek(s64 absolutePos) { - filepos_ = absolutePos; -} - size_t CachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags) { Prepare(); if (absolutePos >= filesize_) { @@ -114,7 +110,6 @@ size_t CachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flag StartReadAhead(absolutePos + readSize); } - filepos_ = absolutePos + readSize; return readSize; } diff --git a/Core/FileLoaders/CachingFileLoader.h b/Core/FileLoaders/CachingFileLoader.h index 2cdd6dcbf..025723943 100644 --- a/Core/FileLoaders/CachingFileLoader.h +++ b/Core/FileLoaders/CachingFileLoader.h @@ -34,13 +34,6 @@ public: s64 FileSize() override; std::string Path() const override; - void Seek(s64 absolutePos) override; - size_t Read(size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, count, data, flags); - } - size_t Read(size_t bytes, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, data, flags); - } size_t ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { return ReadAt(absolutePos, bytes * count, data, flags) / bytes; } @@ -65,7 +58,6 @@ private: }; s64 filesize_; - s64 filepos_; FileLoader *backend_; int exists_; int isDirectory_; diff --git a/Core/FileLoaders/DiskCachingFileLoader.cpp b/Core/FileLoaders/DiskCachingFileLoader.cpp index a677bbc8c..912ca4f50 100644 --- a/Core/FileLoaders/DiskCachingFileLoader.cpp +++ b/Core/FileLoaders/DiskCachingFileLoader.cpp @@ -41,7 +41,7 @@ std::mutex DiskCachingFileLoader::cachesMutex_; // Takes ownership of backend. DiskCachingFileLoader::DiskCachingFileLoader(FileLoader *backend) - : prepared_(false), filesize_(0), filepos_(0), backend_(backend), cache_(nullptr) { + : prepared_(false), filesize_(0), backend_(backend), cache_(nullptr) { } void DiskCachingFileLoader::Prepare() { @@ -88,10 +88,6 @@ std::string DiskCachingFileLoader::Path() const { return backend_->Path(); } -void DiskCachingFileLoader::Seek(s64 absolutePos) { - filepos_ = absolutePos; -} - size_t DiskCachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags) { Prepare(); size_t readSize; @@ -119,7 +115,6 @@ size_t DiskCachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, readSize = backend_->ReadAt(absolutePos, bytes, data, flags); } - filepos_ = absolutePos + readSize; return readSize; } diff --git a/Core/FileLoaders/DiskCachingFileLoader.h b/Core/FileLoaders/DiskCachingFileLoader.h index f0d5150ec..2ae91d1f4 100644 --- a/Core/FileLoaders/DiskCachingFileLoader.h +++ b/Core/FileLoaders/DiskCachingFileLoader.h @@ -37,13 +37,6 @@ public: s64 FileSize() override; std::string Path() const override; - void Seek(s64 absolutePos) override; - size_t Read(size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, count, data, flags); - } - size_t Read(size_t bytes, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, data, flags); - } size_t ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { return ReadAt(absolutePos, bytes * count, data, flags) / bytes; } @@ -58,7 +51,6 @@ private: bool prepared_; s64 filesize_; - s64 filepos_; FileLoader *backend_; DiskCachingFileLoaderCache *cache_; diff --git a/Core/FileLoaders/HTTPFileLoader.cpp b/Core/FileLoaders/HTTPFileLoader.cpp index ec699f5f8..ebada0dae 100644 --- a/Core/FileLoaders/HTTPFileLoader.cpp +++ b/Core/FileLoaders/HTTPFileLoader.cpp @@ -115,10 +115,6 @@ std::string HTTPFileLoader::Path() const { return filename_; } -void HTTPFileLoader::Seek(s64 absolutePos) { - filepos_ = absolutePos; -} - size_t HTTPFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags) { Prepare(); s64 absoluteEnd = std::min(absolutePos + (s64)bytes, filesize_); diff --git a/Core/FileLoaders/HTTPFileLoader.h b/Core/FileLoaders/HTTPFileLoader.h index 5e92877a7..6a84554da 100644 --- a/Core/FileLoaders/HTTPFileLoader.h +++ b/Core/FileLoaders/HTTPFileLoader.h @@ -34,13 +34,6 @@ public: virtual s64 FileSize() override; virtual std::string Path() const override; - virtual void Seek(s64 absolutePos) override; - virtual size_t Read(size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, count, data, flags); - } - virtual size_t Read(size_t bytes, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, data, flags); - } virtual size_t ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { return ReadAt(absolutePos, bytes * count, data, flags) / bytes; } diff --git a/Core/FileLoaders/LocalFileLoader.cpp b/Core/FileLoaders/LocalFileLoader.cpp index a6fe9abba..9518acd45 100644 --- a/Core/FileLoaders/LocalFileLoader.cpp +++ b/Core/FileLoaders/LocalFileLoader.cpp @@ -15,14 +15,15 @@ // Official git repository and contact information can be found at // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/. -#include #include "file/file_util.h" #include "Common/FileUtil.h" #include "Core/FileLoaders/LocalFileLoader.h" LocalFileLoader::LocalFileLoader(const std::string &filename) : fd_(0), f_(nullptr), filesize_(0), filename_(filename) { + // FIXME: perhaps at this point we should just use plain open? f_ = File::OpenCFile(filename, "rb"); + fd_ = fileno(f_); if (!f_) { return; } @@ -30,8 +31,6 @@ LocalFileLoader::LocalFileLoader(const std::string &filename) #ifdef __ANDROID__ // Android NDK does not support 64-bit file I/O using C streams // so we fall back onto syscalls - fd_ = fileno(f_); - off64_t off = lseek64(fd_, 0, SEEK_END); filesize_ = off; lseek64(fd_, 0, SEEK_SET); @@ -73,23 +72,6 @@ std::string LocalFileLoader::Path() const { return filename_; } -void LocalFileLoader::Seek(s64 absolutePos) { -#ifdef __ANDROID__ - lseek64(fd_, absolutePos, SEEK_SET); -#else - fseeko(f_, absolutePos, SEEK_SET); -#endif -} - -size_t LocalFileLoader::Read(size_t bytes, size_t count, void *data, Flags flags) { -#ifdef __ANDROID__ - return read(fd_, data, bytes * count) / bytes; -#else - return fread(data, bytes, count, f_); -#endif -} - size_t LocalFileLoader::ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags) { - Seek(absolutePos); - return Read(bytes, count, data); + return pread(fd_, data, bytes * count, absolutePos) / bytes; } diff --git a/Core/FileLoaders/LocalFileLoader.h b/Core/FileLoaders/LocalFileLoader.h index 46b694736..f85b67ecf 100644 --- a/Core/FileLoaders/LocalFileLoader.h +++ b/Core/FileLoaders/LocalFileLoader.h @@ -29,9 +29,6 @@ public: virtual bool IsDirectory() override; virtual s64 FileSize() override; virtual std::string Path() const override; - - virtual void Seek(s64 absolutePos) override; - virtual size_t Read(size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override; virtual size_t ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override; private: diff --git a/Core/FileLoaders/RamCachingFileLoader.cpp b/Core/FileLoaders/RamCachingFileLoader.cpp index 91a3e9d61..1bc01e974 100644 --- a/Core/FileLoaders/RamCachingFileLoader.cpp +++ b/Core/FileLoaders/RamCachingFileLoader.cpp @@ -28,7 +28,7 @@ // Takes ownership of backend. RamCachingFileLoader::RamCachingFileLoader(FileLoader *backend) - : filesize_(0), filepos_(0), backend_(backend), exists_(-1), isDirectory_(-1), aheadThread_(false) { + : filesize_(0), backend_(backend), exists_(-1), isDirectory_(-1), aheadThread_(false) { filesize_ = backend->FileSize(); if (filesize_ > 0) { InitCache(); @@ -45,7 +45,6 @@ RamCachingFileLoader::~RamCachingFileLoader() { bool RamCachingFileLoader::Exists() { if (exists_ == -1) { - std::lock_guard guard(backendMutex_); exists_ = backend_->Exists() ? 1 : 0; } return exists_ == 1; @@ -53,7 +52,6 @@ bool RamCachingFileLoader::Exists() { bool RamCachingFileLoader::ExistsFast() { if (exists_ == -1) { - std::lock_guard guard(backendMutex_); return backend_->ExistsFast(); } return exists_ == 1; @@ -61,7 +59,6 @@ bool RamCachingFileLoader::ExistsFast() { bool RamCachingFileLoader::IsDirectory() { if (isDirectory_ == -1) { - std::lock_guard guard(backendMutex_); isDirectory_ = backend_->IsDirectory() ? 1 : 0; } return isDirectory_ == 1; @@ -72,18 +69,12 @@ s64 RamCachingFileLoader::FileSize() { } std::string RamCachingFileLoader::Path() const { - std::lock_guard guard(backendMutex_); return backend_->Path(); } -void RamCachingFileLoader::Seek(s64 absolutePos) { - filepos_ = absolutePos; -} - size_t RamCachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags) { size_t readSize = 0; if (cache_ == nullptr || (flags & Flags::HINT_UNCACHED) != 0) { - std::lock_guard guard(backendMutex_); readSize = backend_->ReadAt(absolutePos, bytes, data, flags); } else { readSize = ReadFromCache(absolutePos, bytes, data); @@ -100,8 +91,6 @@ size_t RamCachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, F StartReadAhead(absolutePos + readSize); } - - filepos_ = absolutePos + readSize; return readSize; } @@ -195,10 +184,8 @@ void RamCachingFileLoader::SaveIntoCache(s64 pos, size_t bytes, Flags flags) { } } - backendMutex_.lock(); s64 cacheFilePos = cacheStartPos << BLOCK_SHIFT; size_t bytesRead = backend_->ReadAt(cacheFilePos, blocksToRead << BLOCK_SHIFT, &cache_[cacheFilePos], flags); - backendMutex_.unlock(); // In case there was an error, let's not mark blocks that failed to read as read. u32 blocksActuallyRead = (u32)((bytesRead + BLOCK_SIZE - 1) >> BLOCK_SHIFT); diff --git a/Core/FileLoaders/RamCachingFileLoader.h b/Core/FileLoaders/RamCachingFileLoader.h index a1e6743d3..0911b4307 100644 --- a/Core/FileLoaders/RamCachingFileLoader.h +++ b/Core/FileLoaders/RamCachingFileLoader.h @@ -34,13 +34,6 @@ public: s64 FileSize() override; std::string Path() const override; - void Seek(s64 absolutePos) override; - size_t Read(size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, count, data, flags); - } - size_t Read(size_t bytes, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, data, flags); - } size_t ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { return ReadAt(absolutePos, bytes * count, data, flags) / bytes; } @@ -63,7 +56,6 @@ private: }; s64 filesize_; - s64 filepos_; FileLoader *backend_; u8 *cache_; int exists_; @@ -71,7 +63,6 @@ private: std::vector blocks_; std::mutex blocksMutex_; - mutable std::mutex backendMutex_; u32 aheadRemaining_; s64 aheadPos_; bool aheadThread_; diff --git a/Core/FileLoaders/RetryingFileLoader.cpp b/Core/FileLoaders/RetryingFileLoader.cpp index 28d78ff09..b9c7927cc 100644 --- a/Core/FileLoaders/RetryingFileLoader.cpp +++ b/Core/FileLoaders/RetryingFileLoader.cpp @@ -19,7 +19,7 @@ // Takes ownership of backend. RetryingFileLoader::RetryingFileLoader(FileLoader *backend) - : filepos_(0), backend_(backend) { + : backend_(backend) { } RetryingFileLoader::~RetryingFileLoader() { @@ -60,10 +60,6 @@ std::string RetryingFileLoader::Path() const { return backend_->Path(); } -void RetryingFileLoader::Seek(s64 absolutePos) { - filepos_ = absolutePos; -} - size_t RetryingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags) { size_t readSize = backend_->ReadAt(absolutePos, bytes, data, flags); @@ -74,6 +70,5 @@ size_t RetryingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Fla ++retries; } - filepos_ = absolutePos + readSize; return readSize; } diff --git a/Core/FileLoaders/RetryingFileLoader.h b/Core/FileLoaders/RetryingFileLoader.h index 646af1371..98ed64f03 100644 --- a/Core/FileLoaders/RetryingFileLoader.h +++ b/Core/FileLoaders/RetryingFileLoader.h @@ -31,13 +31,6 @@ public: s64 FileSize() override; std::string Path() const override; - void Seek(s64 absolutePos) override; - size_t Read(size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, count, data, flags); - } - size_t Read(size_t bytes, void *data, Flags flags = Flags::NONE) override { - return ReadAt(filepos_, bytes, data, flags); - } size_t ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) override { return ReadAt(absolutePos, bytes * count, data, flags) / bytes; } @@ -48,6 +41,5 @@ private: MAX_RETRIES = 3, }; - s64 filepos_; FileLoader *backend_; }; diff --git a/Core/FileSystems/BlockDevices.cpp b/Core/FileSystems/BlockDevices.cpp index 34b5dca5f..7fc62c75f 100644 --- a/Core/FileSystems/BlockDevices.cpp +++ b/Core/FileSystems/BlockDevices.cpp @@ -38,7 +38,6 @@ BlockDevice *constructBlockDevice(FileLoader *fileLoader) { return nullptr; char buffer[4]{}; size_t size = fileLoader->ReadAt(0, 1, 4, buffer); - fileLoader->Seek(0); if (size == 4 && !memcmp(buffer, "CISO", 4)) return new CISOFileBlockDevice(fileLoader); else if (size == 4 && !memcmp(buffer, "\x00PBP", 4)) diff --git a/Core/Loaders.h b/Core/Loaders.h index faf9f78cc..e7e4d1756 100644 --- a/Core/Loaders.h +++ b/Core/Loaders.h @@ -80,12 +80,6 @@ public: return filename.substr(pos); } } - - virtual void Seek(s64 absolutePos) = 0; - virtual size_t Read(size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) = 0; - virtual size_t Read(size_t bytes, void *data, Flags flags = Flags::NONE) { - return Read(1, bytes, data, flags); - } virtual size_t ReadAt(s64 absolutePos, size_t bytes, size_t count, void *data, Flags flags = Flags::NONE) = 0; virtual size_t ReadAt(s64 absolutePos, size_t bytes, void *data, Flags flags = Flags::NONE) { return ReadAt(absolutePos, 1, bytes, data, flags);