Some code cleanups around rewind.

Common/Serialize/Serializer.h

@@ -192,21 +192,8 @@ public:
 		return (size_t)ptr;
 	}
 
-	// Expects ptr to have at least MeasurePtr bytes at ptr.
-	template<class T>
-	static Error SavePtr(u8 *ptr, T &_class, size_t expected_size)
-	{
-		const u8 *expected_end = ptr + expected_size;
-		PointerWrap p(&ptr, PointerWrap::MODE_WRITE);
-		_class.DoState(p);
-
-		if (p.error != PointerWrap::ERROR_FAILURE && (expected_end == ptr || expected_size == 0)) {
-			return ERROR_NONE;
-		} else {
-			return ERROR_BROKEN_STATE;
-		}
-	}
-
+	// If *saved is null, will allocate storage using malloc.
+	// If it's not null, it will be used, but only hope can save you from overruns at the end. For libretro.
 	template<class T>
 	static Error MeasureAndSavePtr(T &_class, u8 **saved, size_t *savedSize)
 	{
@@ -216,9 +203,14 @@ public:
 		_assert_(p.error == PointerWrap::ERROR_NONE);
 
 		size_t measuredSize = p.Offset();
-		u8 *data = (u8 *)malloc(measuredSize);
-		if (!data)
-			return ERROR_BAD_ALLOC;
+		u8 *data;
+		if (*saved) {
+			data = *saved;
+		} else {
+			data = (u8 *)malloc(measuredSize);
+			if (!data)
+				return ERROR_BAD_ALLOC;
+		}
 
 		p.RewindForWrite(data);
 		_class.DoState(p);
@@ -233,6 +225,30 @@ public:
 		}
 	}
 
+	// Duplicate of the above but takes and modifies a vector. Less invasive
+	// than modifying the rewind manager to keep things in something else than vectors.
+	template<class T>
+	static Error MeasureAndSavePtr(T &_class, std::vector<u8> *saved)
+	{
+		u8 *ptr = nullptr;
+		PointerWrap p(&ptr, PointerWrap::MODE_MEASURE);
+		_class.DoState(p);
+		_assert_(p.error == PointerWrap::ERROR_NONE);
+
+		size_t measuredSize = p.Offset();
+		saved->resize(measuredSize);
+		u8 *data = saved->data();
+		p.RewindForWrite(data);
+		_class.DoState(p);
+		if (p.CheckAfterWrite()) {
+			return ERROR_NONE;
+		} else {
+			saved->clear();
+			return ERROR_BROKEN_STATE;
+		}
+	}
+
+
 	// Load file template
 	template<class T>
 	static Error Load(const Path &filename, std::string *gitVersion, T& _class, std::string *failureReason)

Core/SaveState.cpp

@@ -91,10 +91,7 @@ namespace SaveState
 
 	CChunkFileReader::Error SaveToRam(std::vector<u8> &data) {
 		SaveStart state;
-		size_t sz = CChunkFileReader::MeasurePtr(state);
-		if (data.size() < sz)
-			data.resize(sz);
-		return CChunkFileReader::SavePtr(&data[0], state, sz);
+		return CChunkFileReader::MeasureAndSavePtr(state, &data);
 	}
 
 	CChunkFileReader::Error LoadFromRam(std::vector<u8> &data, std::string *errorString) {
@@ -102,14 +99,24 @@ namespace SaveState
 		return CChunkFileReader::LoadPtr(&data[0], state, errorString);
 	}
 
-	struct StateRingbuffer
-	{
-		StateRingbuffer(int size) : first_(0), next_(0), size_(size), base_(-1)
-		{
+	// This ring buffer of states is for rewind save states, which are kept in RAM.
+	// Save states are compressed against one of two reference saves (bases_), and the reference
+	// is switched to a fresh save every N saves, where N is BASE_USAGE_INTERVAL.
+	// The compression is a simple block based scheme where 0 means to copy a block from the base,
+	// and 1 means that the following bytes are the next block. See Compress/LockedDecompress.
+	class StateRingbuffer {
+	public:
+		StateRingbuffer(int size) : size_(size) {
 			states_.resize(size);
 			baseMapping_.resize(size);
 		}
 
+		~StateRingbuffer() {
+			if (compressThread_.joinable()) {
+				compressThread_.join();
+			}
+		}
+
 		CChunkFileReader::Error Save()
 		{
 			std::lock_guard<std::mutex> guard(lock_);
@@ -118,8 +125,7 @@ namespace SaveState
 			if ((next_ % size_) == first_)
 				++first_;
 
-			static std::vector<u8> buffer;
-			std::vector<u8> *compressBuffer = &buffer;
+			std::vector<u8> *compressBuffer = &buffer_;
 			CChunkFileReader::Error err;
 
 			if (base_ == -1 || ++baseUsage_ > BASE_USAGE_INTERVAL)
@@ -131,12 +137,13 @@ namespace SaveState
 				compressBuffer = &bases_[base_];
 			}
 			else
-				err = SaveToRam(buffer);
+				err = SaveToRam(buffer_);
 
 			if (err == CChunkFileReader::ERROR_NONE)
 				ScheduleCompress(&states_[n], compressBuffer, &bases_[base_]);
 			else
 				states_[n].clear();
+
 			baseMapping_[n] = base_;
 			return err;
 		}
@@ -177,18 +184,23 @@ namespace SaveState
 			if (first_ == 0 && next_ == 0)
 				return;
 
+			double start_time = time_now_d();
 			result.clear();
+			result.reserve(512 * 1024);
 			for (size_t i = 0; i < state.size(); i += BLOCK_SIZE)
 			{
 				int blockSize = std::min(BLOCK_SIZE, (int)(state.size() - i));
 				if (i + blockSize > base.size() || memcmp(&state[i], &base[i], blockSize) != 0)
 				{
 					result.push_back(1);
-					result.insert(result.end(), state.begin() + i, state.begin() +i + blockSize);
+					result.insert(result.end(), state.begin() + i, state.begin() + i + blockSize);
 				}
 				else
 					result.push_back(0);
 			}
+
+			double taken_s = time_now_d() - start_time;
+			DEBUG_LOG(SAVESTATE, "Rewind: Compressed save from %d bytes to %d in %0.2f ms.", (int)state.size(), (int)result.size(), taken_s * 1000.0);
 		}
 
 		void LockedDecompress(std::vector<u8> &result, const std::vector<u8> &compressed, const std::vector<u8> &base)
@@ -232,14 +244,15 @@ namespace SaveState
 			return next_ == first_;
 		}
 
-		static const int BLOCK_SIZE;
+	private:
+		static const int BLOCK_SIZE = 8192;
 		// TODO: Instead, based on size of compressed state?
-		static const int BASE_USAGE_INTERVAL;
+		static const int BASE_USAGE_INTERVAL = 15;
 
 		typedef std::vector<u8> StateBuffer;
 
-		int first_;
-		int next_;
+		int first_ = 0;
+		int next_ = 0;
 		int size_;
 
 		std::vector<StateBuffer> states_;
@@ -247,9 +260,10 @@ namespace SaveState
 		std::vector<int> baseMapping_;
 		std::mutex lock_;
 		std::thread compressThread_;
+		std::vector<u8> buffer_;
 
-		int base_;
-		int baseUsage_;
+		int base_ = -1;
+		int baseUsage_ = 0;
 	};
 
 	static bool needsProcess = false;
@@ -273,8 +287,6 @@ namespace SaveState
 	// TODO: Any reason for this to be configurable?
 	const static float rewindMaxWallFrequency = 1.0f;
 	static double rewindLastTime = 0.0f;
-	const int StateRingbuffer::BLOCK_SIZE = 8192;
-	const int StateRingbuffer::BASE_USAGE_INTERVAL = 15;
 
 	void SaveStart::DoState(PointerWrap &p)
 	{

libretro/libretro.cpp

@@ -1652,8 +1652,8 @@ size_t retro_serialize_size(void)
    if (useEmuThread)
       EmuThreadPause();
 
-   return (CChunkFileReader::MeasurePtr(state) + 0x800000)
-      & ~0x7FFFFF; // We don't unpause intentionally
+   return (CChunkFileReader::MeasurePtr(state) + 0x800000) & ~0x7FFFFF;
+   // We don't unpause intentionally
 }
 
 bool retro_serialize(void *data, size_t size)
@@ -1668,9 +1668,8 @@ bool retro_serialize(void *data, size_t size)
    if (useEmuThread)
       EmuThreadPause(); // Does nothing if already paused
 
-   size_t measured = CChunkFileReader::MeasurePtr(state);
-   assert(measured <= size);
-   auto err = CChunkFileReader::SavePtr((u8 *)data, state, measured);
+   size_t measuredSize;
+   auto err = CChunkFileReader::MeasureAndSavePtr(state, (u8 **)&data, &measuredSize);
    retVal = err == CChunkFileReader::ERROR_NONE;
 
    if (useEmuThread)