Skip to content

perf: optimize command EXISTS using MultiGet #3194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: unstable
Choose a base branch
from
Open

perf: optimize command EXISTS using MultiGet #3194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
88ac2ec
Change command EXISTS from Get to MultiGet
cyningsun Sep 11, 2025
964502f
fix ReadOptions to MultiGetOptions
cyningsun Sep 19, 2025
1a6d994
inline functions
cyningsun Sep 19, 2025
a72cada
Merge branch 'unstable' into feat/command_exists_optimization
cyningsun Sep 19, 2025
33e93db
Move comments from caller to function declaration
cyningsun Sep 20, 2025
37851d8
change decode input from value to pointer
cyningsun Sep 20, 2025
4f64171
pass PinnableSlice directly
cyningsun Sep 20, 2025
07e268b
revert MDel modification and create another PR to fix it.
cyningsun Sep 20, 2025
fd68946
Merge branch 'unstable' into feat/command_exists_optimization
aleksraiden Sep 20, 2025
277381c
Merge branch 'unstable' into feat/command_exists_optimization
aleksraiden Sep 21, 2025
fe61d31
Merge branch 'unstable' into feat/command_exists_optimization
aleksraiden Sep 25, 2025
5d07c99
Merge branch 'unstable' into feat/command_exists_optimization
aleksraiden Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 35 additions & 19 deletions src/storage/redis_db.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,12 +208,37 @@ rocksdb::Status Database::MDel(engine::Context &ctx, const std::vector<Slice> &k
}

rocksdb::Status Database::Exists(engine::Context &ctx, const std::vector<Slice> &keys, int *ret) {
*ret = 0;

if (keys.empty()) {
return rocksdb::Status::OK();
}

std::vector<std::string> ns_keys;
std::vector<Slice> slice_keys;
ns_keys.reserve(keys.size());
slice_keys.reserve(keys.size());

for (const auto &key : keys) {
ns_keys.emplace_back(AppendNamespacePrefix(key));
slice_keys.emplace_back(ns_keys.back());
}

std::vector<rocksdb::Status> statuses(slice_keys.size());
std::vector<rocksdb::PinnableSlice> pin_values(slice_keys.size());
storage_->MultiGet(ctx, ctx.DefaultMultiGetOptions(), metadata_cf_handle_, slice_keys.size(), slice_keys.data(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following your benchmark here #3169 (comment), should we use Get instead if keys.size() is 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @PragmaTwice, we actually just discussed this exact point in another thread. The short answer is that I agree it's a valid optimization. I'll running additional tests for high cache-hit scenarios before deciding whether to implement the conditional Get for a single key.

You can see the full discussion here: #3194 (comment)

pin_values.data(), statuses.data());

for (size_t i = 0; i < slice_keys.size(); i++) {
if (!statuses[i].ok() && !statuses[i].IsNotFound()) return statuses[i];
if (statuses[i].ok()) {
Metadata metadata(kRedisNone, false);
auto s = metadata.Decode(pin_values[i]);
if (!s.ok()) return s;
if (!metadata.Expired()) *ret += 1;
}
}
return existsInternal(ctx, ns_keys, ret);
return rocksdb::Status::OK();
}

rocksdb::Status Database::TTL(engine::Context &ctx, const Slice &user_key, int64_t *ttl) {
Expand Down Expand Up @@ -621,23 +646,6 @@ Status WriteBatchLogData::Decode(const rocksdb::Slice &blob) {
return Status::OK();
}

rocksdb::Status Database::existsInternal(engine::Context &ctx, const std::vector<std::string> &keys, int *ret) {
*ret = 0;
rocksdb::Status s;
std::string value;
for (const auto &key : keys) {
s = storage_->Get(ctx, ctx.GetReadOptions(), metadata_cf_handle_, key, &value);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
Metadata metadata(kRedisNone, false);
s = metadata.Decode(value);
if (!s.ok()) return s;
if (!metadata.Expired()) *ret += 1;
}
}
return rocksdb::Status::OK();
}

rocksdb::Status Database::typeInternal(engine::Context &ctx, const Slice &key, RedisType *type) {
*type = kRedisNone;
std::string value;
Expand Down Expand Up @@ -667,7 +675,15 @@ rocksdb::Status Database::Copy(engine::Context &ctx, const std::string &key, con

if (nx) {
int exist = 0;
if (s = existsInternal(ctx, {new_key}, &exist), !s.ok()) return s;
std::string value;
s = storage_->Get(ctx, ctx.GetReadOptions(), metadata_cf_handle_, new_key, &value);
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
Metadata metadata(kRedisNone, false);
s = metadata.Decode(value);
if (!s.ok()) return s;
if (!metadata.Expired()) exist = 1;
}
if (exist > 0) {
*res = CopyResult::KEY_ALREADY_EXIST;
return rocksdb::Status::OK();
Expand Down
1 change: 0 additions & 1 deletion src/storage/redis_db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ class Database {

private:
// Already internal keys
[[nodiscard]] rocksdb::Status existsInternal(engine::Context &ctx, const std::vector<std::string> &keys, int *ret);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so sorry to comment this again and again. But why not:

[[nodiscard]] rocksdb::Status existsInternal(engine::Context &ctx, const nonstd::span<Slice> &keys, int *ret) {
  // From benchmark, ...
  if (keys.size() == 1) {
     // Get..
  } else {
    // MultiGet
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mapleFU Thanks so much for commenting—please don't be sorry! I really welcome the discussion, and I'm glad we're digging into the details.

You're absolutely right, and in fact, I completely agree with your thinking. The implementation you're proposing is a more conservative and robust approach. If I were working on this within a company codebase, I would likely advocate for the same solution you've outlined, for a few key reasons:

  • In real-world usage, the number of keys for EXISTS maybe smaller than for MGET or MDEL.
  • With a smaller number of keys and a higher in-memory hit rate, the potential performance difference between a single Get and a MultiGet becomes more significant.

The main reason I went with the current implementation are:

However, you've made a very valid point that deserves more thorough testing. Before we merge this, let me run some more benchmarks. I'll specifically focus on scenarios with a higher cache hit rate and very small key lists to better quantify the impact. Once I have those results, we can make a more data-driven decision on whether to keep the current unified implementation or adopt the optimized approach you suggested for existsInternal.

I really appreciate you pushing for the best solution! Let's sync up again after the next round of tests.

[[nodiscard]] rocksdb::Status typeInternal(engine::Context &ctx, const Slice &key, RedisType *type);

/// lookupKeyByPattern is a helper function of `Sort` to support `GET` and `BY` fields.
Expand Down
3 changes: 3 additions & 0 deletions src/storage/redis_metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,10 @@ class Metadata {
bool IsEmptyableType() const;

virtual void Encode(std::string *dst) const;

// Calls remove_prefix() internally and would modify the original `input` state.
[[nodiscard]] virtual rocksdb::Status Decode(Slice *input);

[[nodiscard]] rocksdb::Status Decode(Slice input);

bool operator==(const Metadata &that) const;
Expand Down
Loading