SlashRemover optimization (#1781)

lib/src/HttpClientImpl.h

@@ -110,7 +110,7 @@ class HttpClientImpl final : public HttpClient,
     void addSSLConfigs(const std::vector<std::pair<std::string, std::string>>
                            &sslConfCmds) override;
 
-    void setSockOptCallback(std::function<void(int)> cb)
+    void setSockOptCallback(std::function<void(int)> cb) override
     {
         sockOptCallback_ = std::move(cb);
     }

lib/src/SlashRemover.cc

@@ -2,16 +2,17 @@
 #include <drogon/plugins/Redirector.h>
 #include <drogon/HttpAppFramework.h>
 #include "drogon/utils/FunctionTraits.h"
+#include <cstddef>
+#include <cstdint>
 #include <functional>
 #include <string>
-#include <regex>
+#include <string_view>
+#include <utility>
 
 using namespace drogon;
 using namespace drogon::plugin;
 using std::string;
-
+using std::string_view;
-#define TRAILING_SLASH_REGEX ".+\\/$"
-#define DUPLICATE_SLASH_REGEX ".*\\/{2,}.*"
 
 enum removeSlashMode : uint8_t
 {
@@ -20,75 +21,176 @@ enum removeSlashMode : uint8_t
     both = trailing | duplicate,
 };
 
-inline constexpr const char* regexes[] = {
+/// Returns the index before the trailing slashes,
-    TRAILING_SLASH_REGEX,
+/// or 0 if only contains slashes
-    DUPLICATE_SLASH_REGEX,
+static inline size_t findTrailingSlashes(string_view url)
-    TRAILING_SLASH_REGEX "|" DUPLICATE_SLASH_REGEX,
-};
-
-static inline bool removeTrailingSlashes(string& url)
 {
-    const auto notSlashIndex = url.find_last_not_of('/');
+    auto len = url.size();
-    if (notSlashIndex == string::npos)  // Root
+    // Must be at least 2 chars and end with a slash
-    {
+    if (len < 2 || url.back() != '/')
-        url.resize(1);
+        return string::npos;
-        return true;
+
-    }
+    size_t a = len - 1;  // We already know the last char is '/',
-    url.resize(notSlashIndex + 1);
+                         // we will use pre-decrement to account for this
-    return false;
+    while (--a > 0 && url[a] == '/')
+        ;  // We know the first char is '/', so don't check for 0
+    return a;
 }
 
-static inline void removeDuplicateSlashes(string& url)
+static inline void removeTrailingSlashes(string& url,
+                                         size_t start,
+                                         string_view originalUrl)
 {
-    size_t a = 1, len = url.size();
+    url = originalUrl.substr(0, start + 1);
-    for (; a < len && (url[a - 1] != '/' || url[a] != '/'); ++a)
+}
-        ;
+
-    for (size_t b = a--; b < len; ++b)
+/// Returns the index of the 2nd duplicate slash
+static inline size_t findDuplicateSlashes(string_view url)
+{
+    size_t len = url.size();
+    if (len < 2)
+        return string::npos;
+
+    bool startedPair = true;  // Always starts with a slash
+    for (size_t a = 1; a < len; ++a)
+    {
+        if (url[a] != '/')  // Broken pair
+        {
+            startedPair = false;
+            continue;
+        }
+        if (startedPair)  // Matching pair
+            return a;
+        startedPair = true;
+    }
+
+    return string::npos;
+}
+
+static inline void removeDuplicateSlashes(string& url, size_t start)
+{
+    // +1 because we don't need to look at the same character again,
+    // which was found by `findDuplicateSlashes`, it saves one iteration
+    for (size_t b = (start--) + 1, len = url.size(); b < len; ++b)
     {
         const char c = url[b];
-        if (c != '/' || url[a] != '/')
+        if (c != '/' || url[start] != '/')
         {
-            ++a;
+            ++start;
-            url[a] = c;
+            url[start] = c;
         }
     }
-    url.resize(a + 1);
+    url.resize(start + 1);
 }
 
-static inline void removeExcessiveSlashes(string& url)
+static inline std::pair<size_t, size_t> findExcessiveSlashes(string_view url)
 {
-    if (url.back() == '/' &&  // This check is so we don't search if there is no
+    size_t len = url.size();
-                              // trailing slash to begin with
+    if (len < 2)  // Must have at least 2 characters to count as either trailing
-        removeTrailingSlashes(
+                  // or duplicate slash
-            url))  // If it is root path, we don't need to check for duplicates
+        return {string::npos, string::npos};
-        return;
 
-    removeDuplicateSlashes(url);
+    // Trail finder
-}
+    size_t trailIdx = len;  // The pre-decrement will put it on last char
+    while (--trailIdx > 0 && url[trailIdx] == '/')
+        ;  // We know first char is '/', no need to check it
 
-static inline bool handleReq(const drogon::HttpRequestPtr& req, int removeMode)
+    // Filled with '/'
-{
+    if (trailIdx == 0)
-    static const std::regex regex(regexes[removeMode - 1]);
+        return {
-    if (std::regex_match(req->path(), regex))
+            0,             // Only keep first slash
+            string::npos,  // No duplicate
+        };
+
+    // Look for a duplicate pair
+    size_t dupIdx = 1;
+    for (bool startedPair = true; dupIdx < trailIdx;
+         ++dupIdx)  // Always starts with a slash
     {
-        string newPath = req->path();
+        if (url[dupIdx] != '/')  // Broken pair
+        {
+            startedPair = false;
+            continue;
+        }
+        if (startedPair)  // Matching pair
+            break;
+        startedPair = true;
+    }
+
+    // Found no duplicate
+    if (dupIdx == trailIdx)
+        return {
+            trailIdx != len - 1
+                ?  // If has gone past last char, then there is a trailing slash
+                trailIdx
+                : string::npos,  // No trail
+            string::npos,        // No duplicate
+        };
+
+    // Duplicate found
+    return {
+        trailIdx != len - 1
+            ?  // If has gone past last char, then there is a trailing slash
+            trailIdx
+            : string::npos,  // No trail
+        dupIdx,
+    };
+}
+
+static inline void removeExcessiveSlashes(string& url,
+                                          std::pair<size_t, size_t> start,
+                                          string_view originalUrl)
+{
+    if (start.first != string::npos)
+        removeTrailingSlashes(url, start.first, originalUrl);
+    else
+        url = originalUrl;
+
+    if (start.second != string::npos)
+        removeDuplicateSlashes(url, start.second);
+}
+
+static inline bool handleReq(const drogon::HttpRequestPtr& req,
+                             uint8_t removeMode)
+{
     switch (removeMode)
     {
         case trailing:
-                removeTrailingSlashes(newPath);
+        {
+            auto find = findTrailingSlashes(req->path());
+            if (find == string::npos)
+                return false;
+
+            string newPath;
+            removeTrailingSlashes(newPath, find, req->path());
+            req->setPath(std::move(newPath));
             break;
+        }
         case duplicate:
-                removeDuplicateSlashes(newPath);
+        {
+            auto find = findDuplicateSlashes(req->path());
+            if (find == string::npos)
+                return false;
+
+            string newPath = req->path();
+            removeDuplicateSlashes(newPath, find);
+            req->setPath(std::move(newPath));
             break;
+        }
         case both:
         default:
-                removeExcessiveSlashes(newPath);
+        {
+            auto find = findExcessiveSlashes(req->path());
+            if (find.first == string::npos && find.second == string::npos)
+                return false;
+
+            string newPath;
+            removeExcessiveSlashes(newPath, find, req->path());
+            req->setPath(std::move(newPath));
             break;
         }
-        req->setPath(std::move(newPath));
-        return true;
     }
-    return false;
+    return true;
 }
 
 void SlashRemover::initAndStart(const Json::Value& config)

lib/tests/unittests/SlashRemoverTest.cc

@@ -1,43 +1,179 @@
 #include <drogon/drogon_test.h>
 #include <../../lib/src/SlashRemover.cc>
+#include <string>
 
 using std::string;
 
 DROGON_TEST(SlashRemoverTest)
 {
-    const string url = "///home//page//", urlNoTrail = "///home//page",
+    string cleanUrl;
-                 urlNoDup = "/home/page/", urlNoExcess = "/home/page",
-                 root = "///", rootNoExcess = "/";
 
-    string cleanUrl = url;
+    {  // Regular URL
-    removeTrailingSlashes(cleanUrl);
+        const string urlNoTrail = "///home//page", urlNoDup = "/home/page/",
+                     urlNoExcess = "/home/page";
+
+        SUBSECTION(Full)
+        {
+            const string url = "///home//page//";
+            removeTrailingSlashes(cleanUrl, findTrailingSlashes(url), url);
             CHECK(cleanUrl == urlNoTrail);
 
             cleanUrl = url;
-    removeDuplicateSlashes(cleanUrl);
+            removeDuplicateSlashes(cleanUrl, findDuplicateSlashes(url));
             CHECK(cleanUrl == urlNoDup);
 
+            removeExcessiveSlashes(cleanUrl, findExcessiveSlashes(url), url);
+            CHECK(cleanUrl == urlNoExcess);
+        }
+
+        SUBSECTION(Partial)
+        {
+            removeExcessiveSlashes(cleanUrl,
+                                   findExcessiveSlashes(urlNoTrail),
+                                   urlNoTrail);
+            CHECK(cleanUrl == urlNoExcess);
+
+            removeExcessiveSlashes(cleanUrl,
+                                   findExcessiveSlashes(urlNoDup),
+                                   urlNoDup);
+            CHECK(cleanUrl == urlNoExcess);
+        }
+    }
+
+    SUBSECTION(Root)
+    {
+        const string urlNoExcess = "/";
+
+        {  // Overlapping indices
+            const string url = "//";
+
             cleanUrl = url;
-    removeExcessiveSlashes(cleanUrl);
+            removeTrailingSlashes(cleanUrl, findTrailingSlashes(url), url);
             CHECK(cleanUrl == urlNoExcess);
 
-    cleanUrl = urlNoTrail;
+            cleanUrl = url;
-    removeExcessiveSlashes(cleanUrl);
+            removeDuplicateSlashes(cleanUrl, findDuplicateSlashes(url));
             CHECK(cleanUrl == urlNoExcess);
 
-    cleanUrl = urlNoDup;
+            removeExcessiveSlashes(cleanUrl, findExcessiveSlashes(url), url);
-    removeExcessiveSlashes(cleanUrl);
+            CHECK(cleanUrl == urlNoExcess);
+        }
+
+        {  // Intersecting indices
+            const string url = "///";
+
+            cleanUrl = url;
+            removeTrailingSlashes(cleanUrl, findTrailingSlashes(url), url);
             CHECK(cleanUrl == urlNoExcess);
 
-    cleanUrl = root;
+            cleanUrl = url;
-    removeTrailingSlashes(cleanUrl);
+            removeDuplicateSlashes(cleanUrl, findDuplicateSlashes(url));
-    CHECK(cleanUrl == rootNoExcess);
+            CHECK(cleanUrl == urlNoExcess);
 
-    cleanUrl = root;
+            removeExcessiveSlashes(cleanUrl, findExcessiveSlashes(url), url);
-    removeDuplicateSlashes(cleanUrl);
+            CHECK(cleanUrl == urlNoExcess);
-    CHECK(cleanUrl == rootNoExcess);
+        }
+    }
 
-    cleanUrl = root;
+    SUBSECTION(Overlap)
-    removeExcessiveSlashes(cleanUrl);
+    {
-    CHECK(cleanUrl == rootNoExcess);
+        const string urlNoDup = "/a/", urlNoExcess = "/a";
+
+        {  // Overlapping indices
+            const string url = "/a//";
+
+            removeTrailingSlashes(cleanUrl, findTrailingSlashes(url), url);
+            CHECK(cleanUrl == urlNoExcess);
+
+            cleanUrl = url;
+            removeDuplicateSlashes(cleanUrl, findDuplicateSlashes(url));
+            CHECK(cleanUrl == urlNoDup);
+
+            removeExcessiveSlashes(cleanUrl, findExcessiveSlashes(url), url);
+            CHECK(cleanUrl == urlNoExcess);
+        }
+
+        {  // Intersecting indices
+            const string url = "/a///";
+
+            removeTrailingSlashes(cleanUrl, findTrailingSlashes(url), url);
+            CHECK(cleanUrl == urlNoExcess);
+
+            cleanUrl = url;
+            removeDuplicateSlashes(cleanUrl, findDuplicateSlashes(url));
+            CHECK(cleanUrl == urlNoDup);
+
+            removeExcessiveSlashes(cleanUrl, findExcessiveSlashes(url), url);
+            CHECK(cleanUrl == urlNoExcess);
+        }
+    }
+
+    SUBSECTION(NoTrail)
+    {
+        const string url = "//a", urlNoExcess = "/a";
+
+        cleanUrl.clear();
+        {
+            auto find = findTrailingSlashes(url);
+            if (find != string::npos)
+                removeTrailingSlashes(cleanUrl, find, url);
+        }
+        CHECK(cleanUrl.empty());
+
+        cleanUrl = url;
+        removeDuplicateSlashes(cleanUrl, findDuplicateSlashes(url));
+        CHECK(cleanUrl == urlNoExcess);
+
+        removeExcessiveSlashes(cleanUrl, findExcessiveSlashes(url), url);
+        CHECK(cleanUrl == urlNoExcess);
+    }
+
+    SUBSECTION(NoDuplicate)
+    {
+        const string url = "/a/", urlNoExcess = "/a";
+
+        removeTrailingSlashes(cleanUrl, findTrailingSlashes(url), url);
+        CHECK(cleanUrl == urlNoExcess);
+
+        cleanUrl = url;
+        {
+            auto find = findDuplicateSlashes(cleanUrl);
+            if (find != string::npos)
+                removeDuplicateSlashes(cleanUrl, find);
+        }
+        CHECK(cleanUrl == url);
+
+        cleanUrl = url;
+        removeExcessiveSlashes(cleanUrl, findExcessiveSlashes(url), url);
+        CHECK(cleanUrl == urlNoExcess);
+    }
+
+    SUBSECTION(None)
+    {
+        const string url = "/a";
+
+        cleanUrl.clear();
+        {
+            auto find = findTrailingSlashes(url);
+            if (find != string::npos)
+                removeTrailingSlashes(cleanUrl, find, url);
+        }
+        CHECK(cleanUrl.empty());
+
+        cleanUrl = url;
+        {
+            auto find = findDuplicateSlashes(cleanUrl);
+            if (find != string::npos)
+                removeDuplicateSlashes(cleanUrl, find);
+        }
+        CHECK(cleanUrl == url);
+
+        cleanUrl.clear();
+        {
+            auto find = findExcessiveSlashes(url);
+            if (find.first != string::npos || find.second != string::npos)
+                removeExcessiveSlashes(cleanUrl, find, url);
+        }
+        CHECK(cleanUrl.empty());
+    }
 }