perf: optimize command EXISTS using MultiGet #3194
Conversation
src/storage/redis_db.cc
Outdated
| // Explicit construct a rocksdb::Slice to avoid the implicit conversion from | ||
| // PinnableSlice to Slice. |
There was a problem hiding this comment.
@mapleFU thanks for your comment!
If you're referring to the comment (A):
I agree it's overly verbose. It can indeed be cleaned up.
If you're referring to the explicit construction (B):
The explicit construction is mainly for safety - since Decode() calls remove_prefix() internally, it would modify the original pin_values[i] state if passed directly. It's better to create a temporary Slice copy, leaving original data intact, which matches the pattern used elsewhere in the codebase.
What's your take on this? I'd be curious to hear your perspective
There was a problem hiding this comment.
The command is ok to me but I think "it would modify the original pin_values[i] state if passed directly" it's more specifically rather than "implicit conversion"?
There was a problem hiding this comment.
[[nodiscard]] virtual rocksdb::Status Decode(Slice *input);
[[nodiscard]] rocksdb::Status Decode(Slice input);
Sorry for my previous incorrect explanation. Actually, due to call the second function of Metadata and parameter is passed by value, an implicit conversion occurs, which modifies the copied value rather than pin_values itself. Therefore, the comment should be accurate. The explicit construction has no obvious different with implicit construction, that's what I mean overly verbose
There was a problem hiding this comment.
Comment is moved from caller to function declaration
src/storage/redis_db.cc
Outdated
| if (!statuses[i].ok() && !statuses[i].IsNotFound()) return statuses[i]; | ||
| if (statuses[i].ok()) { | ||
| Metadata metadata(kRedisNone, false); | ||
| auto s = metadata.Decode(rocksdb::Slice(pin_values[i].data(), pin_values[i].size())); |
There was a problem hiding this comment.
However, PinnableSlice wouldn't be changed if pass by value?
There was a problem hiding this comment.
yes, that's correct
src/storage/redis_db.cc
Outdated
| if (!statuses[i].ok() && !statuses[i].IsNotFound()) return statuses[i]; | ||
| if (statuses[i].ok()) { | ||
| Metadata metadata(kRedisNone, false); | ||
| auto s = metadata.Decode(&rocksdb::Slice(pin_values[i].data(), pin_values[i].size())); |
There was a problem hiding this comment.
Hmmm why not just
auto s = metadata.Decode(pin_values[i]);
There was a problem hiding this comment.
// Explicit construct a rocksdb::Slice to avoid the implicit conversion from
// PinnableSlice to Slice.
In fact, it can. The initial implementation is referred to MDel's implementation. That's why there is a annotation. Upon our discussion and analysis, it is indeed not very reasonable. Let me make some modifications
|
|
|
||
| private: | ||
| // Already internal keys | ||
| [[nodiscard]] rocksdb::Status existsInternal(engine::Context &ctx, const std::vector<std::string> &keys, int *ret); |
There was a problem hiding this comment.
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
}
}
There was a problem hiding this comment.
@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
EXISTSmaybe smaller than forMGETorMDEL. - With a smaller number of keys and a higher in-memory hit rate, the potential performance difference between a single
Getand aMultiGetbecomes more significant.
The main reason I went with the current implementation are:
- In the benchmark testing(Performance Question: Could EXISTS command benefit from MultiGet optimization? #3169), the performance regression for smaller key lists was measured to be quite small—within 5%. (
the memory to disk ratio is indeed not very high) MGETandMDELalso shares the same problem for handling small lists of keys- Using the same
MultiGetpath kept the implementation simpler and more elegant.
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.
PragmaTwice
changed the title
|
|
||
| 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(), |
There was a problem hiding this comment.
Following your benchmark here #3169 (comment), should we use Get instead if keys.size() is 1?
There was a problem hiding this comment.
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)
Close #3169