Skip to content

Commit 6ec47d4

Browse files
romangeromange
authored
chore: add ability for ParsedArgs to reference BackedArguments (#6171)
* chore: add ability for ParsedArgs to reference BackedArguments Reimplement StoredCmd using ParsedArgs and BackedArguments. This reduces code duplication as well as paves the path to use BackedArguments during pipeline squashing. Needed to adjust test_lua_auto_async test as we had accounting bug in StoredCmd::UsedMemory function --------- Signed-off-by: Roman Gershman <roman@dragonflydb.io>
1 parent 8d0154d commit 6ec47d4

File tree

9 files changed

+202
-121
lines changed

9 files changed

+202
-121
lines changed

‎.github/actions/regression-tests/action.yml‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ inputs:
3131
epoll:
3232
required: false
3333
type: string
34-
3534
runs:
3635
using: "composite"
3736
# bring back timeouts once composite actions start supporting them
@@ -41,7 +40,14 @@ runs:
4140
shell: bash
4241
run: |
4342
cd ${GITHUB_WORKSPACE}/tests/dragonfly/valkey_search
44-
./sync-valkey-search-tests.sh 90124dc91756b24cb2e58e5c4eea5b8d53004ea6 # main branch revision
43+
# main branch revision
44+
./sync-valkey-search-tests.sh 90124dc91756b24cb2e58e5c4eea5b8d53004ea6
45+
- name: Prepare Step
46+
shell: bash
47+
run: |
48+
DOCKER_VERSION="26.1.4"
49+
curl -fsSL "https://download.docker.com/linux/static/stable/x86_64/docker-${DOCKER_VERSION}.tgz" | tar xz --strip 1 docker/docker -C ./docker
50+
registry="public.ecr.aws/docker/library" && for image in fedora:latest ubuntu:noble; do ./docker pull "$registry/$image"; done
4551
- name: Run PyTests
4652
id: main
4753
shell: bash

‎src/common/backed_args.h‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// Copyright 2025, DragonflyDB authors. All rights reserved.
2+
// See LICENSE for licensing terms.
3+
//
4+
5+
#pragma once
6+
7+
#include <absl/container/inlined_vector.h>
8+
9+
#include <string_view>
10+
11+
namespace cmn {
12+
13+
class BackedArguments {
14+
constexpr static size_t kLenCap = 5;
15+
constexpr static size_t kStorageCap = 88;
16+
17+
public:
18+
BackedArguments() {
19+
}
20+
21+
template <typename I> BackedArguments(I begin, I end);
22+
23+
size_t HeapMemory() const {
24+
size_t s1 = lens_.capacity() <= kLenCap ? 0 : lens_.capacity() * sizeof(uint32_t);
25+
size_t s2 = storage_.capacity() <= kStorageCap ? 0 : storage_.capacity();
26+
return s1 + s2;
27+
}
28+
29+
// The capacity is chosen so that we allocate a fully utilized (128 bytes) block.
30+
using StorageType = absl::InlinedVector<char, kStorageCap>;
31+
32+
std::string_view Front() const {
33+
return std::string_view{storage_.data(), lens_[0]};
34+
}
35+
36+
size_t size() const {
37+
return lens_.size();
38+
}
39+
40+
size_t elem_len(size_t i) const {
41+
return lens_[i];
42+
}
43+
44+
size_t elem_capacity(size_t i) const {
45+
return elem_len(i) + 1;
46+
}
47+
48+
std::string_view at(uint32_t index, uint32_t offset) const {
49+
const char* ptr = storage_.data() + offset;
50+
return std::string_view{ptr, lens_[index]};
51+
}
52+
53+
protected:
54+
absl::InlinedVector<uint32_t, kLenCap> lens_;
55+
StorageType storage_;
56+
};
57+
58+
static_assert(sizeof(BackedArguments) == 128);
59+
60+
template <typename I> BackedArguments::BackedArguments(I begin, I end) {
61+
lens_.reserve(end - begin);
62+
size_t total_size = 0;
63+
for (auto it = begin; it != end; ++it) {
64+
lens_.push_back(it->size());
65+
total_size += it->size() + 1; // +1 for '\0'
66+
}
67+
storage_.resize(total_size);
68+
69+
char* next = storage_.data();
70+
for (auto it = begin; it != end; ++it) {
71+
if (!it->empty()) {
72+
memcpy(next, it->data(), it->size());
73+
}
74+
next[it->size()] = '\0';
75+
next += it->size() + 1;
76+
}
77+
}
78+
79+
} // namespace cmn

‎src/facade/facade_types.h‎

Lines changed: 66 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include <variant>
1414

1515
#include "base/iterator.h"
16+
#include "common/backed_args.h"
1617
#include "facade/op_status.h"
1718

1819
namespace facade {
@@ -45,31 +46,88 @@ class ParsedArgs {
4546
public:
4647
ParsedArgs() = default;
4748

48-
explicit ParsedArgs(ArgSlice args) : args_(args) { // NOLINT google-explicit-constructor
49+
// References backed arguments. The object must outlive this ParsedArgs.
50+
ParsedArgs(const cmn::BackedArguments& bargs) // NOLINT google-explicit-constructor
51+
: args_(&bargs) {
4952
}
5053

54+
ParsedArgs(ArgSlice slice) // NOLINT google-explicit-constructor
55+
: args_(slice) {
56+
}
57+
58+
ParsedArgs(const ParsedArgs& other) = default;
59+
ParsedArgs& operator=(const ParsedArgs& bargs) = default;
60+
5161
size_t size() const {
52-
return args_.size();
62+
return std::visit([](const auto& args) { return args.size(); }, args_);
5363
}
5464

5565
bool empty() const {
56-
return args_.empty();
66+
return size() == 0;
5767
}
5868

5969
ParsedArgs Tail() const {
60-
return ParsedArgs{args_.subspan(1)};
70+
return std::visit([](const auto& args) { return args.Tail(); }, args_);
6171
}
6272

6373
std::string_view Front() const {
64-
return args_.front();
74+
return std::visit([](const auto& args) { return args.front(); }, args_);
6575
}
6676

67-
ArgSlice ToSlice() const {
68-
return args_;
77+
ArgSlice ToSlice(CmdArgVec* scratch) const {
78+
return std::visit([scratch](const auto& args) { return args.ToSlice(scratch); }, args_);
6979
}
7080

7181
private:
72-
absl::Span<const std::string_view> args_;
82+
struct WrapperBacked {
83+
WrapperBacked(const cmn::BackedArguments* args) : args_(args) { // NOLINT
84+
}
85+
86+
const cmn::BackedArguments* args_;
87+
uint32_t index_ = 0;
88+
uint32_t ofs_bytes_ = 0;
89+
90+
ParsedArgs Tail() const {
91+
ParsedArgs res(*args_);
92+
WrapperBacked* wb = std::get_if<WrapperBacked>(&res.args_);
93+
wb->index_ = index_ + 1;
94+
wb->ofs_bytes_ = ofs_bytes_ + args_->elem_capacity(index_);
95+
return res;
96+
};
97+
98+
size_t size() const {
99+
return args_->size() - index_;
100+
}
101+
102+
std::string_view front() const {
103+
return args_->at(index_, ofs_bytes_);
104+
}
105+
106+
ArgSlice ToSlice(CmdArgVec* scratch) const {
107+
scratch->resize(size());
108+
size_t offset = ofs_bytes_;
109+
for (size_t i = 0; i < scratch->size(); ++i) {
110+
(*scratch)[i] = args_->at(index_ + i, offset);
111+
offset += args_->elem_capacity(index_ + i);
112+
}
113+
return ArgSlice{scratch->data(), scratch->size()};
114+
}
115+
};
116+
117+
struct Slice : public ArgSlice {
118+
using ArgSlice::ArgSlice;
119+
Slice(ArgSlice other) : ArgSlice(other) { // NOLINT
120+
}
121+
122+
ParsedArgs Tail() const {
123+
return ParsedArgs{subspan(1)};
124+
}
125+
126+
ArgSlice ToSlice(std::vector<std::string_view>* /*scratch*/) const {
127+
return *this;
128+
}
129+
};
130+
std::variant<Slice, WrapperBacked> args_;
73131
};
74132

75133
inline std::string_view ToSV(std::string_view slice) {

‎src/server/conn_context.cc‎

Lines changed: 8 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -34,88 +34,21 @@ static void SendSubscriptionChangedResponse(string_view action, std::optional<st
3434
rb->SendLong(count);
3535
}
3636

37-
StoredCmd::StoredCmd(const CommandId* cid, bool own_args, ArgSlice args)
38-
: cid_{cid}, args_{args}, reply_mode_{facade::ReplyMode::FULL} {
39-
if (!own_args)
40-
return;
41-
42-
auto& own_storage = args_.emplace<OwnStorage>(args.size());
43-
size_t total_size = 0;
44-
for (auto args : args) {
45-
total_size += args.size();
46-
}
47-
own_storage.buffer.resize(total_size);
48-
char* next = own_storage.buffer.data();
49-
for (unsigned i = 0; i < args.size(); i++) {
50-
if (!args[i].empty())
51-
memcpy(next, args[i].data(), args[i].size());
52-
next += args[i].size();
53-
own_storage.sizes[i] = args[i].size();
54-
}
55-
}
56-
57-
StoredCmd::StoredCmd(string&& buffer, const CommandId* cid, ArgSlice args, facade::ReplyMode mode)
58-
: cid_{cid}, args_{OwnStorage{args.size()}}, reply_mode_{mode} {
59-
OwnStorage& own_storage = std::get<OwnStorage>(args_);
60-
own_storage.buffer = std::move(buffer);
61-
62-
for (unsigned i = 0; i < args.size(); i++) {
63-
// Assume tightly packed list.
64-
DCHECK(i + 1 == args.size() || args[i].data() + args[i].size() == args[i + 1].data());
65-
own_storage.sizes[i] = args[i].size();
66-
}
37+
StoredCmd::StoredCmd(const CommandId* cid, facade::ArgSlice args, facade::ReplyMode mode)
38+
: cid_{cid}, args_{args}, reply_mode_{mode} {
39+
backed_ = std::make_unique<cmn::BackedArguments>(args.begin(), args.end());
40+
args_ = facade::ParsedArgs{*backed_};
6741
}
6842

6943
CmdArgList StoredCmd::ArgList(CmdArgVec* scratch) const {
70-
return std::visit(
71-
Overloaded{[&](const OwnStorage& s) {
72-
unsigned offset = 0;
73-
scratch->resize(s.sizes.size());
74-
for (unsigned i = 0; i < s.sizes.size(); i++) {
75-
(*scratch)[i] = string_view{s.buffer.data() + offset, s.sizes[i]};
76-
offset += s.sizes[i];
77-
}
78-
return CmdArgList{*scratch};
79-
},
80-
[&](const CmdArgList& s) { return s; }},
81-
args_);
44+
return args_.ToSlice(scratch);
8245
}
8346

8447
std::string StoredCmd::FirstArg() const {
8548
if (NumArgs() == 0) {
8649
return {};
8750
}
88-
return std::visit(Overloaded{[&](const OwnStorage& s) { return s.buffer.substr(0, s.sizes[0]); },
89-
[&](const ArgSlice& s) {
90-
return std::string{s[0].data(), s[0].size()};
91-
}},
92-
args_);
93-
}
94-
95-
facade::ReplyMode StoredCmd::ReplyMode() const {
96-
return reply_mode_;
97-
}
98-
99-
template <typename C> size_t IsStoredInlined(const C& c) {
100-
const char* start = reinterpret_cast<const char*>(&c);
101-
const char* end = start + sizeof(C);
102-
const char* data = reinterpret_cast<const char*>(c.data());
103-
return data >= start && data <= end;
104-
}
105-
106-
size_t StoredCmd::UsedMemory() const {
107-
return std::visit(Overloaded{[&](const OwnStorage& s) {
108-
size_t buffer_size =
109-
IsStoredInlined(s.buffer) ? 0 : s.buffer.capacity();
110-
size_t sz_size = IsStoredInlined(s.sizes) ? 0 : s.sizes.memsize();
111-
return buffer_size + sz_size;
112-
},
113-
[&](const ArgSlice&) -> size_t { return 0U; }},
114-
args_);
115-
}
116-
117-
const CommandId* StoredCmd::Cid() const {
118-
return cid_;
51+
return string{args_.Front()};
11952
}
12053

12154
ConnectionContext::ConnectionContext(facade::Connection* owner, acl::UserCredentials cred)
@@ -232,8 +165,8 @@ size_t ConnectionState::ExecInfo::UsedMemory() const {
232165
return HeapSize(body) + HeapSize(watched_keys);
233166
}
234167

235-
void ConnectionState::ExecInfo::AddStoredCmd(const CommandId* cid, bool own_args, CmdArgList args) {
236-
body.emplace_back(cid, own_args, args);
168+
void ConnectionState::ExecInfo::AddStoredCmd(const CommandId* cid, ArgSlice args) {
169+
body.emplace_back(cid, args);
237170
stored_cmd_bytes += body.back().UsedMemory();
238171
}
239172

0 commit comments

Comments
 (0)