CachingFileLoader: keep the partial last block of the file

since 7e6405291 only full 64 KB reads get cached, so any read touching the
shorter last block of a file came back short. count blocks with
blocks_.size() too, so failed reads can't inflate the count until
MakeCacheSpaceFor loops forever. also stop read-ahead from asking the
backend for blocks past the end of the file.

Core/FileLoaders/CachingFileLoader.cpp

@@ -100,7 +100,6 @@ size_t CachingFileLoader::ReadAt(s64 absolutePos, size_t bytes, void *data, Flag
 }
 
 void CachingFileLoader::InitCache() {
-	cacheSize_ = 0;
 	oldestGeneration_ = 0;
 	generation_ = 0;
 }
@@ -120,7 +119,6 @@ void CachingFileLoader::ShutdownCache() {
 		delete [] block.second.ptr;
 	}
 	blocks_.clear();
-	cacheSize_ = 0;
 }
 
 size_t CachingFileLoader::ReadFromCache(s64 pos, size_t bytes, void *data) {
@@ -152,6 +150,8 @@ size_t CachingFileLoader::ReadFromCache(s64 pos, size_t bytes, void *data) {
 void CachingFileLoader::SaveIntoCache(s64 pos, size_t bytes, Flags flags, bool readingAhead) {
 	s64 cacheStartPos = pos >> BLOCK_SHIFT;
 	s64 cacheEndPos = (pos + bytes - 1) >> BLOCK_SHIFT;
+	// Read-ahead can ask for blocks past the end of the file.
+	cacheEndPos = std::min(cacheEndPos, (filesize_ - 1) >> BLOCK_SHIFT);
 
 	std::lock_guard<std::recursive_mutex> guard(blocksMutex_);
 	size_t blocksToRead = 0;
@@ -183,8 +183,9 @@ void CachingFileLoader::SaveIntoCache(s64 pos, size_t bytes, Flags flags, bool r
 			// Only cache a block we actually fully read - a short/failed read (e.g. a
 			// dropped connection on a Remote ISO) must not be cached as if valid, or
 			// every later read of this block would silently return the uninitialized
-			// tail of `buf` as if it were real file data.
-			if (readBytes == BLOCK_SIZE) {
+			// tail of `buf` as if it were real file data. The last block of the file
+			// is shorter, and complete if the read reached the end.
+			if (readBytes == BLOCK_SIZE || (readBytes > 0 && (cacheStartPos << BLOCK_SHIFT) + (s64)readBytes == filesize_)) {
 				blocks_[cacheStartPos] = BlockInfo{buf};
 			} else {
 				delete [] buf;
@@ -198,6 +199,10 @@ void CachingFileLoader::SaveIntoCache(s64 pos, size_t bytes, Flags flags, bool r
 		u8 *wholeRead = new u8[blocksToRead << BLOCK_SHIFT];
 		size_t readBytes = backend_->ReadAt(cacheStartPos << BLOCK_SHIFT, blocksToRead << BLOCK_SHIFT, wholeRead, flags);
 		size_t wholeBlocksRead = readBytes >> BLOCK_SHIFT;
+		if ((readBytes & (BLOCK_SIZE - 1)) != 0 && (cacheStartPos << BLOCK_SHIFT) + (s64)readBytes == filesize_) {
+			// The short last block of the file.
+			wholeBlocksRead++;
+		}
 
 		blocksMutex_.lock();
 		for (size_t i = 0; i < wholeBlocksRead; ++i) {
@@ -212,19 +217,18 @@ void CachingFileLoader::SaveIntoCache(s64 pos, size_t bytes, Flags flags, bool r
 		delete[] wholeRead;
 	}
 
-	cacheSize_ += blocksToRead;
 	++generation_;
 }
 
 bool CachingFileLoader::MakeCacheSpaceFor(size_t blocks, bool readingAhead) {
 	size_t goal = MAX_BLOCKS_CACHED - blocks;
 
-	if (readingAhead && cacheSize_ > goal) {
+	if (readingAhead && blocks_.size() > goal) {
 		return false;
 	}
 
 	std::lock_guard<std::recursive_mutex> guard(blocksMutex_);
-	while (cacheSize_ > goal) {
+	while (blocks_.size() > goal) {
 		u64 minGeneration = generation_;
 
 		// We increment the iterator inside because we delete things inside.
@@ -240,10 +244,9 @@ bool CachingFileLoader::MakeCacheSpaceFor(size_t blocks, bool readingAhead) {
 				s64 pos = it->first;
 				delete [] it->second.ptr;
 				blocks_.erase(it);
-				--cacheSize_;
 
 				// Our iterator is invalid now.  Keep going?
-				if (cacheSize_ > goal) {
+				if (blocks_.size() > goal) {
 					// This finds the one at that position.
 					it = blocks_.lower_bound(pos);
 				} else {
@@ -267,7 +270,7 @@ void CachingFileLoader::StartReadAhead(s64 pos) {
 		// Already going.
 		return;
 	}
-	if (cacheSize_ + BLOCK_READAHEAD > MAX_BLOCKS_CACHED) {
+	if (blocks_.size() + BLOCK_READAHEAD > MAX_BLOCKS_CACHED) {
 		// Not enough space to readahead.
 		return;
 	}

Core/FileLoaders/CachingFileLoader.h

@@ -62,7 +62,6 @@ private:
 	int isDirectory_ = -1;
 	u64 generation_ = 0;
 	u64 oldestGeneration_ = 0;
-	size_t cacheSize_ = 0;
 
 	struct BlockInfo {
 		u8 *ptr;

unittest/UnitTest.cpp

@@ -37,9 +37,11 @@
 #include <typeinfo>
 
 #include <algorithm>
+#include <atomic>
 #include <cstdio>
 #include <cstdlib>
 #include <cmath>
+#include <memory>
 #include <vector>
 #include <string>
 #include <sstream>
@@ -103,6 +105,7 @@
 #include "Common/UI/View.h"
 #include "Common/UI/ViewGroup.h"
 #include "Core/Debugger/MemBlockInfo.h"
+#include "Core/FileLoaders/CachingFileLoader.h"
 #include "Core/FileSystems/FileSystem.h"
 #include "Core/FileSystems/ISOFileSystem.h"
 #include "Core/MemMap.h"
@@ -2034,6 +2037,65 @@ bool TestParseLBN() {
 	return true;
 }
 
+// Serves byte i as (u8)(i * 31 + 7), clamped to the file size like HTTPFileLoader.
+// The read counter lives outside, since CachingFileLoader deletes its backend.
+class PatternFileLoader : public FileLoader {
+public:
+	PatternFileLoader(s64 size, std::atomic<int> *reads) : size_(size), reads_(reads) {}
+	bool Exists() override { return true; }
+	bool IsDirectory() override { return false; }
+	s64 FileSize() override { return size_; }
+	Path GetPath() const override { return Path(); }
+	size_t ReadAt(s64 pos, size_t bytes, size_t count, void *data, Flags flags) override {
+		(*reads_)++;
+		s64 end = std::min(pos + (s64)(bytes * count), size_);
+		for (s64 i = pos; i < end; i++) {
+			((u8 *)data)[i - pos] = (u8)(i * 31 + 7);
+		}
+		return pos < end ? (size_t)(end - pos) / bytes : 0;
+	}
+
+private:
+	s64 size_;
+	std::atomic<int> *reads_;
+};
+
+static bool MatchesPattern(const u8 *data, s64 pos, size_t bytes) {
+	for (size_t i = 0; i < bytes; i++) {
+		if (data[i] != (u8)((pos + i) * 31 + 7)) {
+			return false;
+		}
+	}
+	return true;
+}
+
+static bool TestCachingFileLoader() {
+	// The last 64 KB block of the file is short.
+	const s64 size = 3 * 65536 + 1234;
+	std::atomic<int> reads{};
+	std::unique_ptr<CachingFileLoader> loader(new CachingFileLoader(new PatternFileLoader(size, &reads)));
+	std::vector<u8> buf(65536 * 2);
+
+	s64 pos = 3 * 65536 + 100;
+	EXPECT_EQ_INT(loader->ReadAt(pos, 100, buf.data()), 100);
+	EXPECT_TRUE(MatchesPattern(buf.data(), pos, 100));
+	pos = 2 * 65536 + 10;
+	EXPECT_EQ_INT(loader->ReadAt(pos, (size_t)(size - pos), buf.data()), (int)(size - pos));
+	EXPECT_TRUE(MatchesPattern(buf.data(), pos, (size_t)(size - pos)));
+
+	// Both reads ended in the last block, so everything they touched is cached and there's
+	// nothing left to read ahead. Nothing further should reach the backend, even past EOF.
+	int readsBefore = reads;
+	pos = 3 * 65536 + 100;
+	for (int i = 0; i < 100; i++) {
+		EXPECT_EQ_INT(loader->ReadAt(pos, 100, buf.data()), 100);
+	}
+	// Waits for any read-ahead.
+	loader.reset();
+	EXPECT_EQ_INT(reads, readsBefore);
+	return true;
+}
+
 // So we can use EXPECT_TRUE, etc.
 struct AlignedMem {
 	AlignedMem(size_t sz, size_t alignment = 16) {
@@ -3060,6 +3122,7 @@ TestItem availableTests[] = {
 	TEST_ITEM(Jit),
 	TEST_ITEM(VFPUMatrixTranspose),
 	TEST_ITEM(ParseLBN),
+	TEST_ITEM(CachingFileLoader),
 	TEST_ITEM(QuickTexHash),
 	TEST_ITEM(CLZ),
 	TEST_ITEM(MemMap),