Skip to content

Commit a7c8195

Browse files
rzarzynskirzarzynski
committed
kv: avoid memcpy around key() in OMAP iterator of KeyValueDB
Signed-off-by: Radoslaw Zarzynski <[email protected]>
1 parent d2531a0 commit a7c8195

File tree

6 files changed

+118
-6
lines changed

6 files changed

+118
-6
lines changed

‎src/kv/KeyValueDB.h‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,9 @@ class KeyValueDB {
206206
return "";
207207
}
208208
virtual ceph::buffer::list value() = 0;
209+
// When valid() returns true, value returned as string-view
210+
// is guaranteed to be valid until iterator is moved to another
211+
// position; that is until call to next() / seek_to_first() / etc.
209212
virtual std::string_view value_as_sv() = 0;
210213
virtual int status() = 0;
211214
virtual ~SimplestIteratorImpl() {}
@@ -216,7 +219,12 @@ class KeyValueDB {
216219
virtual ~IteratorImpl() {}
217220
virtual int seek_to_last() = 0;
218221
virtual int prev() = 0;
222+
// When valid() returns true, key returned as string-view
223+
// is guaranteed to be valid until iterator is moved to another
224+
// position; that is until call to next() / seek_to_first() / etc.
225+
virtual std::string_view key_as_sv() = 0;
219226
virtual std::pair<std::string, std::string> raw_key() = 0;
227+
virtual std::pair<std::string_view, std::string_view> raw_key_as_sv() = 0;
220228
virtual ceph::buffer::ptr value_as_ptr() {
221229
ceph::buffer::list bl = value();
222230
if (bl.length() == 1) {
@@ -243,7 +251,9 @@ class KeyValueDB {
243251
virtual int next() = 0;
244252
virtual int prev() = 0;
245253
virtual std::string key() = 0;
254+
virtual std::string_view key_as_sv() = 0;
246255
virtual std::pair<std::string,std::string> raw_key() = 0;
256+
virtual std::pair<std::string_view, std::string_view> raw_key_as_sv() = 0;
247257
virtual bool raw_key_is_prefixed(const std::string &prefix) = 0;
248258
virtual ceph::buffer::list value() = 0;
249259
virtual ceph::buffer::ptr value_as_ptr() {
@@ -312,9 +322,15 @@ class KeyValueDB {
312322
std::string key() override {
313323
return generic_iter->key();
314324
}
325+
std::string_view key_as_sv() override {
326+
return generic_iter->key_as_sv();
327+
}
315328
std::pair<std::string, std::string> raw_key() override {
316329
return generic_iter->raw_key();
317330
}
331+
std::pair<std::string_view, std::string_view> raw_key_as_sv() override {
332+
return generic_iter->raw_key_as_sv();
333+
}
318334
ceph::buffer::list value() override {
319335
return generic_iter->value();
320336
}

‎src/kv/RocksDBStore.cc‎

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <memory>
77
#include <set>
88
#include <string>
9+
#include <string_view>
910
#include <errno.h>
1011
#include <unistd.h>
1112
#include <sys/types.h>
@@ -47,6 +48,7 @@ using std::ostream;
4748
using std::pair;
4849
using std::set;
4950
using std::string;
51+
using std::string_view;
5052
using std::unique_ptr;
5153
using std::vector;
5254

@@ -1992,7 +1994,7 @@ int RocksDBStore::split_key(rocksdb::Slice in, string *prefix, string *key)
19921994

19931995
// Find separator inside Slice
19941996
char* separator = (char*) memchr(in.data(), 0, in.size());
1995-
if (separator == NULL)
1997+
if (separator == nullptr)
19961998
return -EINVAL;
19971999
prefix_len = size_t(separator - in.data());
19982000
if (prefix_len >= in.size())
@@ -2006,6 +2008,27 @@ int RocksDBStore::split_key(rocksdb::Slice in, string *prefix, string *key)
20062008
return 0;
20072009
}
20082010

2011+
// TODO: deduplicate the code, preferrably by removing the string variant
2012+
int RocksDBStore::split_key(rocksdb::Slice in, string_view *prefix, string_view *key)
2013+
{
2014+
size_t prefix_len = 0;
2015+
2016+
// Find separator inside Slice
2017+
char* separator = (char*) memchr(in.data(), 0, in.size());
2018+
if (separator == nullptr)
2019+
return -EINVAL;
2020+
prefix_len = size_t(separator - in.data());
2021+
if (prefix_len >= in.size())
2022+
return -EINVAL;
2023+
2024+
// Fetch prefix and/or key directly from Slice
2025+
if (prefix)
2026+
*prefix = string_view(in.data(), prefix_len);
2027+
if (key)
2028+
*key = string_view(separator + 1, in.size() - prefix_len - 1);
2029+
return 0;
2030+
}
2031+
20092032
void RocksDBStore::compact()
20102033
{
20112034
dout(2) << __func__ << " starting" << dendl;
@@ -2226,7 +2249,13 @@ int RocksDBStore::RocksDBWholeSpaceIteratorImpl::prev()
22262249
string RocksDBStore::RocksDBWholeSpaceIteratorImpl::key()
22272250
{
22282251
string out_key;
2229-
split_key(dbiter->key(), 0, &out_key);
2252+
split_key(dbiter->key(), nullptr, &out_key);
2253+
return out_key;
2254+
}
2255+
string_view RocksDBStore::RocksDBWholeSpaceIteratorImpl::key_as_sv()
2256+
{
2257+
string_view out_key;
2258+
split_key(dbiter->key(), nullptr, &out_key);
22302259
return out_key;
22312260
}
22322261
pair<string,string> RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key()
@@ -2235,6 +2264,12 @@ pair<string,string> RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key()
22352264
split_key(dbiter->key(), &prefix, &key);
22362265
return make_pair(prefix, key);
22372266
}
2267+
pair<string_view,string_view> RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key_as_sv()
2268+
{
2269+
string_view prefix, key;
2270+
split_key(dbiter->key(), &prefix, &key);
2271+
return make_pair(prefix, key);
2272+
}
22382273

22392274
bool RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key_is_prefixed(const string &prefix) {
22402275
// Look for "prefix\0" right in rocksb::Slice
@@ -2354,9 +2389,15 @@ class CFIteratorImpl : public KeyValueDB::IteratorImpl {
23542389
string key() override {
23552390
return dbiter->key().ToString();
23562391
}
2392+
string_view key_as_sv() override {
2393+
return dbiter->key().ToStringView();
2394+
}
23572395
std::pair<std::string, std::string> raw_key() override {
23582396
return make_pair(prefix, key());
23592397
}
2398+
std::pair<std::string_view, std::string_view> raw_key_as_sv() override {
2399+
return make_pair(prefix, dbiter->key().ToStringView());
2400+
}
23602401
bufferlist value() override {
23612402
return to_bufferlist(dbiter->value());
23622403
}
@@ -2678,6 +2719,15 @@ class WholeMergeIteratorImpl : public KeyValueDB::WholeSpaceIteratorImpl {
26782719
}
26792720
}
26802721

2722+
std::string_view key_as_sv() override
2723+
{
2724+
if (smaller == on_main) {
2725+
return main->key_as_sv();
2726+
} else {
2727+
return current_shard->second->key_as_sv();
2728+
}
2729+
}
2730+
26812731
std::pair<std::string,std::string> raw_key() override
26822732
{
26832733
if (smaller == on_main) {
@@ -2687,6 +2737,15 @@ class WholeMergeIteratorImpl : public KeyValueDB::WholeSpaceIteratorImpl {
26872737
}
26882738
}
26892739

2740+
std::pair<std::string_view,std::string_view> raw_key_as_sv() override
2741+
{
2742+
if (smaller == on_main) {
2743+
return main->raw_key_as_sv();
2744+
} else {
2745+
return { current_shard->first, current_shard->second->key_as_sv() };
2746+
}
2747+
}
2748+
26902749
bool raw_key_is_prefixed(const std::string &prefix) override
26912750
{
26922751
if (smaller == on_main) {
@@ -3036,9 +3095,15 @@ class ShardMergeIteratorImpl : public KeyValueDB::IteratorImpl {
30363095
string key() override {
30373096
return iters[0]->key().ToString();
30383097
}
3098+
string_view key_as_sv() override {
3099+
return iters[0]->key().ToStringView();
3100+
}
30393101
std::pair<std::string, std::string> raw_key() override {
30403102
return make_pair(prefix, key());
30413103
}
3104+
std::pair<std::string_view, std::string_view> raw_key_as_sv() override {
3105+
return make_pair(prefix, iters[0]->key().ToStringView());
3106+
}
30423107
bufferlist value() override {
30433108
return to_bufferlist(iters[0]->value());
30443109
}

‎src/kv/RocksDBStore.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,7 +380,9 @@ class RocksDBStore : public KeyValueDB {
380380
int next() override;
381381
int prev() override;
382382
std::string key() override;
383+
std::string_view key_as_sv() override;
383384
std::pair<std::string,std::string> raw_key() override;
385+
std::pair<std::string_view,std::string_view> raw_key_as_sv() override;
384386
bool raw_key_is_prefixed(const std::string &prefix) override;
385387
ceph::bufferlist value() override;
386388
ceph::bufferptr value_as_ptr() override;
@@ -414,6 +416,7 @@ class RocksDBStore : public KeyValueDB {
414416
}
415417

416418
static int split_key(rocksdb::Slice in, std::string *prefix, std::string *key);
419+
static int split_key(rocksdb::Slice in, std::string_view *prefix, std::string_view *key);
417420

418421
static std::string past_prefix(const std::string &prefix);
419422

‎src/os/bluestore/BlueStore.cc‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4843,6 +4843,19 @@ void BlueStore::Onode::decode_omap_key(const string& key, string *user_key)
48434843
*user_key = key.substr(pos);
48444844
}
48454845

4846+
void BlueStore::Onode::decode_omap_key(const std::string_view& key, std::string_view *user_key)
4847+
{
4848+
size_t pos = sizeof(uint64_t) + 1;
4849+
if (!onode.is_pgmeta_omap()) {
4850+
if (onode.is_perpg_omap()) {
4851+
pos += sizeof(uint64_t) + sizeof(uint32_t);
4852+
} else if (onode.is_perpool_omap()) {
4853+
pos += sizeof(uint64_t);
4854+
}
4855+
}
4856+
*user_key = key.substr(pos);
4857+
}
4858+
48464859
void BlueStore::Onode::finish_write(TransContext* txc, uint32_t offset, uint32_t length)
48474860
{
48484861
while (true) {
@@ -13797,12 +13810,12 @@ int BlueStore::omap_iterate(
1379713810
ceph::timespan next_lat_acc{0};
1379813811
o->get_omap_tail(&tail);
1379913812
while (it->valid()) {
13800-
std::string user_key;
13801-
if (const auto& db_key = it->raw_key().second; db_key >= tail) {
13813+
const auto& db_key = it->raw_key_as_sv().second;
13814+
if (db_key >= tail) {
1380213815
break;
13803-
} else {
13804-
o->decode_omap_key(db_key, &user_key);
1380513816
}
13817+
std::string_view user_key;
13818+
o->decode_omap_key(db_key, &user_key);
1380613819
omap_iter_ret_t ret = f(user_key, it->value_as_sv());
1380713820
if (ret == omap_iter_ret_t::STOP) {
1380813821
break;

‎src/os/bluestore/BlueStore.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1458,6 +1458,7 @@ class BlueStore : public ObjectStore,
14581458

14591459
void rewrite_omap_key(const std::string& old, std::string *out);
14601460
void decode_omap_key(const std::string& key, std::string *user_key);
1461+
void decode_omap_key(const std::string_view& key, std::string_view *user_key);
14611462

14621463
void finish_write(TransContext* txc, uint32_t offset, uint32_t length);
14631464

‎src/test/ObjectMap/KeyValueDBMemory.cc‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,26 @@ class WholeSpaceMemIterator : public KeyValueDB::WholeSpaceIteratorImpl {
132132
return "";
133133
}
134134

135+
string_view key_as_sv() override {
136+
if (valid())
137+
return (*it).first.second;
138+
else
139+
return "";
140+
}
141+
135142
pair<string,string> raw_key() override {
136143
if (valid())
137144
return (*it).first;
138145
else
139146
return make_pair("", "");
140147
}
148+
149+
pair<string_view,string_view> raw_key_as_sv() override {
150+
if (valid())
151+
return (*it).first;
152+
else
153+
return make_pair("", "");
154+
}
141155

142156
bool raw_key_is_prefixed(const string &prefix) override {
143157
return prefix == (*it).first.first;

0 commit comments

Comments
 (0)