commit d7bfcaf57c1fe1d84880e0d31fab9c3d303cdb79
author Florin Malita <fmalita@chromium.org> Thu Jun 14 18:03:26 2018 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Fri Jun 15 12:27:04 2018 +0000
tree 298fe73fcf9402a0212ed66e5c3f3b6bb3a05add
parent 3f13bcba3e5c8234ddd0c36e7758513bb08a72e4

[skjson] Some short string optimizations

1) skip redundant \0 terminator => Value is always zero-initialized

2) skip storing a len record => strlen is cheap for short strings

Change-Id: I3c10c9b9cf6155b95124e2c0194c59e9531a7ca4
Reviewed-on: https://skia-review.googlesource.com/135049
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>

modules/skjson/include/SkJSON.h

diff --git a/modules/skjson/include/SkJSON.h b/modules/skjson/include/SkJSON.h
index 1b46c4a..8baf9fb 100644
--- a/modules/skjson/include/SkJSON.h
+++ b/modules/skjson/include/SkJSON.h
@@ -12,6 +12,8 @@
 #include "SkTo.h"
 #include "SkTypes.h"
 
+#include <cstring>
+
 class SkString;
 class SkWStream;
 
@@ -282,7 +284,11 @@
     size_t size() const {
         switch (this->getTag()) {
         case Tag::kShortString:
-            return kMaxInlineStringSize - SkToSizeT(this->cast<char>()[kMaxInlineStringSize]);
+            // We don't bother storing a length for short strings on the assumption
+            // that strlen is fast in this case.  If this becomes problematic, we
+            // can either go back to storing (7-len) in the tag byte or write a fast
+            // short_strlen.
+            return strlen(this->cast<char>());
         case Tag::kString:
             return this->cast<VectorValue<char, Value::Type::kString>>()->size();
         default:
@@ -297,15 +303,10 @@
     }
 
     const char* end() const {
-        if (this->getTag() == Tag::kShortString) {
-            const auto* payload = this->cast<char>();
-            return payload + kMaxInlineStringSize - SkToSizeT(payload[kMaxInlineStringSize]);
-        }
-        return this->cast<VectorValue<char, Value::Type::kString>>()->end();
+        return this->getTag() == Tag::kShortString
+            ? strchr(this->cast<char>(), '\0')
+            : this->cast<VectorValue<char, Value::Type::kString>>()->end();
     }
-
-private:
-    static constexpr size_t kMaxInlineStringSize = sizeof(Value) - 1;
 };
 
 struct Member {

modules/skjson/src/SkJSON.cpp

diff --git a/modules/skjson/src/SkJSON.cpp b/modules/skjson/src/SkJSON.cpp
index d6585e0..7f082d7 100644
--- a/modules/skjson/src/SkJSON.cpp
+++ b/modules/skjson/src/SkJSON.cpp
@@ -7,6 +7,7 @@
 
 #include "SkJSON.h"
 
+#include "SkMalloc.h"
 #include "SkStream.h"
 #include "SkString.h"
 
@@ -82,12 +83,9 @@
     // The Ts are already in memory, so their size should be safe.
     const auto total_size = sizeof(size_t) + size * sizeof(T) + extra_alloc_size;
     auto* size_ptr = reinterpret_cast<size_t*>(alloc.makeBytesAlignedTo(total_size, kRecAlign));
-    *size_ptr = size;
 
-    if (size) {
-        auto* data_ptr = reinterpret_cast<void*>(size_ptr + 1);
-        memcpy(data_ptr, src, size * sizeof(T));
-    }
+    *size_ptr = size;
+    sk_careful_memcpy(size_ptr + 1, src, size * sizeof(T));
 
     return size_ptr;
 }
@@ -112,6 +110,7 @@
 // The string data plus a null-char terminator are copied over.
 //
 StringValue::StringValue(const char* src, size_t size, SkArenaAlloc& alloc) {
+    static constexpr size_t kMaxInlineStringSize = sizeof(Value) - 1;
     if (size > kMaxInlineStringSize) {
         this->init_tagged_pointer(Tag::kString, MakeVector<char, 1>(src, size, alloc));
 
@@ -122,19 +121,11 @@
     }
 
     this->init_tagged(Tag::kShortString);
+    sk_careful_memcpy(this->cast<char>(), src, size);
 
-    auto* payload = this->cast<char>();
-    if (size) {
-        memcpy(payload, src, size);
-        payload[size] = '\0';
-    }
-
-    const auto len_tag = SkTo<char>(kMaxInlineStringSize - size);
-    // This technically overwrites the tag, but is safe because
-    //   1) kShortString == 0
-    //   2) 0 <= len_tag <= 7
+    // Null terminator provided by init_tagged() above (fData8 is zero-initialized).
+    // This is safe because kShortString is also 0 and can act as a terminator when size == 7.
     static_assert(static_cast<uint8_t>(Tag::kShortString) == 0, "please don't break this");
-    payload[kMaxInlineStringSize] = len_tag;
 
     SkASSERT(this->getTag() == Tag::kShortString);
 }