mapleFU commented on code in PR #2277:
URL: https://github.com/apache/kvrocks/pull/2277#discussion_r1582583968
##########
src/storage/rdb_ziplist.cc:
##########
@@ -152,3 +148,63 @@ Status ZipList::peekOK(size_t n) {
}
uint32_t ZipList::getEncodedLengthSize(uint32_t len) { return len <
ZipListBigLen ? 1 : 5; }
+
+uint32_t ZipList::ZipStorePrevEntryLengthLarge(unsigned char *p, unsigned int
len) {
+ uint32_t u32 = 0;
+ if (p != nullptr) {
+ p[0] = ZipListBigLen;
+ u32 = len;
+ memcpy(p + 1, &u32, sizeof(u32));
+ memrev32ifbe(p + 1);
+ }
+ return 1 + sizeof(uint32_t);
+}
+
+uint32_t ZipList::ZipStorePrevEntryLength(unsigned char *p, unsigned int len) {
+ if (p == nullptr) {
+ return (len < ZipListBigLen) ? 1 : sizeof(uint32_t) + 1;
+ }
+ if (len < ZipListBigLen) {
+ p[0] = len;
+ return 1;
+ }
+ return ZipStorePrevEntryLengthLarge(p, len);
+}
+
+uint32_t ZipList::ZipStoreEntryEncoding(unsigned char *p, unsigned int rawlen)
{
+ unsigned char len = 1, buf[5];
+
+ /* Although encoding is given it may not be set for strings,
+ * so we determine it here using the raw length. */
+ if (rawlen <= 0x3f) {
+ if (!p) return len;
+ buf[0] = ZIP_STR_06B | rawlen;
+ } else if (rawlen <= 0x3fff) {
+ len += 1;
+ if (!p) return len;
+ buf[0] = ZIP_STR_14B | ((rawlen >> 8) & 0x3f);
+ buf[1] = rawlen & 0xff;
+ } else {
+ len += 4;
+ if (!p) return len;
+ buf[0] = ZIP_STR_32B;
+ buf[1] = (rawlen >> 24) & 0xff;
+ buf[2] = (rawlen >> 16) & 0xff;
+ buf[3] = (rawlen >> 8) & 0xff;
+ buf[4] = rawlen & 0xff;
+ }
+
+ /* Store this length at p. */
+ memcpy(p, buf, len);
+ return len;
+}
+
+void ZipList::SetZipListBytes(unsigned char *zl, uint32_t value) {
(*((uint32_t *)(zl))) = value; }
+void ZipList::SetZipListTailOffset(unsigned char *zl, uint32_t value) {
+ (*((uint32_t *)((zl) + sizeof(uint32_t)))) = value;
+}
+void ZipList::SetZipListLength(unsigned char *zl, uint16_t value) {
+ (*((uint16_t *)((zl) + sizeof(uint32_t) * 2))) = value;
Review Comment:
Would you mind use memcpy? Since this might be a type puning here
##########
src/storage/rdb_ziplist.cc:
##########
@@ -152,3 +148,63 @@ Status ZipList::peekOK(size_t n) {
}
uint32_t ZipList::getEncodedLengthSize(uint32_t len) { return len <
ZipListBigLen ? 1 : 5; }
+
+uint32_t ZipList::ZipStorePrevEntryLengthLarge(unsigned char *p, unsigned int
len) {
+ uint32_t u32 = 0;
+ if (p != nullptr) {
+ p[0] = ZipListBigLen;
+ u32 = len;
+ memcpy(p + 1, &u32, sizeof(u32));
+ memrev32ifbe(p + 1);
+ }
+ return 1 + sizeof(uint32_t);
+}
+
+uint32_t ZipList::ZipStorePrevEntryLength(unsigned char *p, unsigned int len) {
+ if (p == nullptr) {
+ return (len < ZipListBigLen) ? 1 : sizeof(uint32_t) + 1;
+ }
+ if (len < ZipListBigLen) {
+ p[0] = len;
+ return 1;
+ }
+ return ZipStorePrevEntryLengthLarge(p, len);
+}
+
+uint32_t ZipList::ZipStoreEntryEncoding(unsigned char *p, unsigned int rawlen)
{
+ unsigned char len = 1, buf[5];
+
+ /* Although encoding is given it may not be set for strings,
+ * so we determine it here using the raw length. */
+ if (rawlen <= 0x3f) {
+ if (!p) return len;
+ buf[0] = ZIP_STR_06B | rawlen;
+ } else if (rawlen <= 0x3fff) {
+ len += 1;
+ if (!p) return len;
+ buf[0] = ZIP_STR_14B | ((rawlen >> 8) & 0x3f);
+ buf[1] = rawlen & 0xff;
+ } else {
+ len += 4;
+ if (!p) return len;
+ buf[0] = ZIP_STR_32B;
+ buf[1] = (rawlen >> 24) & 0xff;
+ buf[2] = (rawlen >> 16) & 0xff;
+ buf[3] = (rawlen >> 8) & 0xff;
+ buf[4] = rawlen & 0xff;
+ }
+
+ /* Store this length at p. */
+ memcpy(p, buf, len);
+ return len;
+}
+
+void ZipList::SetZipListBytes(unsigned char *zl, uint32_t value) {
(*((uint32_t *)(zl))) = value; }
+void ZipList::SetZipListTailOffset(unsigned char *zl, uint32_t value) {
+ (*((uint32_t *)((zl) + sizeof(uint32_t)))) = value;
Review Comment:
ditto, would you mind use memcpy here?
##########
src/storage/rdb_ziplist.cc:
##########
@@ -152,3 +148,63 @@ Status ZipList::peekOK(size_t n) {
}
uint32_t ZipList::getEncodedLengthSize(uint32_t len) { return len <
ZipListBigLen ? 1 : 5; }
+
+uint32_t ZipList::ZipStorePrevEntryLengthLarge(unsigned char *p, unsigned int
len) {
+ uint32_t u32 = 0;
+ if (p != nullptr) {
+ p[0] = ZipListBigLen;
+ u32 = len;
+ memcpy(p + 1, &u32, sizeof(u32));
+ memrev32ifbe(p + 1);
Review Comment:
Just curious why memrev after memcpy? Wouldn't that be faster to rev the
register before writing into memory?
##########
src/storage/rdb_ziplist.cc:
##########
@@ -152,3 +148,63 @@ Status ZipList::peekOK(size_t n) {
}
uint32_t ZipList::getEncodedLengthSize(uint32_t len) { return len <
ZipListBigLen ? 1 : 5; }
+
+uint32_t ZipList::ZipStorePrevEntryLengthLarge(unsigned char *p, unsigned int
len) {
+ uint32_t u32 = 0;
+ if (p != nullptr) {
+ p[0] = ZipListBigLen;
+ u32 = len;
+ memcpy(p + 1, &u32, sizeof(u32));
+ memrev32ifbe(p + 1);
+ }
+ return 1 + sizeof(uint32_t);
+}
+
+uint32_t ZipList::ZipStorePrevEntryLength(unsigned char *p, unsigned int len) {
+ if (p == nullptr) {
+ return (len < ZipListBigLen) ? 1 : sizeof(uint32_t) + 1;
+ }
+ if (len < ZipListBigLen) {
+ p[0] = len;
+ return 1;
+ }
+ return ZipStorePrevEntryLengthLarge(p, len);
+}
+
+uint32_t ZipList::ZipStoreEntryEncoding(unsigned char *p, unsigned int rawlen)
{
+ unsigned char len = 1, buf[5];
+
+ /* Although encoding is given it may not be set for strings,
+ * so we determine it here using the raw length. */
+ if (rawlen <= 0x3f) {
+ if (!p) return len;
+ buf[0] = ZIP_STR_06B | rawlen;
+ } else if (rawlen <= 0x3fff) {
+ len += 1;
+ if (!p) return len;
+ buf[0] = ZIP_STR_14B | ((rawlen >> 8) & 0x3f);
+ buf[1] = rawlen & 0xff;
+ } else {
+ len += 4;
+ if (!p) return len;
+ buf[0] = ZIP_STR_32B;
+ buf[1] = (rawlen >> 24) & 0xff;
+ buf[2] = (rawlen >> 16) & 0xff;
+ buf[3] = (rawlen >> 8) & 0xff;
+ buf[4] = rawlen & 0xff;
+ }
+
+ /* Store this length at p. */
+ memcpy(p, buf, len);
+ return len;
+}
+
+void ZipList::SetZipListBytes(unsigned char *zl, uint32_t value) {
(*((uint32_t *)(zl))) = value; }
Review Comment:
Would you mind use memcpy here?
##########
src/storage/rdb.cc:
##########
@@ -1005,3 +1000,35 @@ Status RDB::rdbSaveBinaryDoubleValue(double val) {
memrev64ifbe(&val);
return stream_->Write((const char *)(&val), sizeof(val));
}
+
+Status RDB::rdbSaveZipListObject(const std::string &elem) {
+ // calc total ziplist size
+ uint prevlen = 0;
+ const size_t ziplist_size = zlHeaderSize + zlEndSize + elem.length() +
+ ZipList::ZipStorePrevEntryLength(nullptr,
prevlen) +
+ ZipList::ZipStoreEntryEncoding(nullptr,
elem.length());
+ auto zl_string = std::string(ziplist_size, 0);
+ auto zl_ptr = reinterpret_cast<unsigned char *>(&zl_string[0]);
+
+ // set ziplist header
+ ZipList::SetZipListBytes(zl_ptr, intrev32ifbe(ziplist_size));
Review Comment:
`ziplist_size` is not a uint32, right? Would we check and cast it to u32?
##########
src/storage/rdb_ziplist.cc:
##########
@@ -152,3 +148,63 @@ Status ZipList::peekOK(size_t n) {
}
uint32_t ZipList::getEncodedLengthSize(uint32_t len) { return len <
ZipListBigLen ? 1 : 5; }
+
+uint32_t ZipList::ZipStorePrevEntryLengthLarge(unsigned char *p, unsigned int
len) {
+ uint32_t u32 = 0;
+ if (p != nullptr) {
+ p[0] = ZipListBigLen;
+ u32 = len;
+ memcpy(p + 1, &u32, sizeof(u32));
+ memrev32ifbe(p + 1);
+ }
+ return 1 + sizeof(uint32_t);
+}
+
+uint32_t ZipList::ZipStorePrevEntryLength(unsigned char *p, unsigned int len) {
+ if (p == nullptr) {
+ return (len < ZipListBigLen) ? 1 : sizeof(uint32_t) + 1;
+ }
+ if (len < ZipListBigLen) {
+ p[0] = len;
+ return 1;
+ }
+ return ZipStorePrevEntryLengthLarge(p, len);
+}
+
+uint32_t ZipList::ZipStoreEntryEncoding(unsigned char *p, unsigned int rawlen)
{
+ unsigned char len = 1, buf[5];
+
+ /* Although encoding is given it may not be set for strings,
+ * so we determine it here using the raw length. */
+ if (rawlen <= 0x3f) {
+ if (!p) return len;
+ buf[0] = ZIP_STR_06B | rawlen;
+ } else if (rawlen <= 0x3fff) {
+ len += 1;
+ if (!p) return len;
+ buf[0] = ZIP_STR_14B | ((rawlen >> 8) & 0x3f);
+ buf[1] = rawlen & 0xff;
+ } else {
+ len += 4;
+ if (!p) return len;
+ buf[0] = ZIP_STR_32B;
+ buf[1] = (rawlen >> 24) & 0xff;
+ buf[2] = (rawlen >> 16) & 0xff;
+ buf[3] = (rawlen >> 8) & 0xff;
+ buf[4] = rawlen & 0xff;
+ }
+
+ /* Store this length at p. */
+ memcpy(p, buf, len);
+ return len;
+}
+
Review Comment:
Besides, would we also applying length check for these set below?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
