You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "ggyuchive (via GitHub)" <gi...@apache.org> on 2026/01/02 15:01:39 UTC
[PR] perf(string): Reduce read access in MSETEX [kvrocks]
ggyuchive opened a new pull request, #3322:
URL: https://github.com/apache/kvrocks/pull/3322
When run `MSETEX` command with `NX|XX` and `KEEPTTL`, it access 2*N times for read.
- Reduce read access from 2*N to N.
- Change `Exists` ret type from `int` to `uint32_t`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(string): Reduce read access in MSETEX [kvrocks]
Posted by "ggyuchive (via GitHub)" <gi...@apache.org>.
ggyuchive commented on code in PR #3322:
URL: https://github.com/apache/kvrocks/pull/3322#discussion_r2657944435
##########
src/types/redis_string.cc:
##########
@@ -395,43 +395,38 @@ rocksdb::Status String::IncrByFloat(engine::Context &ctx, const std::string &use
}
rocksdb::Status String::MSet(engine::Context &ctx, const std::vector<StringPair> &pairs, StringMSetArgs args,
- bool *flag) {
- if (flag != nullptr) {
- *flag = false;
- }
-
- if (args.type != StringSetType::NONE) {
- int exists = 0;
- int key_count = static_cast<int>(pairs.size());
- std::vector<Slice> keys;
- keys.reserve(pairs.size());
- for (const auto &pair : pairs) {
- keys.emplace_back(pair.key);
- }
- auto s = Exists(ctx, keys, &exists);
- if (!s.ok()) return s;
- if ((args.type == StringSetType::NX && exists > 0) || (args.type == StringSetType::XX && exists < key_count)) {
- return rocksdb::Status::OK();
- }
- }
+ bool *flag = nullptr) {
+ if (flag) *flag = false;
auto batch = storage_->GetWriteBatchBase();
WriteBatchLogData log_data(kRedisString);
- rocksdb::Status s = batch->PutLogData(log_data.Encode());
+ auto s = batch->PutLogData(log_data.Encode());
Review Comment:
@PragmaTwice , you're right. This change has an issue in tx mode.
```
127.0.0.1:6666> msetex 1 key1 value1
(integer) 1
127.0.0.1:6666> get key2
(nil)
127.0.0.1:6666> multi
OK
127.0.0.1:6666(TX)> msetex 2 key1 value100 key2 value200 xx
QUEUED
127.0.0.1:6666(TX)> exec
1) (integer) 0
127.0.0.1:6666> get key1
"value100"
127.0.0.1:6666>
```
key1 must be value1 after commit. I will fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(string): Reduce read access in MSETEX [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #3322:
URL: https://github.com/apache/kvrocks/pull/3322#discussion_r2657878791
##########
src/types/redis_string.cc:
##########
@@ -395,43 +395,38 @@ rocksdb::Status String::IncrByFloat(engine::Context &ctx, const std::string &use
}
rocksdb::Status String::MSet(engine::Context &ctx, const std::vector<StringPair> &pairs, StringMSetArgs args,
- bool *flag) {
- if (flag != nullptr) {
- *flag = false;
- }
-
- if (args.type != StringSetType::NONE) {
- int exists = 0;
- int key_count = static_cast<int>(pairs.size());
- std::vector<Slice> keys;
- keys.reserve(pairs.size());
- for (const auto &pair : pairs) {
- keys.emplace_back(pair.key);
- }
- auto s = Exists(ctx, keys, &exists);
- if (!s.ok()) return s;
- if ((args.type == StringSetType::NX && exists > 0) || (args.type == StringSetType::XX && exists < key_count)) {
- return rocksdb::Status::OK();
- }
- }
+ bool *flag = nullptr) {
+ if (flag) *flag = false;
auto batch = storage_->GetWriteBatchBase();
WriteBatchLogData log_data(kRedisString);
- rocksdb::Status s = batch->PutLogData(log_data.Encode());
+ auto s = batch->PutLogData(log_data.Encode());
Review Comment:
I think we should get and modify the batch after all checks (e.g. NX/XX checks below). Even we don't write this batch into storage, in transaction mode we'll use this batch to commit.
##########
src/types/redis_string.cc:
##########
@@ -395,43 +395,38 @@ rocksdb::Status String::IncrByFloat(engine::Context &ctx, const std::string &use
}
rocksdb::Status String::MSet(engine::Context &ctx, const std::vector<StringPair> &pairs, StringMSetArgs args,
- bool *flag) {
- if (flag != nullptr) {
- *flag = false;
- }
-
- if (args.type != StringSetType::NONE) {
- int exists = 0;
- int key_count = static_cast<int>(pairs.size());
- std::vector<Slice> keys;
- keys.reserve(pairs.size());
- for (const auto &pair : pairs) {
- keys.emplace_back(pair.key);
- }
- auto s = Exists(ctx, keys, &exists);
- if (!s.ok()) return s;
- if ((args.type == StringSetType::NX && exists > 0) || (args.type == StringSetType::XX && exists < key_count)) {
- return rocksdb::Status::OK();
- }
- }
+ bool *flag = nullptr) {
+ if (flag) *flag = false;
auto batch = storage_->GetWriteBatchBase();
WriteBatchLogData log_data(kRedisString);
- rocksdb::Status s = batch->PutLogData(log_data.Encode());
+ auto s = batch->PutLogData(log_data.Encode());
Review Comment:
I think we should get and modify the batch after all checks (e.g. NX/XX checks below). Even we don't write this batch into storage, in transaction mode we'll use this batch to commit. cc @git-hulk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(string): Reduce read access in MSETEX [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3322:
URL: https://github.com/apache/kvrocks/pull/3322#issuecomment-3707066595
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3322) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3322&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=3322&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=3322&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [73.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3322&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3322&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3322)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(string): reduce read access in MSETEX [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #3322:
URL: https://github.com/apache/kvrocks/pull/3322
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org