From 911676479377723f9672e2ed0e2b03e15412f2df Mon Sep 17 00:00:00 2001
From: w00t <w00t@e03df62e-2008-0410-955e-edbf42e46eb7>
Date: Wed, 7 May 2008 21:53:30 +0000
Subject: Masterful rewrite of User::AddBuffer to remove a string copy (and
 make more efficient) thanks to some nifty string manipulations. This should
 provide benefit on highly loaded nodes. This has *not* been thoroughly tested
 considering it's criticality, but I have spent the past ~4 hours writing and
 testing it, and it seems ok.

git-svn-id: http://svn.inspircd.org/repository/trunk/inspircd@9661 e03df62e-2008-0410-955e-edbf42e46eb7
---
 include/users.h |  2 +-
 src/users.cpp   | 74 +++++++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/include/users.h b/include/users.h
index 229edcfa2..9a7b9a6e7 100644
--- a/include/users.h
+++ b/include/users.h
@@ -807,7 +807,7 @@ class CoreExport User : public connection
 	 * @param a The string to add to the users read buffer
 	 * @return True if the string was successfully added to the read buffer
 	 */
-	bool AddBuffer(std::string a);
+	bool AddBuffer(const std::string &a);
 
 	/** This method returns true if the buffer contains at least one carriage return
 	 * character (e.g. one complete line may be read)
diff --git a/src/users.cpp b/src/users.cpp
index d8ed553a7..ab4270845 100644
--- a/src/users.cpp
+++ b/src/users.cpp
@@ -507,42 +507,74 @@ bool User::HasPermission(const std::string &command)
 	return false;
 }
 
-/** NOTE: We cannot pass a const reference to this method.
- * The string is changed by the workings of the method,
- * so that if we pass const ref, we end up copying it to
- * something we can change anyway. Makes sense to just let
- * the compiler do that copy for us.
- */
-bool User::AddBuffer(std::string a)
+bool User::AddBuffer(const std::string &a)
 {
-	try
+	std::string::size_type start = 0;
+	std::string::size_type i = a.find('\r');
+
+	/*
+	 * The old implementation here took a copy, and rfind() on \r, removing as it found them, before
+	 * copying a second time onto the recvq. That's ok, but involves three copies minimum (recv() to buffer,
+	 * buffer to here, here to recvq) - The new method now copies twice (recv() to buffer, buffer to recvq).
+	 *
+	 * We use find() instead of rfind() for clarity, however unlike the old code, our scanning of the string is
+	 * contiguous: as we specify a startpoint, we never see characters we have scanned previously, making this
+	 * marginally faster in cases with a number of \r hidden early on in the buffer.
+	 *
+	 * How it works:
+	 * Start at first pos of string, find first \r, append everything in the chunk (excluding \r) to recvq. Set
+	 * i ahead of the \r, search for next \r, add next chunk to buffer... repeat.
+	 *		-- w00t (7 may, 2008)
+	 */
+	if (i == std::string::npos)
 	{
-		std::string::size_type i = a.rfind('\r');
+		// no \r that we need to dance around, just add to buffer
+		recvq.append(a);
+	}
+	else
+	{
+		ServerInstance->Logs->Log("recvqdebug", DEBUG, "Current recvq size is %d and I got called with a string of %d\n(%s)", recvq.length(), a.length(), a.c_str());
 
+		// While we can find the end of a chunk to add
 		while (i != std::string::npos)
 		{
-			a.erase(i, 1);
-			i = a.rfind('\r');
-		}
+			// Append the chunk that we have
+			recvq.append(a, start, (i - start));
+			ServerInstance->Logs->Log("recvqdebug", DEBUG, "Appended a chunk, length is now %d", recvq.length());
 
-		if (a.length())
-			recvq.append(a);
+			// Start looking for the next one
+			start = i + 1;
+			i = a.find('\r', start);
+		}
 
-		if (this->MyClass && (recvq.length() > this->MyClass->GetRecvqMax()))
+		if (start != a.length())
 		{
-			this->SetWriteError("RecvQ exceeded");
-			ServerInstance->SNO->WriteToSnoMask('A', "User %s RecvQ of %lu exceeds connect class maximum of %lu",this->nick,(unsigned long int)recvq.length(),this->MyClass->GetRecvqMax());
-			return false;
+			/*
+			 * This is here to catch a corner case when we get something like:
+			 * NICK w0
+			 * 0t\r\nU
+			 * SER ...
+			 * in successive calls to us.
+			 *
+			 * Without this conditional, the 'U' on the second case will be dropped,
+			 * which is most *certainly* not the behaviour we want!
+			 *		-- w00t
+			 */
+			ServerInstance->Logs->Log("recvqdebug", DEBUG, "*** ALERT *** start != a.length, we should probably add more");
+			recvq.append(a, start, (a.length() - start));
 		}
 
-		return true;
+		ServerInstance->Logs->Log("recvqdebug", DEBUG, "Final recvq length is %d\n(%s)", recvq.length(), recvq.c_str());
 	}
 
-	catch (...)
+	if (this->MyClass && (recvq.length() > this->MyClass->GetRecvqMax()))
 	{
-		ServerInstance->Logs->Log("USERS", DEBUG,"Exception in User::AddBuffer()");
+		this->SetWriteError("RecvQ exceeded");
+		ServerInstance->SNO->WriteToSnoMask('A', "User %s RecvQ of %lu exceeds connect class maximum of %lu",this->nick,(unsigned long int)recvq.length(),this->MyClass->GetRecvqMax());
 		return false;
 	}
+
+	return true;
 }
 
 bool User::BufferIsReady()
-- 
cgit v1.2.3