This is an automated email from the ASF dual-hosted git repository.
yangzhg pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-doris.git
The following commit(s) were added to refs/heads/master by this push:
new 47067e4 [refactor](common) optimize Status implemention: no dynamic
new (#8117)
47067e4 is described below
commit 47067e40a6c4b1ebb8961588e8afe9b3fc79c0ff
Author: zuochunwei <[email protected]>
AuthorDate: Tue Feb 22 09:23:29 2022 +0800
[refactor](common) optimize Status implemention: no dynamic new (#8117)
---
be/src/common/status.cpp | 62 +++++--------------
be/src/common/status.h | 102 +++++++++++++++++++++++--------
be/src/runtime/buffered_tuple_stream3.cc | 2 +-
be/src/runtime/minidump.cpp | 2 +-
4 files changed, 92 insertions(+), 76 deletions(-)
diff --git a/be/src/common/status.cpp b/be/src/common/status.cpp
index 75eab87..c497ba5 100644
--- a/be/src/common/status.cpp
+++ b/be/src/common/status.cpp
@@ -8,61 +8,34 @@
namespace doris {
-inline const char* assemble_state(TStatusCode::type code, const Slice& msg,
int16_t precise_code,
- const Slice& msg2) {
- DCHECK(code != TStatusCode::OK);
-
- const uint32_t len1 = msg.size;
- const uint32_t len2 = msg2.size;
- const uint32_t size = len1 + ((len2 > 0) ? (2 + len2) : 0);
- auto result = new char[size + 7];
- memcpy(result, &size, sizeof(size));
- result[4] = static_cast<char>(code);
- memcpy(result + 5, &precise_code, sizeof(precise_code));
- memcpy(result + 7, msg.data, len1);
- if (len2 > 0) {
- result[7 + len1] = ':';
- result[8 + len1] = ' ';
- memcpy(result + 9 + len1, msg2.data, len2);
- }
- return result;
-}
-
-const char* Status::copy_state(const char* state) {
- uint32_t size;
- strings::memcpy_inlined(&size, state, sizeof(size));
- auto result = new char[size + 7];
- strings::memcpy_inlined(result, state, size + 7);
- return result;
-}
-
-Status::Status(const TStatus& s) : _state(nullptr) {
+Status::Status(const TStatus& s) {
if (s.status_code != TStatusCode::OK) {
if (s.error_msgs.empty()) {
- _state = assemble_state(s.status_code, Slice(), 1, Slice());
+ assemble_state(s.status_code, Slice(), 1, Slice());
} else {
- _state = assemble_state(s.status_code, s.error_msgs[0], 1,
Slice());
+ assemble_state(s.status_code, s.error_msgs[0], 1, Slice());
}
+ } else {
+ set_ok();
}
}
-Status::Status(const PStatus& s) : _state(nullptr) {
+Status::Status(const PStatus& s) {
TStatusCode::type code = (TStatusCode::type)s.status_code();
if (code != TStatusCode::OK) {
if (s.error_msgs_size() == 0) {
- _state = assemble_state(code, Slice(), 1, Slice());
+ assemble_state(code, Slice(), 1, Slice());
} else {
- _state = assemble_state(code, s.error_msgs(0), 1, Slice());
+ assemble_state(code, s.error_msgs(0), 1, Slice());
}
+ } else {
+ set_ok();
}
}
-Status::Status(TStatusCode::type code, const Slice& msg, int16_t precise_code,
const Slice& msg2)
- : _state(assemble_state(code, msg, precise_code, msg2)) {}
-
void Status::to_thrift(TStatus* s) const {
s->error_msgs.clear();
- if (_state == nullptr) {
+ if (ok()) {
s->status_code = TStatusCode::OK;
} else {
s->status_code = code();
@@ -74,7 +47,7 @@ void Status::to_thrift(TStatus* s) const {
void Status::to_protobuf(PStatus* s) const {
s->clear_error_msgs();
- if (_state == nullptr) {
+ if (ok()) {
s->set_status_code((int)TStatusCode::OK);
} else {
s->set_status_code(code());
@@ -84,9 +57,6 @@ void Status::to_protobuf(PStatus* s) const {
}
std::string Status::code_as_string() const {
- if (_state == nullptr) {
- return "OK";
- }
switch (code()) {
case TStatusCode::OK:
return "OK";
@@ -155,7 +125,7 @@ std::string Status::code_as_string() const {
std::string Status::to_string() const {
std::string result(code_as_string());
- if (_state == nullptr) {
+ if (ok()) {
return result;
}
@@ -172,13 +142,11 @@ std::string Status::to_string() const {
}
Slice Status::message() const {
- if (_state == nullptr) {
+ if (ok()) {
return Slice();
}
- uint32_t length;
- memcpy(&length, _state, sizeof(length));
- return Slice(_state + 7, length);
+ return Slice(_state + HEADER_LEN, _length);
}
Status Status::clone_and_prepend(const Slice& msg) const {
diff --git a/be/src/common/status.h b/be/src/common/status.h
index aba8fc9..5289158 100644
--- a/be/src/common/status.h
+++ b/be/src/common/status.h
@@ -16,30 +16,37 @@
namespace doris {
class Status {
+ enum {
+ STATE_CAPACITY = 256,
+ HEADER_LEN = 7,
+ MESSAGE_LEN = STATE_CAPACITY - HEADER_LEN
+ };
public:
- Status() : _state(nullptr) {}
- ~Status() noexcept { delete[] _state; }
+ Status() : _length(0) {}
// copy c'tor makes copy of error detail so Status can be returned by value
- Status(const Status& s) : _state(s._state == nullptr ? nullptr :
copy_state(s._state)) {}
+ Status(const Status& rhs) {
+ *this = rhs;
+ }
+
+ // move c'tor
+ Status(Status&& rhs) {
+ *this = rhs;
+ }
// same as copy c'tor
- Status& operator=(const Status& s) {
- // The following condition catches both aliasing (when this == &s),
- // and the common case where both s and *this are OK.
- if (_state != s._state) {
- delete[] _state;
- _state = (s._state == nullptr) ? nullptr : copy_state(s._state);
+ Status& operator=(const Status& rhs) {
+ if (rhs._length) {
+ memcpy(_state, rhs._state, rhs._length + Status::HEADER_LEN);
+ } else {
+ _length = 0;
}
return *this;
}
- // move c'tor
- Status(Status&& s) noexcept : _state(s._state) { s._state = nullptr; }
-
// move assign
- Status& operator=(Status&& s) noexcept {
- std::swap(_state, s._state);
+ Status& operator=(Status&& rhs) {
+ this->operator=(rhs);
return *this;
}
@@ -143,7 +150,8 @@ public:
return Status(TStatusCode::DATA_QUALITY_ERROR, msg, precise_code,
msg2);
}
- bool ok() const { return _state == nullptr; }
+ bool ok() const { return _length == 0; }
+ void set_ok() { _length = 0; }
bool is_cancelled() const { return code() == TStatusCode::CANCELLED; }
bool is_mem_limit_exceeded() const { return code() ==
TStatusCode::MEM_LIMIT_EXCEEDED; }
@@ -203,16 +211,11 @@ public:
Slice message() const;
TStatusCode::type code() const {
- return _state == nullptr ? TStatusCode::OK :
static_cast<TStatusCode::type>(_state[4]);
+ return ok() ? TStatusCode::OK : static_cast<TStatusCode::type>(_code);
}
int16_t precise_code() const {
- if (_state == nullptr) {
- return 0;
- }
- int16_t precise_code;
- memcpy(&precise_code, _state + 5, sizeof(precise_code));
- return precise_code;
+ return ok() ? 0 : _precise_code;
}
/// Clone this status and add the specified prefix to the message.
@@ -235,21 +238,66 @@ public:
/// trailing message.
Status clone_and_append(const Slice& msg) const;
- operator bool() { return this->ok(); }
+ operator bool() const { return this->ok(); }
private:
- const char* copy_state(const char* state);
+ void assemble_state(TStatusCode::type code, const Slice& msg, int16_t
precise_code, const Slice& msg2) {
+ DCHECK(code != TStatusCode::OK);
+ uint32_t len1 = msg.size;
+ uint32_t len2 = msg2.size;
+ uint32_t size = len1 + ((len2 > 0) ? (2 + len2) : 0);
+
+ // limited to MESSAGE_LEN
+ if (UNLIKELY(size > MESSAGE_LEN)) {
+ std::string str = code_as_string();
+ str.append(": ");
+ str.append(msg.data, msg.size);
+ char buf[64] = {};
+ int n = snprintf(buf, sizeof(buf), " precise_code:%d ",
precise_code);
+ str.append(buf, n);
+ str.append(msg2.data, msg2.size);
+ LOG(WARNING) << "warning: Status msg truncated, " << str;
+ size = MESSAGE_LEN;
+ }
+
+ _length = size;
+ _code = (char)code;
+ _precise_code = precise_code;
- Status(TStatusCode::type code, const Slice& msg, int16_t precise_code,
const Slice& msg2);
+ // copy msg
+ char* result = _state + HEADER_LEN;
+ uint32_t len = std::min<uint32_t>(len1, MESSAGE_LEN);
+ memcpy(result, msg.data, len);
+
+ // copy msg2
+ if (len2 > 0 && len < MESSAGE_LEN - 2) {
+ result[len++] = ':';
+ result[len++] = ' ';
+ memcpy(&result[len], msg2.data, std::min<uint32_t>(len2,
MESSAGE_LEN - len));
+ }
+ }
+
+ Status(TStatusCode::type code, const Slice& msg, int16_t precise_code,
const Slice& msg2) {
+ assemble_state(code, msg, precise_code, msg2);
+ }
private:
- // OK status has a nullptr _state. Otherwise, _state is a new[] array
+ // OK status has a zero _length. Otherwise, _state is a static array
// of the following form:
// _state[0..3] == length of message
// _state[4] == code
// _state[5..6] == precise_code
// _state[7..] == message
- const char* _state;
+ union {
+ char _state[STATE_CAPACITY];
+
+ struct {
+ int64_t _length : 32; // message length
+ int64_t _code : 8;
+ int64_t _precise_code : 16;
+ int64_t _message : 8; // save message since here
+ };
+ };
};
// some generally useful macros
diff --git a/be/src/runtime/buffered_tuple_stream3.cc
b/be/src/runtime/buffered_tuple_stream3.cc
index e5bdb9e..9a7d8d6 100644
--- a/be/src/runtime/buffered_tuple_stream3.cc
+++ b/be/src/runtime/buffered_tuple_stream3.cc
@@ -892,7 +892,7 @@ bool BufferedTupleStream3::AddRowSlow(TupleRow* row,
Status* status) noexcept {
}
uint8_t* BufferedTupleStream3::AddRowCustomBeginSlow(int64_t size, Status*
status) noexcept {
- bool got_reservation;
+ bool got_reservation = false;
*status = AdvanceWritePage(size, &got_reservation);
if (!status->ok() || !got_reservation) {
return nullptr;
diff --git a/be/src/runtime/minidump.cpp b/be/src/runtime/minidump.cpp
index 3bcbbe8..7d527db 100644
--- a/be/src/runtime/minidump.cpp
+++ b/be/src/runtime/minidump.cpp
@@ -120,7 +120,7 @@ void Minidump::_clean_old_minidump() {
// list all files
std::vector<std::string> files;
- Status st = FileUtils::list_files(Env::Default(),
config::minidump_dir, &files);
+ FileUtils::list_files(Env::Default(), config::minidump_dir, &files);
for (auto it = files.begin(); it != files.end();) {
if (!ends_with(*it, ".dmp")) {
it = files.erase(it);
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
