chore: get rid of MutableSlice (#3952)

* chore: get rid of MutableSlice Signed-off-by: Roman Gershman <[email protected]> * chore: comments --------- Signed-off-by: Roman Gershman <[email protected]>
dragonflydb · Oct 23, 2024 · 4aa0ca1 · 4aa0ca1
1 parent 0ebc1a1
commit 4aa0ca1
Show file tree

Hide file tree

Showing 33 changed files with 95 additions and 147 deletions.
diff --git a/src/core/core_types.h b/src/core/core_types.h
diff --git a/src/core/interpreter.cc b/src/core/interpreter.cc
@@ -244,7 +244,7 @@ optional<int> FetchKey(lua_State* lua, const char* key) {
   return type;
 }
 
-void SetGlobalArrayInternal(lua_State* lua, const char* name, MutSliceSpan args) {
+void SetGlobalArrayInternal(lua_State* lua, const char* name, Interpreter::SliceSpan args) {
   lua_createtable(lua, args.size(), 0);
   for (size_t j = 0; j < args.size(); j++) {
     lua_pushlstring(lua, args[j].data(), args[j].size());
@@ -652,7 +652,7 @@ auto Interpreter::RunFunction(string_view sha, std::string* error) -> RunResult
   return err == 0 ? RUN_OK : RUN_ERR;
 }
 
-void Interpreter::SetGlobalArray(const char* name, MutSliceSpan args) {
+void Interpreter::SetGlobalArray(const char* name, SliceSpan args) {
   SetGlobalArrayInternal(lua_, name, args);
 }
 
@@ -952,7 +952,7 @@ int Interpreter::RedisGenericCommand(bool raise_error, bool async, ObjectExplore
   }
 
   char name_buffer[32];  // backing storage for cmd name
-  absl::FixedArray<absl::Span<char>, 4> args(argc);
+  absl::FixedArray<string_view, 4> args(argc);
 
   // Copy command name to name_buffer and set it as first arg.
   unsigned name_len = lua_rawlen(lua_, 1);
@@ -1004,7 +1004,7 @@ int Interpreter::RedisGenericCommand(bool raise_error, bool async, ObjectExplore
     explorer = &*translator;
   }
 
-  redis_func_(CallArgs{MutSliceSpan{args}, &buffer_, explorer, async, raise_error, &raise_error});
+  redis_func_(CallArgs{SliceSpan{args}, &buffer_, explorer, async, raise_error, &raise_error});
   cmd_depth_--;
 
   // Shrink reusable buffer if it's too big.

diff --git a/src/core/interpreter.h b/src/core/interpreter.h
@@ -4,11 +4,12 @@
 
 #pragma once
 
+#include <absl/types/span.h>
+
 #include <functional>
 #include <optional>
 #include <string_view>
 
-#include "core/core_types.h"
 #include "util/fibers/synchronization.h"
 
 typedef struct lua_State lua_State;
@@ -40,10 +41,12 @@ class ObjectExplorer {
 
 class Interpreter {
  public:
+  using SliceSpan = absl::Span<const std::string_view>;
+
   // Arguments received from redis.call
   struct CallArgs {
     // Full arguments, including cmd name.
-    MutSliceSpan args;
+    SliceSpan args;
 
     // Pointer to backing storage for args (excluding cmd name).
     // Moving can invalidate arg slice pointers. Moved by async to re-use buffer.
@@ -93,7 +96,7 @@ class Interpreter {
     RUN_ERR = 2,
   };
 
-  void SetGlobalArray(const char* name, MutSliceSpan args);
+  void SetGlobalArray(const char* name, SliceSpan args);
 
   // Runs already added function sha returned by a successful call to AddFunction().
   // Returns: true if the call succeeded, otherwise fills error and returns false.

diff --git a/src/core/interpreter_test.cc b/src/core/interpreter_test.cc
@@ -73,6 +73,7 @@ class TestSerializer : public ObjectExplorer {
   }
 };
 
+using SliceSpan = Interpreter::SliceSpan;
 class InterpreterTest : public ::testing::Test {
  protected:
   InterpreterTest() {
@@ -99,12 +100,12 @@ class InterpreterTest : public ::testing::Test {
 };
 
 void InterpreterTest::SetGlobalArray(const char* name, const vector<string_view>& vec) {
-  vector<MutableSlice> slices(vec.size());
+  vector<string_view> slices(vec.size());
   for (size_t i = 0; i < vec.size(); ++i) {
     strings_.emplace_back(new string(vec[i]));
-    slices[i] = MutableSlice{*strings_.back()};
+    slices[i] = string_view{*strings_.back()};
   }
-  intptr_.SetGlobalArray(name, MutSliceSpan{slices});
+  intptr_.SetGlobalArray(name, SliceSpan{slices});
 }
 
 bool InterpreterTest::Execute(string_view script) {
@@ -329,7 +330,7 @@ TEST_F(InterpreterTest, CallArray) {
 
 TEST_F(InterpreterTest, ArgKeys) {
   vector<string> vec_arr{};
-  vector<MutableSlice> slices;
+  vector<string_view> slices;
   SetGlobalArray("ARGV", {"foo", "bar"});
   SetGlobalArray("KEYS", {"key1", "key2"});
   EXPECT_TRUE(Execute("return {ARGV[1], KEYS[1], KEYS[2]}"));

diff --git a/src/core/search/base.h b/src/core/search/base.h
@@ -14,7 +14,6 @@
 #include <vector>
 
 #include "base/pmr/memory_resource.h"
-#include "core/core_types.h"
 #include "core/string_map.h"
 
 namespace dfly::search {

diff --git a/src/core/small_string.h b/src/core/small_string.h
@@ -6,8 +6,6 @@
 #include <cstdint>
 #include <string_view>
 
-#include "core/core_types.h"
-
 namespace dfly {
 
 // blob strings of upto ~256B. Small sizes are probably predominant

diff --git a/src/core/uring.h b/src/core/uring.h
diff --git a/src/facade/dragonfly_connection.cc b/src/facade/dragonfly_connection.cc
@@ -17,7 +17,6 @@
 #include "base/io_buf.h"
 #include "base/logging.h"
 #include "core/heap_size.h"
-#include "core/uring.h"
 #include "facade/conn_context.h"
 #include "facade/dragonfly_listener.h"
 #include "facade/memcache_parser.h"
@@ -30,6 +29,10 @@
 #include "util/tls/tls_socket.h"
 #endif
 
+#ifdef __linux__
+#include "util/fibers/uring_file.h"
+#endif
+
 using namespace std;
 using facade::operator""_MB;
 
@@ -1341,7 +1344,7 @@ bool Connection::ShouldEndDispatchFiber(const MessageHandle& msg) {
 void Connection::SquashPipeline() {
   DCHECK_EQ(dispatch_q_.size(), pending_pipeline_cmd_cnt_);
 
-  vector<CmdArgList> squash_cmds;
+  vector<ArgSlice> squash_cmds;
   squash_cmds.reserve(dispatch_q_.size());
 
   for (auto& msg : dispatch_q_) {

diff --git a/src/facade/dragonfly_connection.h b/src/facade/dragonfly_connection.h
@@ -91,7 +91,7 @@ class Connection : public util::Connection {
     // The capacity is chosen so that we allocate a fully utilized (256 bytes) block.
     using StorageType = absl::InlinedVector<char, kReqStorageSize, mi_stl_allocator<char>>;
 
-    absl::InlinedVector<MutableSlice, 6> args;
+    absl::InlinedVector<std::string_view, 6> args;
     StorageType storage;
   };
 

diff --git a/src/facade/facade_types.h b/src/facade/facade_types.h
@@ -35,16 +35,12 @@ constexpr size_t kSanitizerOverhead = 0u;
 
 enum class Protocol : uint8_t { MEMCACHE = 1, REDIS = 2 };
 
-using MutableSlice = absl::Span<char>;
-using CmdArgList = absl::Span<MutableSlice>;
-using CmdArgVec = std::vector<MutableSlice>;
+using MutableSlice = std::string_view;
+using CmdArgList = absl::Span<const std::string_view>;
+using CmdArgVec = std::vector<std::string_view>;
 using ArgSlice = absl::Span<const std::string_view>;
 using OwnedArgSlice = absl::Span<const std::string>;
 
-inline std::string_view ToSV(MutableSlice slice) {
-  return std::string_view{slice.data(), slice.size()};
-}
-
 inline std::string_view ToSV(std::string_view slice) {
   return slice;
 }
@@ -57,13 +53,8 @@ inline std::string_view ToSV(std::string&& slice) = delete;
 
 constexpr auto kToSV = [](auto&& v) { return ToSV(std::forward<decltype(v)>(v)); };
 
-inline std::string_view ArgS(CmdArgList args, size_t i) {
-  auto arg = args[i];
-  return {arg.data(), arg.size()};
-}
-
-inline auto ArgS(CmdArgList args) {
-  return base::it::Transform(kToSV, base::it::Range{args.begin(), args.end()});
+inline std::string_view ArgS(ArgSlice args, size_t i) {
+  return args[i];
 }
 
 struct ArgRange {
@@ -95,7 +86,7 @@ struct ArgRange {
     return std::visit([idx](const auto& span) { return facade::ToSV(span[idx]); }, span);
   }
 
-  std::variant<CmdArgList, ArgSlice, OwnedArgSlice> span;
+  std::variant<ArgSlice, OwnedArgSlice> span;
 };
 struct ConnectionStats {
   size_t read_buf_capacity = 0;                // total capacity of input buffers
@@ -179,10 +170,6 @@ struct ErrorReply {
   std::optional<OpStatus> status{std::nullopt};
 };
 
-inline MutableSlice ToMSS(absl::Span<uint8_t> span) {
-  return MutableSlice{reinterpret_cast<char*>(span.data()), span.size()};
-}
-
 constexpr inline unsigned long long operator""_MB(unsigned long long x) {
   return 1024L * 1024L * x;
 }

diff --git a/src/facade/ok_main.cc b/src/facade/ok_main.cc
@@ -21,11 +21,11 @@ namespace {
 
 class OkService : public ServiceInterface {
  public:
-  void DispatchCommand(CmdArgList args, ConnectionContext* cntx) final {
+  void DispatchCommand(ArgSlice args, ConnectionContext* cntx) final {
     cntx->SendOk();
   }
 
-  size_t DispatchManyCommands(absl::Span<CmdArgList> args_lists, ConnectionContext* cntx) final {
+  size_t DispatchManyCommands(absl::Span<ArgSlice> args_lists, ConnectionContext* cntx) final {
     for (auto args : args_lists)
       DispatchCommand(args, cntx);
     return args_lists.size();

diff --git a/src/facade/resp_expr.cc b/src/facade/resp_expr.cc
@@ -13,7 +13,7 @@ void RespExpr::VecToArgList(const Vec& src, CmdArgVec* dest) {
   for (size_t i = 0; i < src.size(); ++i) {
     DCHECK(src[i].type == RespExpr::STRING);
 
-    (*dest)[i] = ToMSS(src[i].GetBuf());
+    (*dest)[i] = ToSV(src[i].GetBuf());
   }
 }
 

diff --git a/src/facade/service_interface.h b/src/facade/service_interface.h
@@ -25,11 +25,10 @@ class ServiceInterface {
   virtual ~ServiceInterface() {
   }
 
-  virtual void DispatchCommand(CmdArgList args, ConnectionContext* cntx) = 0;
+  virtual void DispatchCommand(ArgSlice args, ConnectionContext* cntx) = 0;
 
   // Returns number of processed commands
-  virtual size_t DispatchManyCommands(absl::Span<CmdArgList> args_list,
-                                      ConnectionContext* cntx) = 0;
+  virtual size_t DispatchManyCommands(absl::Span<ArgSlice> args_list, ConnectionContext* cntx) = 0;
 
   virtual void DispatchMC(const MemcacheParser::Command& cmd, std::string_view value,
                           ConnectionContext* cntx) = 0;

diff --git a/src/server/acl/validator.cc b/src/server/acl/validator.cc
@@ -18,7 +18,7 @@ extern "C" {
 namespace dfly::acl {
 
 [[nodiscard]] bool IsUserAllowedToInvokeCommand(const ConnectionContext& cntx, const CommandId& id,
-                                                CmdArgList tail_args) {
+                                                ArgSlice tail_args) {
   if (cntx.skip_acl_validation) {
     return true;
   }

diff --git a/src/server/command_registry.cc b/src/server/command_registry.cc
@@ -189,8 +189,8 @@ CommandRegistry::FamiliesVec CommandRegistry::GetFamilies() {
   return std::move(family_of_commands_);
 }
 
-std::pair<const CommandId*, CmdArgList> CommandRegistry::FindExtended(string_view cmd,
-                                                                      CmdArgList tail_args) const {
+std::pair<const CommandId*, ArgSlice> CommandRegistry::FindExtended(string_view cmd,
+                                                                    ArgSlice tail_args) const {
   if (cmd == RenamedOrOriginal("ACL"sv)) {
     if (tail_args.empty()) {
       return {Find(cmd), {}};

diff --git a/src/server/command_registry.h b/src/server/command_registry.h
@@ -203,8 +203,8 @@ class CommandRegistry {
   using FamiliesVec = std::vector<std::vector<std::string>>;
   FamiliesVec GetFamilies();
 
-  std::pair<const CommandId*, facade::CmdArgList> FindExtended(std::string_view cmd,
-                                                               facade::CmdArgList tail_args) const;
+  std::pair<const CommandId*, facade::ArgSlice> FindExtended(std::string_view cmd,
+                                                             facade::ArgSlice tail_args) const;
 
  private:
   absl::flat_hash_map<std::string, CommandId> cmd_map_;

diff --git a/src/server/conn_context.cc b/src/server/conn_context.cc
@@ -21,7 +21,7 @@ namespace dfly {
 using namespace std;
 using namespace facade;
 
-StoredCmd::StoredCmd(const CommandId* cid, CmdArgList args, facade::ReplyMode mode)
+StoredCmd::StoredCmd(const CommandId* cid, ArgSlice args, facade::ReplyMode mode)
     : cid_{cid}, buffer_{}, sizes_(args.size()), reply_mode_{mode} {
   size_t total_size = 0;
   for (auto args : args) {
@@ -38,7 +38,7 @@ StoredCmd::StoredCmd(const CommandId* cid, CmdArgList args, facade::ReplyMode mo
   }
 }
 
-StoredCmd::StoredCmd(string&& buffer, const CommandId* cid, CmdArgList args, facade::ReplyMode mode)
+StoredCmd::StoredCmd(string&& buffer, const CommandId* cid, ArgSlice args, facade::ReplyMode mode)
     : cid_{cid}, buffer_{std::move(buffer)}, sizes_(args.size()), reply_mode_{mode} {
   for (unsigned i = 0; i < args.size(); i++) {
     // Assume tightly packed list.
@@ -47,7 +47,7 @@ StoredCmd::StoredCmd(string&& buffer, const CommandId* cid, CmdArgList args, fac
   }
 }
 
-void StoredCmd::Fill(CmdArgList args) {
+void StoredCmd::Fill(absl::Span<std::string_view> args) {
   DCHECK_GE(args.size(), sizes_.size());
 
   unsigned offset = 0;
@@ -162,7 +162,7 @@ vector<unsigned> ChangeSubscriptions(bool pattern, CmdArgList args, bool to_add,
 
   // Gather all the channels we need to subscribe to / remove.
   size_t i = 0;
-  for (string_view channel : ArgS(args)) {
+  for (string_view channel : args) {
     if (to_add && local_store.emplace(channel).second)
       csu.Record(channel);
     else if (!to_add && local_store.erase(channel) > 0)

diff --git a/src/server/conn_context.h b/src/server/conn_context.h
@@ -27,11 +27,10 @@ struct FlowInfo;
 // Used for storing MULTI/EXEC commands.
 class StoredCmd {
  public:
-  StoredCmd(const CommandId* cid, CmdArgList args,
-            facade::ReplyMode mode = facade::ReplyMode::FULL);
+  StoredCmd(const CommandId* cid, ArgSlice args, facade::ReplyMode mode = facade::ReplyMode::FULL);
 
   // Create on top of already filled tightly-packed buffer.
-  StoredCmd(std::string&& buffer, const CommandId* cid, CmdArgList args,
+  StoredCmd(std::string&& buffer, const CommandId* cid, ArgSlice args,
             facade::ReplyMode mode = facade::ReplyMode::FULL);
 
   size_t NumArgs() const;
@@ -40,7 +39,7 @@ class StoredCmd {
 
   // Fill the arg list with stored arguments, it should be at least of size NumArgs().
   // Between filling and invocation, cmd should NOT be moved.
-  void Fill(CmdArgList args);
+  void Fill(absl::Span<std::string_view> args);
 
   void Fill(CmdArgVec* dest) {
     dest->resize(sizes_.size());

diff --git a/src/server/debugcmd.cc b/src/server/debugcmd.cc
@@ -145,7 +145,7 @@ void DoPopulateBatch(string_view type, string_view prefix, size_t val_size, bool
   local_tx->StartMultiNonAtomic();
   boost::intrusive_ptr<Transaction> stub_tx =
       new Transaction{local_tx.get(), EngineShard::tlocal()->shard_id(), nullopt};
-  absl::InlinedVector<MutableSlice, 5> args_view;
+  absl::InlinedVector<string_view, 5> args_view;
   facade::CapturingReplyBuilder crb;
   ConnectionContext local_cntx{cntx, stub_tx.get(), &crb};
 
@@ -162,7 +162,7 @@ void DoPopulateBatch(string_view type, string_view prefix, size_t val_size, bool
 
     args_view.clear();
     for (auto& arg : args) {
-      args_view.push_back(absl::MakeSpan(arg));
+      args_view.push_back(arg);
     }
     auto args_span = absl::MakeSpan(args_view);