Make router take up less space for verbs
As is, the router designates routes for every possible boost verb, of
which there are 31. In bmcweb, we only make use of 6 of those verbs, so
that ends up being quite a bit of wasted space and cache non-locality.
This commit invents a new enum class for declaring a subset of boost
verbs that we support, and a mapping between bmcweb verbs and boost
verbs.
Then it walks through and updates the router to support converting one
to another.
Tested:
Unit Tested
Redfish Service Validator performed on future commit
Signed-off-by: Ed Tanous <edtanous@google.com>
Signed-off-by: Edward Lee <edwarddl@google.com>
Change-Id: I3c89e896c632a5d4134dbd08a30b313c12a60de6
diff --git a/http/routing.hpp b/http/routing.hpp
index 870a62c..6c393ab 100644
--- a/http/routing.hpp
+++ b/http/routing.hpp
@@ -9,6 +9,7 @@
#include "privileges.hpp"
#include "sessions.hpp"
#include "utility.hpp"
+#include "verb.hpp"
#include "websocket.hpp"
#include <async_resp.hpp>
@@ -20,6 +21,7 @@
#include <cstdlib>
#include <limits>
#include <memory>
+#include <optional>
#include <tuple>
#include <utility>
#include <vector>
@@ -27,13 +29,7 @@
namespace crow
{
-// Note, this is an imperfect abstraction. There are a lot of verbs that we
-// use memory for, but are basically unused by most implementations.
-// Ideally we would have a list of verbs that we do use, and only index in
-// to a smaller array of those, but that would require a translation from
-// boost::beast::http::verb, to the bmcweb index.
-static constexpr size_t maxVerbIndex =
- static_cast<size_t>(boost::beast::http::verb::patch);
+static constexpr size_t maxVerbIndex = static_cast<size_t>(HttpVerb::Max) - 1U;
// MaxVerb + 1 is designated as the "not found" verb. It is done this way
// to keep the BaseRule as a single bitfield (thus keeping the struct small)
@@ -107,8 +103,7 @@
return false;
}
- size_t methodsBitfield{
- 1 << static_cast<size_t>(boost::beast::http::verb::get)};
+ size_t methodsBitfield{1 << static_cast<size_t>(HttpVerb::Get)};
static_assert(std::numeric_limits<decltype(methodsBitfield)>::digits >
methodNotAllowedIndex,
"Not enough bits to store bitfield");
@@ -445,7 +440,11 @@
self_t& methods(boost::beast::http::verb method)
{
self_t* self = static_cast<self_t*>(this);
- self->methodsBitfield = 1U << static_cast<size_t>(method);
+ std::optional<HttpVerb> verb = httpVerbFromBoost(method);
+ if (verb)
+ {
+ self->methodsBitfield = 1U << static_cast<size_t>(*verb);
+ }
return *self;
}
@@ -454,7 +453,11 @@
{
self_t* self = static_cast<self_t*>(this);
methods(argsMethod...);
- self->methodsBitfield |= 1U << static_cast<size_t>(method);
+ std::optional<HttpVerb> verb = httpVerbFromBoost(method);
+ if (verb)
+ {
+ self->methodsBitfield |= 1U << static_cast<size_t>(*verb);
+ }
return *self;
}
@@ -1210,7 +1213,12 @@
{
FindRouteResponse findRoute;
- size_t reqMethodIndex = static_cast<size_t>(req.method());
+ std::optional<HttpVerb> verb = httpVerbFromBoost(req.method());
+ if (!verb)
+ {
+ return findRoute;
+ }
+ size_t reqMethodIndex = static_cast<size_t>(*verb);
// Check to see if this url exists at any verb
for (size_t perMethodIndex = 0; perMethodIndex <= maxVerbIndex;
perMethodIndex++)
@@ -1227,8 +1235,8 @@
{
findRoute.allowHeader += ", ";
}
- findRoute.allowHeader += boost::beast::http::to_string(
- static_cast<boost::beast::http::verb>(perMethodIndex));
+ HttpVerb thisVerb = static_cast<HttpVerb>(perMethodIndex);
+ findRoute.allowHeader += httpVerbToString(thisVerb);
if (perMethodIndex == reqMethodIndex)
{
findRoute.route = route;
@@ -1240,14 +1248,14 @@
template <typename Adaptor>
void handleUpgrade(const Request& req, Response& res, Adaptor&& adaptor)
{
- if (static_cast<size_t>(req.method()) >= perMethods.size())
+ std::optional<HttpVerb> verb = httpVerbFromBoost(req.method());
+ if (!verb || static_cast<size_t>(*verb) >= perMethods.size())
{
res.result(boost::beast::http::status::not_found);
res.end();
return;
}
-
- PerMethod& perMethod = perMethods[static_cast<size_t>(req.method())];
+ PerMethod& perMethod = perMethods[static_cast<size_t>(*verb)];
Trie& trie = perMethod.trie;
std::vector<BaseRule*>& rules = perMethod.rules;
@@ -1309,7 +1317,8 @@
void handle(Request& req,
const std::shared_ptr<bmcweb::AsyncResp>& asyncResp)
{
- if (static_cast<size_t>(req.method()) >= perMethods.size())
+ std::optional<HttpVerb> verb = httpVerbFromBoost(req.method());
+ if (!verb || static_cast<size_t>(*verb) >= perMethods.size())
{
asyncResp->res.result(boost::beast::http::status::not_found);
return;
diff --git a/http/verb.hpp b/http/verb.hpp
new file mode 100644
index 0000000..33dd569
--- /dev/null
+++ b/http/verb.hpp
@@ -0,0 +1,69 @@
+#pragma once
+
+#include <boost/beast/http/verb.hpp>
+
+#include <optional>
+#include <string_view>
+
+enum class HttpVerb
+{
+ Delete = 0,
+ Get,
+ Head,
+ Options,
+ Patch,
+ Post,
+ Put,
+ Max,
+};
+
+inline std::optional<HttpVerb> httpVerbFromBoost(boost::beast::http::verb bv)
+{
+ switch (bv)
+ {
+ case boost::beast::http::verb::delete_:
+ return HttpVerb::Delete;
+ case boost::beast::http::verb::get:
+ return HttpVerb::Get;
+ case boost::beast::http::verb::head:
+ return HttpVerb::Head;
+ case boost::beast::http::verb::options:
+ return HttpVerb::Options;
+ case boost::beast::http::verb::patch:
+ return HttpVerb::Patch;
+ case boost::beast::http::verb::post:
+ return HttpVerb::Post;
+ case boost::beast::http::verb::put:
+ return HttpVerb::Put;
+ default:
+ return std::nullopt;
+ }
+
+ return std::nullopt;
+}
+
+inline std::string_view httpVerbToString(HttpVerb verb)
+{
+ switch (verb)
+ {
+ case HttpVerb::Delete:
+ return "DELETE";
+ case HttpVerb::Get:
+ return "GET";
+ case HttpVerb::Head:
+ return "HEAD";
+ case HttpVerb::Patch:
+ return "PATCH";
+ case HttpVerb::Post:
+ return "POST";
+ case HttpVerb::Put:
+ return "PUT";
+ case HttpVerb::Options:
+ return "OPTIONS";
+ case HttpVerb::Max:
+ return "";
+ }
+
+ // Should never reach here
+ return "";
+}
diff --git a/meson.build b/meson.build
index 2074248..f62da1e 100644
--- a/meson.build
+++ b/meson.build
@@ -357,6 +357,7 @@
'test/http/crow_getroutes_test.cpp',
'test/http/router_test.cpp',
'test/http/utility_test.cpp',
+ 'test/http/verb_test.cpp',
'test/include/dbus_utility_test.cpp',
'test/include/google/google_service_root_test.cpp',
'test/include/http_utility_test.cpp',
diff --git a/test/http/verb_test.cpp b/test/http/verb_test.cpp
new file mode 100644
index 0000000..76dd8d0
--- /dev/null
+++ b/test/http/verb_test.cpp
@@ -0,0 +1,56 @@
+#include "verb.hpp"
+
+#include <boost/beast/http/verb.hpp>
+
+#include <map>
+#include <optional>
+#include <string_view>
+
+#include <gtest/gtest.h> // IWYU pragma: keep
+
+using BoostVerb = boost::beast::http::verb;
+
+TEST(BoostToHttpVerb, ValidCase)
+{
+ std::map<HttpVerb, BoostVerb> verbMap = {
+ {HttpVerb::Delete, BoostVerb::delete_},
+ {HttpVerb::Get, BoostVerb::get},
+ {HttpVerb::Head, BoostVerb::head},
+ {HttpVerb::Options, BoostVerb::options},
+ {HttpVerb::Patch, BoostVerb::patch},
+ {HttpVerb::Post, BoostVerb::post},
+ {HttpVerb::Put, BoostVerb::put},
+ };
+
+ for (int verbIndex = 0; verbIndex < static_cast<int>(HttpVerb::Max);
+ ++verbIndex)
+ {
+ HttpVerb httpVerb = static_cast<HttpVerb>(verbIndex);
+ std::optional<HttpVerb> verb = httpVerbFromBoost(verbMap[httpVerb]);
+ ASSERT_TRUE(verb.has_value());
+ EXPECT_EQ(*verb, httpVerb);
+ }
+}
+
+TEST(BoostToHttpVerbTest, InvalidCase)
+{
+ std::optional<HttpVerb> verb = httpVerbFromBoost(BoostVerb::unknown);
+ EXPECT_FALSE(verb.has_value());
+}
+
+TEST(HttpVerbToStringTest, ValidCase)
+{
+ std::map<HttpVerb, std::string_view> verbMap = {
+ {HttpVerb::Delete, "DELETE"}, {HttpVerb::Get, "GET"},
+ {HttpVerb::Head, "HEAD"}, {HttpVerb::Options, "OPTIONS"},
+ {HttpVerb::Patch, "PATCH"}, {HttpVerb::Post, "POST"},
+ {HttpVerb::Put, "PUT"},
+ };
+
+ for (int verbIndex = 0; verbIndex < static_cast<int>(HttpVerb::Max);
+ ++verbIndex)
+ {
+ HttpVerb httpVerb = static_cast<HttpVerb>(verbIndex);
+ EXPECT_EQ(httpVerbToString(httpVerb), verbMap[httpVerb]);
+ }
+}
\ No newline at end of file