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/27 13:36:58 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #1358: Rewrite encoding functions
PragmaTwice opened a new pull request, #1358:
URL: https://github.com/apache/incubator-kvrocks/pull/1358
We use `if constexpr`, `__builtin_bswapN` and `__builtin_memcpy` for better performance and maintainability.
--
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] torwig commented on a diff in pull request #1358: Rewrite encoding functions
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1358:
URL: https://github.com/apache/incubator-kvrocks/pull/1358#discussion_r1149399970
##########
src/common/encoding.cc:
##########
@@ -217,50 +206,40 @@ bool GetDouble(rocksdb::Slice *input, double *value) {
}
uint16_t DecodeFixed16(const char *ptr) {
- if (BYTE_ORDER == BIG_ENDIAN) {
- uint16_t value = 0;
- memcpy(&value, ptr, sizeof(value));
- return value;
- } else {
- return ((static_cast<uint16_t>(static_cast<uint8_t>(ptr[1]))) |
- (static_cast<uint16_t>(static_cast<uint8_t>(ptr[0])) << 8));
- }
+ uint16_t value = 0;
+
+ __builtin_memcpy(&value, ptr, sizeof(value));
+
+ return BYTE_ORDER == LITTLE_ENDIAN ? __builtin_bswap16(value) : value;
}
uint32_t DecodeFixed32(const char *ptr) {
- if (BYTE_ORDER == BIG_ENDIAN) {
- uint32_t value = 0;
- memcpy(&value, ptr, sizeof(value));
- return value;
- } else {
- return ((static_cast<uint32_t>(static_cast<uint8_t>(ptr[3]))) |
- (static_cast<uint32_t>(static_cast<uint8_t>(ptr[2])) << 8) |
- (static_cast<uint32_t>(static_cast<uint8_t>(ptr[1])) << 16) |
- (static_cast<uint32_t>(static_cast<uint8_t>(ptr[0])) << 24));
- }
+ uint32_t value = 0;
+
+ __builtin_memcpy(&value, ptr, sizeof(value));
+
+ return BYTE_ORDER == LITTLE_ENDIAN ? __builtin_bswap32(value) : value;
Review Comment:
@PragmaTwice Is there a possibility to use `if constexpr` here with the condition? Or it wouldn't change anything?
--
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 #1358: Rewrite encoding functions
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1358:
URL: https://github.com/apache/incubator-kvrocks/pull/1358#discussion_r1149482055
##########
src/common/encoding.cc:
##########
@@ -217,50 +206,40 @@ bool GetDouble(rocksdb::Slice *input, double *value) {
}
uint16_t DecodeFixed16(const char *ptr) {
- if (BYTE_ORDER == BIG_ENDIAN) {
- uint16_t value = 0;
- memcpy(&value, ptr, sizeof(value));
- return value;
- } else {
- return ((static_cast<uint16_t>(static_cast<uint8_t>(ptr[1]))) |
- (static_cast<uint16_t>(static_cast<uint8_t>(ptr[0])) << 8));
- }
+ uint16_t value = 0;
+
+ __builtin_memcpy(&value, ptr, sizeof(value));
+
+ return BYTE_ORDER == LITTLE_ENDIAN ? __builtin_bswap16(value) : value;
}
uint32_t DecodeFixed32(const char *ptr) {
- if (BYTE_ORDER == BIG_ENDIAN) {
- uint32_t value = 0;
- memcpy(&value, ptr, sizeof(value));
- return value;
- } else {
- return ((static_cast<uint32_t>(static_cast<uint8_t>(ptr[3]))) |
- (static_cast<uint32_t>(static_cast<uint8_t>(ptr[2])) << 8) |
- (static_cast<uint32_t>(static_cast<uint8_t>(ptr[1])) << 16) |
- (static_cast<uint32_t>(static_cast<uint8_t>(ptr[0])) << 24));
- }
+ uint32_t value = 0;
+
+ __builtin_memcpy(&value, ptr, sizeof(value));
+
+ return BYTE_ORDER == LITTLE_ENDIAN ? __builtin_bswap32(value) : value;
Review Comment:
Yes, `if constexpr` is just a constraint in this case. Compiler will eliminate constant conditions.
--
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 merged pull request #1358: Rewrite encoding functions
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #1358:
URL: https://github.com/apache/incubator-kvrocks/pull/1358
--
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