Merge pull request #19644 from hrydgard/buffer-optimization

Optimize "Buffer" with a new data structure

CMakeLists.txt

@@ -653,6 +653,7 @@ add_library(Common STATIC
 	Common/Data/Collections/Hashmaps.h
 	Common/Data/Collections/TinySet.h
 	Common/Data/Collections/FastVec.h
+	Common/Data/Collections/CharQueue.h
 	Common/Data/Collections/CyclicBuffer.h
 	Common/Data/Collections/ThreadSafeList.h
 	Common/Data/Color/RGBAUtil.cpp

Common/Buffer.cpp

@@ -9,9 +9,7 @@
 
 char *Buffer::Append(size_t length) {
 	if (length > 0) {
-		size_t old_size = data_.size();
-		data_.resize(old_size + length);
-		return &data_[0] + old_size;
+		return data_.push_back_write(length);
 	} else {
 		return nullptr;
 	}
@@ -33,8 +31,11 @@ void Buffer::Append(const char *str) {
 void Buffer::Append(const Buffer &other) {
 	size_t len = other.size();
 	if (len > 0) {
-		char *dest = Append(len);
-		memcpy(dest, &other.data_[0], len);
+		// Append other to the current buffer.
+		other.data_.iterate_blocks([&](const char *data, size_t size) {
+			data_.push_back(data, size);
+			return true;
+		});
 	}
 }
 
@@ -57,8 +58,8 @@ void Buffer::Take(size_t length, std::string *dest) {
 }
 
 void Buffer::Take(size_t length, char *dest) {
-	memcpy(dest, &data_[0], length);
-	data_.erase(data_.begin(), data_.begin() + length);
+	size_t retval = data_.pop_front_bulk(dest, length);
+	_dbg_assert_(retval == length);
 }
 
 int Buffer::TakeLineCRLF(std::string *dest) {
@@ -79,7 +80,7 @@ void Buffer::Skip(size_t length) {
 		ERROR_LOG(Log::IO, "Truncating length in Buffer::Skip()");
 		length = data_.size();
 	}
-	data_.erase(data_.begin(), data_.begin() + length);
+	data_.skip(length);
 }
 
 int Buffer::SkipLineCRLF() {
@@ -92,9 +93,10 @@ int Buffer::SkipLineCRLF() {
 	}
 }
 
+// This relies on having buffered data!
 int Buffer::OffsetToAfterNextCRLF() {
 	for (int i = 0; i < (int)data_.size() - 1; i++) {
-		if (data_[i] == '\r' && data_[i + 1] == '\n') {
+		if (data_.peek(i) == '\r' && data_.peek(i + 1) == '\n') {
 			return i + 2;
 		}
 	}
@@ -124,7 +126,11 @@ bool Buffer::FlushToFile(const Path &filename) {
 	if (!f)
 		return false;
 	if (!data_.empty()) {
-		fwrite(&data_[0], 1, data_.size(), f);
+		// Write the buffer to the file.
+		data_.iterate_blocks([=](const char *blockData, size_t blockSize) {
+			return fwrite(blockData, 1, blockSize, f) == blockSize;
+		});
+		data_.clear();
 	}
 	fclose(f);
 	return true;
@@ -132,5 +138,8 @@ bool Buffer::FlushToFile(const Path &filename) {
 
 void Buffer::PeekAll(std::string *dest) {
 	dest->resize(data_.size());
-	memcpy(&(*dest)[0], &data_[0], data_.size());
+	data_.iterate_blocks(([=](const char *blockData, size_t blockSize) {
+		dest->append(blockData, blockSize);
+		return true;
+	}));
 }

Common/Buffer.h

@@ -4,6 +4,7 @@
 #include <vector>
 
 #include "Common/Common.h"
+#include "Common/Data/Collections/CharQueue.h"
 
 class Path;
 
@@ -72,12 +73,12 @@ public:
 	// Utilities. Try to avoid checking for size.
 	size_t size() const { return data_.size(); }
 	bool empty() const { return size() == 0; }
-	void clear() { data_.resize(0); }
+	void clear() { data_.clear(); }
 	bool IsVoid() const { return void_; }
 
 protected:
-	// TODO: Find a better internal representation, like a cord.
-	std::vector<char> data_;
+	// Custom queue implementation.
+	CharQueue data_;
 	bool void_ = false;
 
 private:

Common/Common.vcxproj

@@ -456,6 +456,7 @@
     <ClInclude Include="BitSet.h" />
     <ClInclude Include="Buffer.h" />
     <ClInclude Include="Data\Collections\ConstMap.h" />
+    <ClInclude Include="Data\Collections\CharQueue.h" />
     <ClInclude Include="Data\Collections\FixedSizeQueue.h" />
     <ClInclude Include="Data\Collections\Hashmaps.h" />
     <ClInclude Include="Data\Collections\Slice.h" />

Common/Common.vcxproj.filters

@@ -671,6 +671,9 @@
     <ClInclude Include="..\ext\lua\lzio.h">
       <Filter>ext\lua</Filter>
     </ClInclude>
+    <ClInclude Include="Data\Collections\CharQueue.h">
+      <Filter>Data\Collections</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="ABI.cpp" />

Common/Data/Collections/CharQueue.h

@@ -0,0 +1,211 @@
+#pragma once
+
+#include "Common/Log.h"
+#include "Common/Data/Collections/Slice.h"
+
+#include <cstdlib>
+#include <cstring>
+#include <string_view>
+
+// Queue with a dynamic size, optimized for bulk inserts and retrievals - and optimized
+// to be fast in debug builds, hence it's pretty much C internally.
+class CharQueue {
+public:
+	explicit CharQueue(size_t blockSize = 16384) : blockSize_(blockSize) {
+		head_ = new Block{};
+		tail_ = head_;
+		head_->data = (char *)malloc(blockSize_);
+		head_->size = (int)blockSize_;
+		head_->next = nullptr;
+	}
+
+	// Remove copy constructors.
+	CharQueue(const CharQueue &) = delete;
+	void operator=(const CharQueue &) = delete;
+
+	// But let's have a move constructor.
+	CharQueue(CharQueue &&src) noexcept {
+		// Steal the data from the other queue.
+		blockSize_ = src.blockSize_;
+		head_ = src.head_;
+		tail_ = src.tail_;
+		// Give the old queue a new block. Could probably also leave it in an invalid state and get rid of it.
+		src.head_ = new Block{};
+		src.tail_ = src.head_;
+		src.head_->data = (char *)malloc(src.blockSize_);
+		src.head_->size = (int)src.blockSize_;
+	}
+
+	~CharQueue() {
+		clear();
+		_dbg_assert_(head_ == tail_);
+		_dbg_assert_(head_->size == blockSize_);
+		_dbg_assert_(size() == 0);
+		// delete the final block
+		delete head_;
+	}
+
+	char *push_back_write(size_t size) {
+		int remain = tail_->size - tail_->tail;
+		_dbg_assert_(remain >= 0);
+		if (remain >= (int)size) {
+			char *retval = tail_->data + tail_->tail;
+			tail_->tail += (int)size;
+			return retval;
+		} else {
+			// Can't fit? Just allocate a new block and fill it up with the new data.
+			int bsize = (int)blockSize_;
+			if (size > bsize) {
+				bsize = (int)size;
+			}
+			Block *b = new Block{};
+			b->head = 0;
+			b->tail = (int)size;
+			b->size = bsize;
+			b->data = (char *)malloc(bsize);
+			tail_->next = b;
+			tail_ = b;
+			return tail_->data;
+		}
+	}
+
+	void push_back(const char *data, size_t size) {
+		memcpy(push_back_write(size), data, size);
+	}
+
+	void push_back(std::string_view chars) {
+		memcpy(push_back_write(chars.size()), chars.data(), chars.size());
+	}
+
+	// For debugging, mainly.
+	size_t block_count() const {
+		int count = 0;
+		Block *b = head_;
+		do {
+			count++;
+			b = b->next;
+		} while (b);
+		return count;
+	}
+
+	size_t size() const {
+		size_t s = 0;
+		Block *b = head_;
+		do {
+			s += b->tail - b->head;
+			b = b->next;
+		} while (b);
+		return s;
+	}
+
+	char peek(size_t peekOff) {
+		Block *b = head_;
+		do {
+			int remain = b->tail - b->head;
+			if (remain > peekOff) {
+				return b->data[b->head + peekOff];
+			} else {
+				peekOff -= remain;
+			}
+			b = b->next;
+		} while (b);
+		// Ran out of data.
+		_dbg_assert_(false);
+		return 0;
+	}
+
+	bool empty() const {
+		return size() == 0;
+	}
+
+	// Pass in a lambda that takes each partial buffer as char*, size_t.
+	template<typename Func>
+	bool iterate_blocks(Func callback) const {
+		Block *b = head_;
+		do {
+			if (b->tail > b->head) {
+				if (!callback(b->data + b->head, b->tail - b->head)) {
+					return false;
+				}
+			}
+			b = b->next;
+		} while (b);
+		return true;
+	}
+
+	size_t pop_front_bulk(char *dest, size_t size) {
+		int popSize = (int)size;
+		int writeOff = 0;
+		while (popSize > 0) {
+			int remain = head_->tail - head_->head;
+			int readSize = popSize;
+			if (readSize > remain) {
+				readSize = remain;
+			}
+			if (dest) {
+				memcpy(dest + writeOff, head_->data + head_->head, readSize);
+			}
+			writeOff += readSize;
+			head_->head += readSize;
+			popSize -= readSize;
+			if (head_->head == head_->tail) {
+				// Ran out of data in this block. Let's hope there's more...
+				if (head_ == tail_) {
+					// Can't read any more, bail.
+					break;
+				}
+				Block *next = head_->next;
+				delete head_;
+				head_ = next;
+			}
+		}
+		return (int)size - popSize;
+	}
+
+	size_t skip(size_t size) {
+		return pop_front_bulk(nullptr, size);
+	}
+
+	void clear() {
+		Block *b = head_;
+		// Delete all blocks except the last.
+		while (b != tail_) {
+			Block *next = b->next;
+			delete b;
+			b = next;
+		}
+		if (b->size != blockSize_) {
+			// Restore the remaining block to default size.
+			free(b->data);
+			b->data = (char *)malloc(blockSize_);
+			b->size = (int)blockSize_;
+		}
+		b->head = 0;
+		b->tail = 0;
+		// head and tail are now equal.
+		head_ = b;
+	}
+
+private:
+	struct Block {
+		~Block() {
+			if (data) {
+				free(data);
+				data = 0;
+			}
+			size = 0;
+		}
+		Block *next;
+		char *data;
+		int size;  // Can be bigger than the default block size if a push is very large.
+		// Internal head and tail inside the block.
+		int head;
+		int tail;
+	};
+
+	// There's always at least one block, initialized in the constructor.
+	Block *head_;
+	Block *tail_;
+	// Default min block size for new blocks.
+	size_t blockSize_;
+};

Common/Data/Collections/Slice.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <vector>
+#include <cstdlib>
 
 // Like a const begin/end pair, just more convenient to use (and can only be used for linear array data).
 // Inspired by Rust's slices and Google's StringPiece.

Common/Net/NetBuffer.cpp

@@ -36,27 +36,32 @@ void RequestProgress::Update(int64_t downloaded, int64_t totalBytes, bool done)
 
 bool Buffer::FlushSocket(uintptr_t sock, double timeout, bool *cancelled) {
 	static constexpr float CANCEL_INTERVAL = 0.25f;
-	for (size_t pos = 0, end = data_.size(); pos < end; ) {
-		bool ready = false;
-		double endTimeout = time_now_d() + timeout;
-		while (!ready) {
-			if (cancelled && *cancelled)
-				return false;
-			ready = fd_util::WaitUntilReady(sock, CANCEL_INTERVAL, true);
-			if (!ready && time_now_d() > endTimeout) {
-				ERROR_LOG(Log::IO, "FlushSocket timed out");
+
+	data_.iterate_blocks([&](const char *data, size_t size) {
+		for (size_t pos = 0, end = size; pos < end; ) {
+			bool ready = false;
+			double endTimeout = time_now_d() + timeout;
+			while (!ready) {
+				if (cancelled && *cancelled)
+					return false;
+				ready = fd_util::WaitUntilReady(sock, CANCEL_INTERVAL, true);
+				if (!ready && time_now_d() > endTimeout) {
+					ERROR_LOG(Log::IO, "FlushSocket timed out");
+					return false;
+				}
+			}
+			int sent = send(sock, &data[pos], end - pos, MSG_NOSIGNAL);
+			// TODO: Do we need some retry logic here, instead of just giving up?
+			if (sent < 0) {
+				ERROR_LOG(Log::IO, "FlushSocket failed to send: %d", errno);
 				return false;
 			}
+			pos += sent;
 		}
-		int sent = send(sock, &data_[pos], end - pos, MSG_NOSIGNAL);
-		// TODO: Do we need some retry logic here, instead of just giving up?
-		if (sent < 0) {
-			ERROR_LOG(Log::IO, "FlushSocket failed to send: %d", errno);
-			return false;
-		}
-		pos += sent;
-	}
-	data_.resize(0);
+		return true;
+	});
+
+	data_.clear();
 	return true;
 }
 

unittest/UnitTest.cpp

@@ -48,10 +48,12 @@
 
 #include "Common/Data/Collections/TinySet.h"
 #include "Common/Data/Collections/FastVec.h"
+#include "Common/Data/Collections/CharQueue.h"
 #include "Common/Data/Convert/SmallDataConvert.h"
 #include "Common/Data/Text/Parsers.h"
 #include "Common/Data/Text/WrapText.h"
 #include "Common/Data/Encoding/Utf8.h"
+#include "Common/Buffer.h"
 #include "Common/File/Path.h"
 #include "Common/Input/InputState.h"
 #include "Common/Math/math_util.h"
@@ -1034,6 +1036,59 @@ bool TestColorConv() {
 	return true;
 }
 
+CharQueue GetQueue() {
+	CharQueue queue(5);
+	return queue;
+}
+
+bool TestCharQueue() {
+	// We use a tiny block size for testing.
+	CharQueue queue = std::move(GetQueue());
+
+	// Add 16 chars.
+	queue.push_back("abcdefghijkl");
+	queue.push_back("mnop");
+
+	std::string testStr;
+	queue.iterate_blocks([&](const char *buf, size_t sz) {
+		testStr.append(buf, sz);
+		return true;
+	});
+	EXPECT_EQ_STR(testStr, std::string("abcdefghijklmnop"));
+
+	EXPECT_EQ_CHAR(queue.peek(11), 'l');
+	EXPECT_EQ_CHAR(queue.peek(12), 'm');
+	EXPECT_EQ_CHAR(queue.peek(15), 'p');
+	EXPECT_EQ_INT(queue.block_count(), 3);  // Didn't fit in the first block, so the two pushes above should have each created one additional block.
+	EXPECT_EQ_INT(queue.size(), 16);
+	char dest[15];
+	EXPECT_EQ_INT(queue.pop_front_bulk(dest, 4), 4);
+	EXPECT_EQ_INT(queue.size(), 12);
+	EXPECT_EQ_MEM(dest, "abcd", 4);
+	EXPECT_EQ_INT(queue.pop_front_bulk(dest, 6), 6);
+	EXPECT_EQ_INT(queue.size(), 6);
+	EXPECT_EQ_MEM(dest, "efghij", 6);
+	queue.push_back("qr");
+	EXPECT_EQ_INT(queue.pop_front_bulk(dest, 4), 4);  // should pop off klmn
+	EXPECT_EQ_MEM(dest, "klmn", 4);
+	EXPECT_EQ_INT(queue.size(), 4);
+	EXPECT_EQ_CHAR(queue.peek(3), 'r');
+	queue.pop_front_bulk(dest, 4);
+	EXPECT_EQ_MEM(dest, "opqr", 4);
+	EXPECT_TRUE(queue.empty());
+	return true;
+}
+
+bool TestBuffer() {
+	Buffer b = Buffer::Void();
+	b.Append("hello");
+	b.Append("world");
+	std::string temp;
+	b.Take(10, &temp);
+	EXPECT_EQ_STR(temp, std::string("helloworld"));
+	return true;
+}
+
 typedef bool (*TestFunc)();
 struct TestItem {
 	const char *name;
@@ -1094,6 +1149,8 @@ TestItem availableTests[] = {
 	TEST_ITEM(Substitutions),
 	TEST_ITEM(IniFile),
 	TEST_ITEM(ColorConv),
+	TEST_ITEM(CharQueue),
+	TEST_ITEM(Buffer),
 };
 
 int main(int argc, const char *argv[]) {

unittest/UnitTest.h

@@ -17,10 +17,12 @@ inline bool rel_equal(float a, float b, float precision) {
 #define EXPECT_TRUE(a) if (!(a)) { printf("%s:%i: Test Fail\n", __FUNCTION__, __LINE__); return false; }
 #define EXPECT_FALSE(a) if ((a)) { printf("%s:%i: Test Fail\n", __FUNCTION__, __LINE__); return false; }
 #define EXPECT_EQ_INT(a, b) if ((a) != (b)) { printf("%s:%i: Test Fail\n%d\nvs\n%d\n", __FUNCTION__, __LINE__, (int)(a), (int)(b)); return false; }
+#define EXPECT_EQ_CHAR(a, b) if ((a) != (b)) { printf("%s:%i: Test Fail\n%c\nvs\n%c\n", __FUNCTION__, __LINE__, (int)(a), (int)(b)); return false; }
 #define EXPECT_EQ_HEX(a, b) if ((a) != (b)) { printf("%s:%i: Test Fail\n%x\nvs\n%x\n", __FUNCTION__, __LINE__, a, b); return false; }
 #define EXPECT_EQ_FLOAT(a, b) if ((a) != (b)) { printf("%s:%i: Test Fail\n%0.7f\nvs\n%0.7f\n", __FUNCTION__, __LINE__, a, b); return false; }
 #define EXPECT_APPROX_EQ_FLOAT(a, b) if (fabsf((a)-(b))>0.00001f) { printf("%s:%i: Test Fail\n%f\nvs\n%f\n", __FUNCTION__, __LINE__, a, b); /*return false;*/ }
 #define EXPECT_REL_EQ_FLOAT(a, b, precision) if (!rel_equal(a, b, precision)) { printf("%s:%i: Test Fail\n%0.9f\nvs\n%0.9f\n", __FUNCTION__, __LINE__, a, b); /*return false;*/ }
 #define EXPECT_EQ_STR(a, b) if (a != b) { printf("%s: Test Fail\n%s\nvs\n%s\n", __FUNCTION__, a.c_str(), b.c_str()); return false; }
+#define EXPECT_EQ_MEM(a, b, sz) if (memcmp(a, b, sz) != 0) { printf("%s: Test Fail\n%.*s\nvs\n%.*s\n", __FUNCTION__, (int)sz, a, (int)sz, b); return false; }
 
 #define RET(a) if (!(a)) { return false; }