You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/06/21 06:31:48 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #643: Optimize happy path for constructing Status
PragmaTwice opened a new pull request, #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643
In kvrocks, `Status` is used to indicate the state of the operation when it is completed.
Obviously, these operation will complete normally (often called a happy path) in most cases (or say, with high probability), and fail in a small number of cases.
Field `msg_` typed `std::string` will be assigned to `ok` when constructing the normal state, which can be more time consuming than the default constructor (although usually `std::string` implementations in the standard library do SSO, small string optimization, thus heap allocation may be avoided).
In this PR, I delayed the `ok` to message dumping to speed up the happy path.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902351147
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
- std::string Msg() { return msg_; }
- static Status OK() { return Status(cOK, "ok"); }
+ std::string Msg() {
+ if (IsOK()) {
+ return "ok";
+ }
+ return msg_;
Review Comment:
The function `Msg` only will be called in a error path, so the probability of calling is low. This fits perfectly with my idea.
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902356147
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
Review Comment:
> because we already have a method named `OK` 🤣
Yes, exactly. But `cOK` is NOT a good name since it's inconsistent with other code.
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902383424
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
Review Comment:
There are several typos in our codebase. I don't think they deserve an issue but if you'd like to fix it with another patch, go ahead. Otherwise I may handle it later this week =。=
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902377007
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
Review Comment:
🤣
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902351147
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
- std::string Msg() { return msg_; }
- static Status OK() { return Status(cOK, "ok"); }
+ std::string Msg() {
+ if (IsOK()) {
+ return "ok";
+ }
+ return msg_;
Review Comment:
The function `Msg` only will be called in a error path, not normal path, so the probability of calling is low. This fits perfectly with my idea.
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
- std::string Msg() { return msg_; }
- static Status OK() { return Status(cOK, "ok"); }
+ std::string Msg() {
+ if (IsOK()) {
+ return "ok";
+ }
+ return msg_;
Review Comment:
The function `Msg` only will be called in a error path, not normal path, so the probability of calling is low.
This fits perfectly with my idea.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902372341
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
Review Comment:
Maybe open a help wanted issue to encourage more involvement🤣
Not sure whether it is a proper way, calling expert @tisonkun
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902351147
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
- std::string Msg() { return msg_; }
- static Status OK() { return Status(cOK, "ok"); }
+ std::string Msg() {
+ if (IsOK()) {
+ return "ok";
+ }
+ return msg_;
Review Comment:
The function `Msg` only will be called in a error path, not normal path, so the probability of calling is low.
This fits perfectly with my idea, so I think something like `__builtin_expect` may be unnecessary : )
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902346518
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
Review Comment:
because we already have a method named `OK` 🤣
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902351147
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
- std::string Msg() { return msg_; }
- static Status OK() { return Status(cOK, "ok"); }
+ std::string Msg() {
+ if (IsOK()) {
+ return "ok";
+ }
+ return msg_;
Review Comment:
The function `Msg` only will be called in a error path (in most cases), not normal path, so the probability of calling is low.
This fits perfectly with my idea, so I think something like `__builtin_expect` may be unnecessary : )
A classic pattern for `Msg`:
```c++
Status s = ...
if(!s.OK()) {
dump s.Msg() to somewhere...
}
```
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902351147
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
- std::string Msg() { return msg_; }
- static Status OK() { return Status(cOK, "ok"); }
+ std::string Msg() {
+ if (IsOK()) {
+ return "ok";
+ }
+ return msg_;
Review Comment:
The function `Msg` only will be called in a error path (in most cases), not normal path, so the probability of calling is low.
This fits perfectly with my idea, so I think something like `__builtin_expect` may be unnecessary : )
--
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
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902356385
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
Review Comment:
it's typo
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902345056
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
Review Comment:
@ShooterIT @git-hulk may I ask why `cOK` gets its name at the first place instead of `OK`?
--
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
[GitHub] [incubator-kvrocks] tisonkun merged pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902348261
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
- std::string Msg() { return msg_; }
- static Status OK() { return Status(cOK, "ok"); }
+ std::string Msg() {
+ if (IsOK()) {
+ return "ok";
+ }
+ return msg_;
Review Comment:
Will this change cause branch prediction overhead and affect efficiency which we'd like to improve in this PR? It seems not a net win.
--
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
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902345903
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
Review Comment:
```suggestion
bool IsImporting() { return code_ == SlotImport; }
```
Typo? May we fix it in another PR though.
--
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
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #643: Optimize happy path for constructing Status
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #643:
URL: https://github.com/apache/incubator-kvrocks/pull/643#discussion_r902389202
##########
src/status.h:
##########
@@ -57,13 +57,18 @@ class Status {
NetSendErr,
};
- Status() : Status(cOK, "ok") {}
- explicit Status(Code code, std::string msg = "") : code_(code), msg_(std::move(msg)) {}
+ Status() : Status(cOK) {}
+ explicit Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {}
bool IsOK() { return code_ == cOK; }
bool IsNotFound() { return code_ == NotFound; }
bool IsImorting() { return code_ == SlotImport; }
Review Comment:
Got it. Thanks.
--
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