You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "PragmaTwice (via GitHub)" <gi...@apache.org> on 2023/03/18 07:36:04 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #1330: Refactor Status to avoid large stack size in happy path
PragmaTwice opened a new pull request, #1330:
URL: https://github.com/apache/incubator-kvrocks/pull/1330
Follow the implementation of Status in RocksDB, google absl, doris and more, we refactor the Status to make it just a pointer (8 instead of 40 for libstdc++ std::string).
For happy path (OK status) with large probability, it benefits the performance.
--
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 pull request #1330: Refactor Status to avoid large stack size in happy path
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1330:
URL: https://github.com/apache/incubator-kvrocks/pull/1330#issuecomment-1475154076
> There are still ICE:
>
> ```
> lto1: internal compiler error: in add_symbol_to_partition_1, at lto/lto-partition.c:153
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <file:///usr/share/doc/gcc-9/README.Bugs> for instructions.
> lto-wrapper: fatal error: /usr/bin/g++ returned 1 exit status
> compilation terminated.
> /usr/bin/ld: error: lto-wrapper failed
> collect2: error: ld returned 1 exit status
> ```
sad
--
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 closed pull request #1330: Refactor Status to avoid large stack size in happy path
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice closed pull request #1330: Refactor Status to avoid large stack size in happy path
URL: https://github.com/apache/incubator-kvrocks/pull/1330
--
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 #1330: Refactor Status to avoid large stack size in happy path
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1330:
URL: https://github.com/apache/incubator-kvrocks/pull/1330#discussion_r1141234948
##########
src/common/status.h:
##########
@@ -67,29 +66,41 @@ class [[nodiscard]] Status {
BlockingCmd,
};
- Status() : Status(cOK) {}
+ Status() : impl_{nullptr} {}
- // WARNING: it is NOT ALLOWED to construct Status with cOK code and a non-empty message string
- Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {} // NOLINT
+ Status(Code code, std::string msg = {}) : impl_{new Impl{code, std::move(msg)}} { // NOLINT
+ CHECK(code != cOK);
+ }
+
+ Status(const Status& s) : impl_{s ? nullptr : new Impl{s.impl_->code_, s.impl_->msg_}} {}
+ Status(Status&& s) = default;
+
+ ~Status() = default;
+
+ Status& operator=(const Status& s) {
+ Status tmp{s};
+ return *this = std::move(tmp);
+ }
+ Status& operator=(Status&&) = default;
template <Code code>
bool Is() const {
- return code_ == code;
+ return impl_ && impl_->code_ == code;
}
- bool IsOK() const { return Is<cOK>(); }
+ bool IsOK() const { return !impl_; }
explicit operator bool() const { return IsOK(); }
- Code GetCode() const { return code_; }
+ Code GetCode() const { return impl_ ? impl_->code_ : cOK; }
std::string Msg() const& {
if (*this) return ok_msg;
- return msg_;
+ return impl_->msg_;
}
std::string Msg() && {
if (*this) return ok_msg;
- return std::move(msg_);
+ return std::move(impl_->msg_);
}
static Status OK() { return {}; }
Review Comment:
Shall we return `cOK` here also to avoid new object instance?
--
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 #1330: Refactor Status to avoid large stack size in happy path
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #1330:
URL: https://github.com/apache/incubator-kvrocks/pull/1330#discussion_r1141236612
##########
src/common/status.h:
##########
@@ -67,29 +66,41 @@ class [[nodiscard]] Status {
BlockingCmd,
};
- Status() : Status(cOK) {}
+ Status() : impl_{nullptr} {}
- // WARNING: it is NOT ALLOWED to construct Status with cOK code and a non-empty message string
- Status(Code code, std::string msg = {}) : code_(code), msg_(std::move(msg)) {} // NOLINT
+ Status(Code code, std::string msg = {}) : impl_{new Impl{code, std::move(msg)}} { // NOLINT
+ CHECK(code != cOK);
+ }
+
+ Status(const Status& s) : impl_{s ? nullptr : new Impl{s.impl_->code_, s.impl_->msg_}} {}
+ Status(Status&& s) = default;
+
+ ~Status() = default;
+
+ Status& operator=(const Status& s) {
+ Status tmp{s};
+ return *this = std::move(tmp);
+ }
+ Status& operator=(Status&&) = default;
template <Code code>
bool Is() const {
- return code_ == code;
+ return impl_ && impl_->code_ == code;
}
- bool IsOK() const { return Is<cOK>(); }
+ bool IsOK() const { return !impl_; }
explicit operator bool() const { return IsOK(); }
- Code GetCode() const { return code_; }
+ Code GetCode() const { return impl_ ? impl_->code_ : cOK; }
std::string Msg() const& {
if (*this) return ok_msg;
- return msg_;
+ return impl_->msg_;
}
std::string Msg() && {
if (*this) return ok_msg;
- return std::move(msg_);
+ return std::move(impl_->msg_);
}
static Status OK() { return {}; }
Review Comment:
Since we don't return a reference here, it should not be different.
--
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 pull request #1330: Refactor Status to avoid large stack size in happy path
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1330:
URL: https://github.com/apache/incubator-kvrocks/pull/1330#issuecomment-1475097492
There are still ICE:
```
lto1: internal compiler error: in add_symbol_to_partition_1, at lto/lto-partition.c:153
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-9/README.Bugs> for instructions.
lto-wrapper: fatal error: /usr/bin/g++ returned 1 exit status
compilation terminated.
/usr/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status
```
--
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 pull request #1330: Refactor Status to avoid large stack size in happy path
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1330:
URL: https://github.com/apache/incubator-kvrocks/pull/1330#issuecomment-1477213859
Will split this PR to multiple ones against ICE.
--
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