Reland [skjson] Implementation/API tweaks

  *  move most common accessor methods to the header, for inlining
  *  drop the lazy type checking semantics in favor of explicit guarded/unguarded
     conversions
  *  revisit the public class hierarchy to better constrain type-bound APIs
  *  expose public type factories and add tests
  *  drop the empty-vector optimization -- allocating an external size_t in these
     uncommon cases is better than paying for a conditional on every access.

TBR=
Change-Id: Ic609bb74f12cad1756865a2489ad56c03ecc5494
Reviewed-on: https://skia-review.googlesource.com/134845
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@chromium.org>

3 files changed, 496 insertions, 393 deletions

diff --git a/modules/skjson/include/SkJSON.h b/modules/skjson/include/SkJSON.h
index 4cb312c64f..99da29de72 100644
--- a/modules/skjson/include/SkJSON.h
+++ b/modules/skjson/include/SkJSON.h
@@ -9,8 +9,10 @@
 #define SkJSON_DEFINED
 
 #include "SkArenaAlloc.h"
+#include "SkTo.h"
 #include "SkTypes.h"
 
+class SkString;
 class SkWStream;
 
 namespace skjson {
@@ -27,12 +29,26 @@ namespace skjson {
  *
  *  Values are opaque, fixed-size (64 bits), immutable records.
  *
- *  They can be freely converted to any of the facade types for type-specific functionality.
+ *  They can be converted to facade types for type-specific functionality.
  *
- *  Note: type checking is lazy/deferred, to facilitate chained property access - e.g.
+ *  E.g.:
  *
- *      if (!v.as<ObjectValue>()["foo"].as<ObjectValue>()["bar"].is<NullValue>())
- *          LOG("found v.foo.bar!");
+ *     if (v.is<ArrayValue>()) {
+ *         for (const auto& item : v.as<ArrayValue>()) {
+ *             if (const NumberValue* n = item) {
+ *                 printf("Found number: %f", **n);
+ *             }
+ *         }
+ *     }
+ *
+ *     if (v.is<ObjectValue>()) {
+ *         const StringValue* id = v.as<ObjectValue>()["id"];
+ *         if (id) {
+ *             printf("Found object ID: %s", id->begin());
+ *         } else {
+ *             printf("Missing object ID");
+ *         }
+ *     }
  */
 class alignas(8) Value {
 public:
@@ -46,7 +62,7 @@ public:
     };
 
     /**
-     * @return    The public type of this record.
+     * @return    The type of this value.
      */
     Type getType() const;
 
@@ -54,85 +70,289 @@ public:
      * @return    True if the record matches the facade type T.
      */
     template <typename T>
-    bool is() const { return T::IsType(this->getType()); }
+    bool is() const { return this->getType() == T::kType; }
 
     /**
-     * @return    The record cast as facade type T.
+     * Unguarded conversion to facade types.
      *
-     * Note: this is always safe, as proper typing is enforced in the facade methods.
+     * @return    The record cast as facade type T&.
      */
     template <typename T>
     const T& as() const {
-        return *reinterpret_cast<const T*>(this->is<T>() ? this : &Value::Null());
+        SkASSERT(this->is<T>());
+        return *reinterpret_cast<const T*>(this);
     }
 
     /**
-     * @return    Null value singleton.
+     * Guarded conversion to facade types.
+     *
+     * @return    The record cast as facade type T*.
      */
-    static const Value& Null();
+    template <typename T>
+    operator const T*() const {
+        return this->is<T>() ? &this->as<T>() : nullptr;
+    }
+
+    /**
+     * @return    The string representation of this value.
+     */
+    SkString toString() const;
 
 protected:
-    uint8_t   fData8[8];
+    /*
+      Value implementation notes:
+
+        -- fixed 64-bit size
+
+        -- 8-byte aligned
+
+        -- union of:
+
+             bool
+             int32
+             float
+             char[8] (short string storage)
+             external payload (tagged) pointer
+
+         -- highest 3 bits reserved for type storage
+
+     */
+    enum class Tag : uint8_t {
+        // We picked kShortString == 0 so that tag 0x00 and stored max_size-size (7-7=0)
+        // conveniently overlap the '\0' terminator, allowing us to store a 7 character
+        // C string inline.
+        kShortString                  = 0b00000000,  // inline payload
+        kNull                         = 0b00100000,  // no payload
+        kBool                         = 0b01000000,  // inline payload
+        kInt                          = 0b01100000,  // inline payload
+        kFloat                        = 0b10000000,  // inline payload
+        kString                       = 0b10100000,  // ptr to external storage
+        kArray                        = 0b11000000,  // ptr to external storage
+        kObject                       = 0b11100000,  // ptr to external storage
+    };
+    static constexpr uint8_t kTagMask = 0b11100000;
+
+    void init_tagged(Tag);
+    void init_tagged_pointer(Tag, void*);
+
+    Tag getTag() const {
+        return static_cast<Tag>(fData8[kTagOffset] & kTagMask);
+    }
+
+    // Access the record data as T.
+    //
+    // This is also used to access the payload for inline records.  Since the record type lives in
+    // the high bits, sizeof(T) must be less than sizeof(Value) when accessing inline payloads.
+    //
+    // E.g.
+    //
+    //   uint8_t
+    //    -----------------------------------------------------------------------
+    //   |  val8  |  val8  |  val8  |  val8  |  val8  |  val8  |  val8  |    TYPE|
+    //    -----------------------------------------------------------------------
+    //
+    //   uint32_t
+    //    -----------------------------------------------------------------------
+    //   |               val32               |          unused          |    TYPE|
+    //    -----------------------------------------------------------------------
+    //
+    //   T* (64b)
+    //    -----------------------------------------------------------------------
+    //   |                        T* (kTypeShift bits)                      |TYPE|
+    //    -----------------------------------------------------------------------
+    //
+    template <typename T>
+    const T* cast() const {
+        static_assert(sizeof (T) <=  sizeof(Value), "");
+        static_assert(alignof(T) <= alignof(Value), "");
+        return reinterpret_cast<const T*>(this);
+    }
+
+    template <typename T>
+    T* cast() { return const_cast<T*>(const_cast<const Value*>(this)->cast<T>()); }
+
+    // Access the pointer payload.
+    template <typename T>
+    const T* ptr() const {
+        static_assert(sizeof(uintptr_t)     == sizeof(Value) ||
+                      sizeof(uintptr_t) * 2 == sizeof(Value), "");
+
+        return (sizeof(uintptr_t) < sizeof(Value))
+            // For 32-bit, pointers are stored unmodified.
+            ? *this->cast<const T*>()
+            // For 64-bit, we use the high bits of the pointer as tag storage.
+            : reinterpret_cast<T*>(*this->cast<uintptr_t>() & kTagPointerMask);
+    }
+
+private:
+    static constexpr size_t kValueSize = 8;
+
+    uint8_t fData8[kValueSize];
+
+#if defined(SK_CPU_LENDIAN)
+    static constexpr size_t kTagOffset = kValueSize - 1;
+
+    static constexpr uintptr_t kTagPointerMask =
+            ~(static_cast<uintptr_t>(kTagMask) << ((sizeof(uintptr_t) - 1) * 8));
+#else
+    // The current value layout assumes LE and will take some tweaking for BE.
+    static_assert(false, "Big-endian builds are not supported at this time.");
+#endif
 };
 
 class NullValue final : public Value {
 public:
-    static bool IsType(Value::Type t) { return t == Type::kNull; }
+    static constexpr Type kType = Type::kNull;
+
+    NullValue();
 };
 
-template <typename T, Value::Type vtype>
-class PrimitiveValue final : public Value {
+class BoolValue final : public Value {
 public:
-    static bool IsType(Value::Type t) { return t == vtype; }
+    static constexpr Type kType = Type::kBool;
+
+    explicit BoolValue(bool);
 
-    T operator *() const;
+    bool operator *() const {
+        SkASSERT(this->getTag() == Tag::kBool);
+        return *this->cast<bool>();
+    }
+};
+
+class NumberValue final : public Value {
+public:
+    static constexpr Type kType = Type::kNumber;
+
+    explicit NumberValue(int32_t);
+    explicit NumberValue(float);
+
+    double operator *() const {
+        SkASSERT(this->getTag() == Tag::kInt ||
+                 this->getTag() == Tag::kFloat);
+
+        return this->getTag() == Tag::kInt
+            ? static_cast<double>(*this->cast<int32_t>())
+            : static_cast<double>(*this->cast<float>());
+    }
 };
 
 template <typename T, Value::Type vtype>
 class VectorValue : public Value {
 public:
-    static bool IsType(Value::Type t) { return t == vtype; }
+    using ValueT = T;
+    static constexpr Type kType = vtype;
 
-    size_t size() const;
+    size_t size() const {
+        SkASSERT(this->getType() == kType);
+        return *this->ptr<size_t>();
+    }
 
-    const T* begin() const;
-    const T*   end() const;
+    const T* begin() const {
+        SkASSERT(this->getType() == kType);
+        const auto* size_ptr = this->ptr<size_t>();
+        return reinterpret_cast<const T*>(size_ptr + 1);
+    }
+
+    const T* end() const {
+        SkASSERT(this->getType() == kType);
+        const auto* size_ptr = this->ptr<size_t>();
+        return reinterpret_cast<const T*>(size_ptr + 1) + *size_ptr;
+    }
 
     const T& operator[](size_t i) const {
-        return (i < this->size()) ? *(this->begin() + i) : T::Null();
+        SkASSERT(this->getType() == kType);
+        SkASSERT(i < this->size());
+
+        return *(this->begin() + i);
     }
 };
 
-using   BoolValue = PrimitiveValue<bool  , Value::Type::kBool  >;
-using NumberValue = PrimitiveValue<double, Value::Type::kNumber>;
-using StringValue =    VectorValue<char  , Value::Type::kString>;
-using  ArrayValue =    VectorValue<Value , Value::Type::kArray >;
+class ArrayValue final : public VectorValue<Value, Value::Type::kArray> {
+public:
+    ArrayValue(const Value* src, size_t size, SkArenaAlloc& alloc);
+};
+
+class StringValue final : public Value {
+public:
+    static constexpr Type kType = Type::kString;
+
+    StringValue();
+    StringValue(const char* src, size_t size, SkArenaAlloc& alloc);
+
+    size_t size() const {
+        switch (this->getTag()) {
+        case Tag::kShortString:
+            return kMaxInlineStringSize - SkToSizeT(this->cast<char>()[kMaxInlineStringSize]);
+        case Tag::kString:
+            return this->cast<VectorValue<char, Value::Type::kString>>()->size();
+        default:
+            return 0;
+        }
+    }
+
+    const char* begin() const {
+        return this->getTag() == Tag::kShortString
+            ? this->cast<char>()
+            : this->cast<VectorValue<char, Value::Type::kString>>()->begin();
+    }
+
+    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();
+    }
+
+private:
+    static constexpr size_t kMaxInlineStringSize = sizeof(Value) - 1;
+};
 
 struct Member {
     StringValue fKey;
           Value fValue;
-
-    static const Member& Null();
 };
 
 class ObjectValue final : public VectorValue<Member, Value::Type::kObject> {
 public:
+    ObjectValue(const Member* src, size_t size, SkArenaAlloc& alloc);
+
     const Value& operator[](const char*) const;
+
+private:
+    // Not particularly interesting - hiding for disambiguation.
+    const Member& operator[](size_t i) const = delete;
 };
 
 class DOM final : public SkNoncopyable {
 public:
     explicit DOM(const char*);
 
-    const Value& root() const { return *fRoot; }
+    const Value& root() const { return fRoot; }
 
     void write(SkWStream*) const;
 
 private:
     SkArenaAlloc fAlloc;
-    const Value* fRoot;
+    Value        fRoot;
 };
 
+inline Value::Type Value::getType() const {
+    switch (this->getTag()) {
+    case Tag::kNull:        return Type::kNull;
+    case Tag::kBool:        return Type::kBool;
+    case Tag::kInt:         return Type::kNumber;
+    case Tag::kFloat:       return Type::kNumber;
+    case Tag::kShortString: return Type::kString;
+    case Tag::kString:      return Type::kString;
+    case Tag::kArray:       return Type::kArray;
+    case Tag::kObject:      return Type::kObject;
+    }
+
+    SkASSERT(false); // unreachable
+    return Type::kNull;
+}
+
 } // namespace skjson
 
 #endif // SkJSON_DEFINED
diff --git a/modules/skjson/src/SkJSON.cpp b/modules/skjson/src/SkJSON.cpp
index 8af85ba695..b513c78536 100644
--- a/modules/skjson/src/SkJSON.cpp
+++ b/modules/skjson/src/SkJSON.cpp
@@ -9,7 +9,6 @@
 
 #include "SkStream.h"
 #include "SkString.h"
-#include "SkTo.h"
 
 #include <cmath>
 #include <vector>
@@ -18,357 +17,131 @@ namespace skjson {
 
 //#define SK_JSON_REPORT_ERRORS
 
-namespace {
-
-/*
-  Value's impl side:
-
-    -- fixed 64-bit size
-
-    -- 8-byte aligned
-
-    -- union of:
-
-         bool
-         int32
-         float
-         char[8] (short string storage)
-         external payload pointer
 
-     -- highest 3 bits reserved for type storage
-
- */
 static_assert( sizeof(Value) == 8, "");
 static_assert(alignof(Value) == 8, "");
 
 static constexpr size_t kRecAlign = alignof(Value);
 
-// The current record layout assumes LE and will take some tweaking for BE.
-#if defined(SK_CPU_BENDIAN)
-static_assert(false, "Big-endian builds are not supported.");
-#endif
-
-class ValueRec : public Value {
-public:
-    static constexpr uint64_t kTypeBits  = 3,
-                              kTypeShift = 64 - kTypeBits,
-                              kTypeMask  = ((1ULL << kTypeBits) - 1) << kTypeShift;
-
-    enum RecType : uint64_t {
-        // We picked kShortString == 0 so that tag 0b000 and stored max_size-size (7-7=0)
-        // conveniently overlap the '\0' terminator, allowing us to store a 7 character
-        // C string inline.
-        kShortString = 0b000ULL << kTypeShift,  // inline payload
-        kNull        = 0b001ULL << kTypeShift,  // no payload
-        kBool        = 0b010ULL << kTypeShift,  // inline payload
-        kInt         = 0b011ULL << kTypeShift,  // inline payload
-        kFloat       = 0b100ULL << kTypeShift,  // inline payload
-        kString      = 0b101ULL << kTypeShift,  // ptr to external storage
-        kArray       = 0b110ULL << kTypeShift,  // ptr to external storage
-        kObject      = 0b111ULL << kTypeShift,  // ptr to external storage
-    };
-
-    RecType getRecType() const {
-        return static_cast<RecType>(*this->cast<uint64_t>() & kTypeMask);
-    }
-
-    // Access the record data as T.
-    //
-    // This is also used to access the payload for inline records.  Since the record type lives in
-    // the high bits, sizeof(T) must be less than sizeof(Value) when accessing inline payloads.
-    //
-    // E.g.
-    //
-    //   uint8_t
-    //    -----------------------------------------------------------------------
-    //   |  val8  |  val8  |  val8  |  val8  |  val8  |  val8  |  val8  |    TYPE|
-    //    -----------------------------------------------------------------------
-    //
-    //   uint32_t
-    //    -----------------------------------------------------------------------
-    //   |               val32               |          unused          |    TYPE|
-    //    -----------------------------------------------------------------------
-    //
-    //   T* (64b)
-    //    -----------------------------------------------------------------------
-    //   |                        T* (kTypeShift bits)                      |TYPE|
-    //    -----------------------------------------------------------------------
-    //
-    template <typename T>
-    const T* cast() const {
-        static_assert(sizeof (T) <=  sizeof(ValueRec), "");
-        static_assert(alignof(T) <= alignof(ValueRec), "");
-        return reinterpret_cast<const T*>(this);
-    }
-
-    template <typename T>
-    T* cast() { return const_cast<T*>(const_cast<const ValueRec*>(this)->cast<T>()); }
-
-    // Access the pointer payload.
-    template <typename T>
-    const T* ptr() const {
-        static_assert(sizeof(uintptr_t)     == sizeof(Value) ||
-                      sizeof(uintptr_t) * 2 == sizeof(Value), "");
-
-        return (sizeof(uintptr_t) < sizeof(Value))
-            // For 32-bit, pointers are stored unmodified.
-            ? *this->cast<const T*>()
-            // For 64-bit, we use the high bits of the pointer as type storage.
-            : reinterpret_cast<T*>(*this->cast<uintptr_t>() & ~kTypeMask);
-    }
-
-    // Type-bound recs only store their type.
-    static ValueRec MakeTypeBound(RecType t) {
-        ValueRec v;
-        *v.cast<uint64_t>() = t;
-        SkASSERT(v.getRecType() == t);
-        return v;
-    }
-
-    // Primitive recs store a type and inline primitive payload.
-    template <typename T>
-    static ValueRec MakePrimitive(RecType t, T src) {
-        ValueRec v = MakeTypeBound(t);
-        *v.cast<T>() = src;
-        SkASSERT(v.getRecType() == t);
-        return v;
-    }
-
-    // Pointer recs store a type (in the upper kTypeBits bits) and a pointer.
-    template <typename T>
-    static ValueRec MakePtr(RecType t, const T* p) {
-        SkASSERT((t & kTypeMask) == t);
-        if (sizeof(uintptr_t) == sizeof(Value)) {
-            // For 64-bit, we rely on the pointer hi bits being unused.
-            SkASSERT(!(reinterpret_cast<uintptr_t>(p) & kTypeMask));
-        }
-
-        ValueRec v = MakeTypeBound(t);
-        *v.cast<uintptr_t>() |= reinterpret_cast<uintptr_t>(p);
-
-        SkASSERT(v.getRecType() == t);
-        SkASSERT(v.ptr<T>() == p);
-
-        return v;
-    }
-
-    // Vector recs point to externally allocated slabs with the following layout:
-    //
-    //   [size_t n] [REC_0] ... [REC_n-1] [optional extra trailing storage]
-    //
-    // Long strings use extra_alloc_size == 1 to store the \0 terminator.
-    template <typename T, size_t extra_alloc_size = 0>
-    static ValueRec MakeVector(RecType t, const T* src, size_t size, SkArenaAlloc& alloc) {
-        // For zero-size arrays, we just store a nullptr.
-        size_t* size_ptr = nullptr;
-
-        if (size) {
-            // The Ts are already in memory, so their size should be safeish.
-            const auto total_size = sizeof(size_t) + sizeof(T) * size + extra_alloc_size;
-            size_ptr = reinterpret_cast<size_t*>(alloc.makeBytesAlignedTo(total_size, kRecAlign));
-            auto* data_ptr = reinterpret_cast<T*>(size_ptr + 1);
-            *size_ptr = size;
-            memcpy(data_ptr, src, sizeof(T) * size);
-        }
-
-        return MakePtr(t, size_ptr);
-    }
-
-    size_t vectorSize(RecType t) const {
-        if (this->is<NullValue>()) return 0;
-        SkASSERT(this->getRecType() == t);
-
-        const auto* size_ptr = this->ptr<const size_t>();
-        return size_ptr ? *size_ptr : 0;
-    }
-
-    template <typename T>
-    const T* vectorBegin(RecType t) const {
-        if (this->is<NullValue>()) return nullptr;
-        SkASSERT(this->getRecType() == t);
-
-        const auto* size_ptr = this->ptr<const size_t>();
-        return size_ptr ? reinterpret_cast<const T*>(size_ptr + 1) : nullptr;
-    }
-
-    template <typename T>
-    const T* vectorEnd(RecType t) const {
-        if (this->is<NullValue>()) return nullptr;
-        SkASSERT(this->getRecType() == t);
-
-        const auto* size_ptr = this->ptr<const size_t>();
-        return size_ptr ? reinterpret_cast<const T*>(size_ptr + 1) + *size_ptr : nullptr;
-    }
-
-    // Strings have two flavors:
-    //
-    // -- short strings (len <= 7) -> these are stored inline, in the record
-    //    (one byte reserved for null terminator/type):
-    //
-    //        [str] [\0]|[max_len - actual_len]
-    //
-    //    Storing [max_len - actual_len] allows the 'len' field to double-up as a
-    //    null terminator when size == max_len (this works 'cause kShortString == 0).
-    //
-    // -- long strings (len > 7) -> these are externally allocated vectors (VectorRec<char>).
-    //
-    // The string data plus a null-char terminator are copied over.
-    static constexpr size_t kMaxInlineStringSize = sizeof(Value) - 1;
-
-    static ValueRec MakeString(const char* src, size_t size, SkArenaAlloc& alloc) {
-        ValueRec v;
-
-        if (size > kMaxInlineStringSize) {
-            v = MakeVector<char, 1>(kString, src, size, alloc);
-            const_cast<char *>(v.vectorBegin<char>(ValueRec::kString))[size] = '\0';
-        } else {
-            v = MakeTypeBound(kShortString);
-            auto* payload = v.cast<char>();
-            memcpy(payload, src, size);
-            payload[size] = '\0';
-
-            const auto len_tag = SkTo<char>(kMaxInlineStringSize - size);
-            // This technically overwrites the type hi bits, but is safe because
-            //   1) kShortString == 0
-            //   2) 0 <= len_tag <= 7
-            static_assert(kShortString == 0, "please don't break this");
-            payload[kMaxInlineStringSize] = len_tag;
-            SkASSERT(v.getRecType() == kShortString);
-        }
-        return v;
-    }
-
-    size_t stringSize() const {
-        if (this->getRecType() == ValueRec::kShortString) {
-            const auto* payload = this->cast<char>();
-            return kMaxInlineStringSize - SkToSizeT(payload[kMaxInlineStringSize]);
-        }
-
-        return this->vectorSize(ValueRec::kString);
-    }
-
-    const char* stringBegin() const {
-        if (this->getRecType() == ValueRec::kShortString) {
-            return this->cast<char>();
-        }
-
-        return this->vectorBegin<char>(ValueRec::kString);
-    }
+void Value::init_tagged(Tag t) {
+    memset(fData8, 0, sizeof(fData8));
+    fData8[Value::kTagOffset] = SkTo<uint8_t>(t);
+    SkASSERT(this->getTag() == t);
+}
 
-    const char* stringEnd() const {
-        if (this->getRecType() == ValueRec::kShortString) {
-            const auto* payload = this->cast<char>();
-            return payload + kMaxInlineStringSize - SkToSizeT(payload[kMaxInlineStringSize]);
-        }
+// Pointer values store a type (in the upper kTagBits bits) and a pointer.
+void Value::init_tagged_pointer(Tag t, void* p) {
+    *this->cast<uintptr_t>() = reinterpret_cast<uintptr_t>(p);
 
-        return this->vectorEnd<char>(ValueRec::kString);
+    if (sizeof(Value) == sizeof(uintptr_t)) {
+        // For 64-bit, we rely on the pointer upper bits being unused/zero.
+        SkASSERT(!(fData8[kTagOffset] & kTagMask));
+        fData8[kTagOffset] |= SkTo<uint8_t>(t);
+    } else {
+        // For 32-bit, we need to zero-initialize the upper 32 bits
+        SkASSERT(sizeof(Value) == sizeof(uintptr_t) * 2);
+        this->cast<uintptr_t>()[kTagOffset >> 2] = 0;
+        fData8[kTagOffset] = SkTo<uint8_t>(t);
     }
-};
 
-} // namespace
-
-
-// Boring public Value glue.
-
-const Value& Value::Null() {
-    static const Value g_null = ValueRec::MakeTypeBound(ValueRec::kNull);
-    return g_null;
+    SkASSERT(this->getTag()    == t);
+    SkASSERT(this->ptr<void>() == p);
 }
 
-const Member& Member::Null() {
-    static const Member g_null = { Value::Null().as<StringValue>(), Value::Null() };
-    return g_null;
+NullValue::NullValue() {
+    this->init_tagged(Tag::kNull);
+    SkASSERT(this->getTag() == Tag::kNull);
 }
 
-Value::Type Value::getType() const {
-    static constexpr Value::Type kTypeMap[] = {
-        Value::Type::kString, // kShortString
-        Value::Type::kNull,   // kNull
-        Value::Type::kBool,   // kBool
-        Value::Type::kNumber, // kInt
-        Value::Type::kNumber, // kFloat
-        Value::Type::kString, // kString
-        Value::Type::kArray,  // kArray
-        Value::Type::kObject, // kObject
-    };
-
-    const auto& rec = *reinterpret_cast<const ValueRec*>(this);
-    const auto type_index = static_cast<size_t>(rec.getRecType() >> ValueRec::kTypeShift);
-    SkASSERT(type_index < SK_ARRAY_COUNT(kTypeMap));
-
-    return kTypeMap[type_index];
+BoolValue::BoolValue(bool b) {
+    this->init_tagged(Tag::kBool);
+    *this->cast<bool>() = b;
+    SkASSERT(this->getTag() == Tag::kBool);
 }
 
-template <>
-bool PrimitiveValue<bool, Value::Type::kBool>::operator*() const {
-    const auto& rec = *reinterpret_cast<const ValueRec*>(this);
-
-    if (rec.is<NullValue>()) return false;
-
-    SkASSERT(rec.getRecType() == ValueRec::kBool);
-
-    return *rec.cast<bool>();
+NumberValue::NumberValue(int32_t i) {
+    this->init_tagged(Tag::kInt);
+    *this->cast<int32_t>() = i;
+    SkASSERT(this->getTag() == Tag::kInt);
 }
 
-template <>
-double PrimitiveValue<double, Value::Type::kNumber>::operator*() const {
-    const auto& rec = *reinterpret_cast<const ValueRec*>(this);
-
-    if (rec.is<NullValue>()) return 0;
-
-    SkASSERT(rec.getRecType() == ValueRec::kInt ||
-             rec.getRecType() == ValueRec::kFloat);
-
-    return rec.getRecType() == ValueRec::kInt
-        ? static_cast<double>(*rec.cast<int32_t>())
-        : static_cast<double>(*rec.cast<float>());
+NumberValue::NumberValue(float f) {
+    this->init_tagged(Tag::kFloat);
+    *this->cast<float>() = f;
+    SkASSERT(this->getTag() == Tag::kFloat);
 }
 
-template <>
-size_t VectorValue<Value, Value::Type::kArray>::size() const {
-    return reinterpret_cast<const ValueRec*>(this)->vectorSize(ValueRec::kArray);
+// Vector recs point to externally allocated slabs with the following layout:
+//
+//   [size_t n] [REC_0] ... [REC_n-1] [optional extra trailing storage]
+//
+// Long strings use extra_alloc_size == 1 to store the \0 terminator.
+//
+template <typename T, size_t extra_alloc_size = 0>
+static void* MakeVector(const void* src, size_t size, SkArenaAlloc& alloc) {
+    // 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));
+    auto* data_ptr = reinterpret_cast<void*>(size_ptr + 1);
+    *size_ptr = size;
+    memcpy(data_ptr, src, size * sizeof(T));
+
+    return size_ptr;
 }
 
-template <>
-const Value* VectorValue<Value, Value::Type::kArray>::begin() const {
-    return reinterpret_cast<const ValueRec*>(this)->vectorBegin<Value>(ValueRec::kArray);
+ArrayValue::ArrayValue(const Value* src, size_t size, SkArenaAlloc& alloc) {
+    this->init_tagged_pointer(Tag::kArray, MakeVector<Value>(src, size, alloc));
+    SkASSERT(this->getTag() == Tag::kArray);
 }
 
-template <>
-const Value* VectorValue<Value, Value::Type::kArray>::end() const {
-    return reinterpret_cast<const ValueRec*>(this)->vectorEnd<Value>(ValueRec::kArray);
-}
+// Strings have two flavors:
+//
+// -- short strings (len <= 7) -> these are stored inline, in the record
+//    (one byte reserved for null terminator/type):
+//
+//        [str] [\0]|[max_len - actual_len]
+//
+//    Storing [max_len - actual_len] allows the 'len' field to double-up as a
+//    null terminator when size == max_len (this works 'cause kShortString == 0).
+//
+// -- long strings (len > 7) -> these are externally allocated vectors (VectorRec<char>).
+//
+// The string data plus a null-char terminator are copied over.
+//
+StringValue::StringValue(const char* src, size_t size, SkArenaAlloc& alloc) {
+    if (size > kMaxInlineStringSize) {
+        this->init_tagged_pointer(Tag::kString, MakeVector<char, 1>(src, size, alloc));
 
-template <>
-size_t VectorValue<Member, Value::Type::kObject>::size() const {
-    return reinterpret_cast<const ValueRec*>(this)->vectorSize(ValueRec::kObject);
-}
+        auto* data = this->cast<VectorValue<char, Value::Type::kString>>()->begin();
+        const_cast<char*>(data)[size] = '\0';
+        SkASSERT(this->getTag() == Tag::kString);
+        return;
+    }
 
-template <>
-const Member* VectorValue<Member, Value::Type::kObject>::begin() const {
-    return reinterpret_cast<const ValueRec*>(this)->vectorBegin<Member>(ValueRec::kObject);
-}
+    this->init_tagged(Tag::kShortString);
 
-template <>
-const Member* VectorValue<Member, Value::Type::kObject>::end() const {
-    return reinterpret_cast<const ValueRec*>(this)->vectorEnd<Member>(ValueRec::kObject);
-}
+    auto* payload = this->cast<char>();
+    memcpy(payload, src, size);
+    payload[size] = '\0';
 
-template <>
-size_t VectorValue<char, Value::Type::kString>::size() const {
-    return reinterpret_cast<const ValueRec*>(this)->stringSize();
-}
+    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
+    static_assert(static_cast<uint8_t>(Tag::kShortString) == 0, "please don't break this");
+    payload[kMaxInlineStringSize] = len_tag;
 
-template <>
-const char* VectorValue<char, Value::Type::kString>::begin() const {
-    return reinterpret_cast<const ValueRec*>(this)->stringBegin();
+    SkASSERT(this->getTag() == Tag::kShortString);
 }
 
-template <>
-const char* VectorValue<char, Value::Type::kString>::end() const {
-    return reinterpret_cast<const ValueRec*>(this)->stringEnd();
+ObjectValue::ObjectValue(const Member* src, size_t size, SkArenaAlloc& alloc) {
+    this->init_tagged_pointer(Tag::kObject, MakeVector<Member>(src, size, alloc));
+    SkASSERT(this->getTag() == Tag::kObject);
 }
 
+
+// Boring public Value glue.
+
 const Value& ObjectValue::operator[](const char* key) const {
     // Reverse search for duplicates resolution (policy: return last).
     const auto* begin  = this->begin();
@@ -381,7 +154,8 @@ const Value& ObjectValue::operator[](const char* key) const {
         }
     }
 
-    return Value::Null();
+    static const Value g_null = NullValue();
+    return g_null;
 }
 
 namespace {
@@ -457,7 +231,7 @@ public:
         fScopeStack.reserve(kScopeStackReserve);
     }
 
-    const Value& parse(const char* p) {
+    const Value parse(const char* p) {
         p = skip_ws(p);
 
         switch (*p) {
@@ -466,7 +240,7 @@ public:
         case '[':
             goto match_array;
         default:
-            return this->error(Value::Null(), p, "invalid top-level value");
+            return this->error(NullValue(), p, "invalid top-level value");
         }
 
     match_object:
@@ -480,15 +254,15 @@ public:
         // goto match_object_key;
     match_object_key:
         p = skip_ws(p);
-        if (*p != '"') return this->error(Value::Null(), p, "expected object key");
+        if (*p != '"') return this->error(NullValue(), p, "expected object key");
 
         p = this->matchString(p, [this](const char* key, size_t size) {
             this->pushObjectKey(key, size);
         });
-        if (!p) return Value::Null();
+        if (!p) return NullValue();
 
         p = skip_ws(p);
-        if (*p != ':') return this->error(Value::Null(), p, "expected ':' separator");
+        if (*p != ':') return this->error(NullValue(), p, "expected ':' separator");
 
         ++p;
 
@@ -498,7 +272,7 @@ public:
 
         switch (*p) {
         case '\0':
-            return this->error(Value::Null(), p, "unexpected input end");
+            return this->error(NullValue(), p, "unexpected input end");
         case '"':
             p = this->matchString(p, [this](const char* str, size_t size) {
                 this->pushString(str, size);
@@ -522,7 +296,7 @@ public:
             break;
         }
 
-        if (!p) return Value::Null();
+        if (!p) return NullValue();
 
         // goto match_post_value;
     match_post_value:
@@ -542,7 +316,7 @@ public:
         case '}':
             goto pop_object;
         default:
-            return this->error(Value::Null(), p - 1, "unexpected value-trailing token");
+            return this->error(NullValue(), p - 1, "unexpected value-trailing token");
         }
 
         // unreachable
@@ -552,7 +326,7 @@ public:
         SkASSERT(*p == '}');
 
         if (fScopeStack.back() < 0) {
-            return this->error(Value::Null(), p, "unexpected object terminator");
+            return this->error(NullValue(), p, "unexpected object terminator");
         }
 
         this->popObjectScope();
@@ -565,13 +339,11 @@ public:
 
         if (fScopeStack.empty()) {
             SkASSERT(fValueStack.size() == 1);
-            auto* root = fAlloc.make<Value>();
-            *root = fValueStack.front();
 
             // Stop condition: parsed the top level element and there is no trailing garbage.
             return *skip_ws(p) == '\0'
-                ? *root
-                : this->error(Value::Null(), p, "trailing root garbage");
+                ? fValueStack.front()
+                : this->error(NullValue(), p, "trailing root garbage");
         }
 
         goto match_post_value;
@@ -589,7 +361,7 @@ public:
         SkASSERT(*p == ']');
 
         if (fScopeStack.back() >= 0) {
-            return this->error(Value::Null(), p, "unexpected array terminator");
+            return this->error(NullValue(), p, "unexpected array terminator");
         }
 
         this->popArrayScope();
@@ -597,7 +369,7 @@ public:
         goto pop_common;
 
         SkASSERT(false);
-        return Value::Null();
+        return NullValue();
     }
 
     const SkString& getError() const {
@@ -614,11 +386,12 @@ private:
 
     SkString             fError;
 
-    template <typename T>
-    void popScopeAsVec(ValueRec::RecType type, size_t scope_start) {
+    template <typename VectorT>
+    void popScopeAsVec(size_t scope_start) {
         SkASSERT(scope_start > 0);
         SkASSERT(scope_start <= fValueStack.size());
 
+        using T = typename VectorT::ValueT;
         static_assert( sizeof(T) >=  sizeof(Value), "");
         static_assert( sizeof(T)  %  sizeof(Value) == 0, "");
         static_assert(alignof(T) == alignof(Value), "");
@@ -630,7 +403,7 @@ private:
         const auto* begin = reinterpret_cast<const T*>(fValueStack.data() + scope_start);
 
         // Instantiate the placeholder value added in onPush{Object/Array}.
-        fValueStack[scope_start - 1] = ValueRec::MakeVector<T>(type, begin, count, fAlloc);
+        fValueStack[scope_start - 1] = VectorT(begin, count, fAlloc);
 
         // Drop the current scope.
         fScopeStack.pop_back();
@@ -648,7 +421,7 @@ private:
     void popObjectScope() {
         const auto scope_start = fScopeStack.back();
         SkASSERT(scope_start > 0);
-        this->popScopeAsVec<Member>(ValueRec::kObject, SkTo<size_t>(scope_start));
+        this->popScopeAsVec<ObjectValue>(SkTo<size_t>(scope_start));
 
         SkDEBUGCODE(
             const auto& obj = fValueStack.back().as<ObjectValue>();
@@ -670,7 +443,7 @@ private:
     void popArrayScope() {
         const auto scope_start = -fScopeStack.back();
         SkASSERT(scope_start > 0);
-        this->popScopeAsVec<Value>(ValueRec::kArray, SkTo<size_t>(scope_start));
+        this->popScopeAsVec<ArrayValue>(SkTo<size_t>(scope_start));
 
         SkDEBUGCODE(
             const auto& arr = fValueStack.back().as<ArrayValue>();
@@ -686,31 +459,31 @@ private:
     }
 
     void pushTrue() {
-        fValueStack.push_back(ValueRec::MakePrimitive<bool>(ValueRec::kBool, true));
+        fValueStack.push_back(BoolValue(true));
     }
 
     void pushFalse() {
-        fValueStack.push_back(ValueRec::MakePrimitive<bool>(ValueRec::kBool, false));
+        fValueStack.push_back(BoolValue(false));
     }
 
     void pushNull() {
-        fValueStack.push_back(ValueRec::MakeTypeBound(ValueRec::kNull));
+        fValueStack.push_back(NullValue());
     }
 
     void pushString(const char* s, size_t size) {
-        fValueStack.push_back(ValueRec::MakeString(s, size, fAlloc));
+        fValueStack.push_back(StringValue(s, size, fAlloc));
     }
 
     void pushInt32(int32_t i) {
-        fValueStack.push_back(ValueRec::MakePrimitive<int32_t>(ValueRec::kInt, i));
+        fValueStack.push_back(NumberValue(i));
     }
 
     void pushFloat(float f) {
-        fValueStack.push_back(ValueRec::MakePrimitive<float>(ValueRec::kFloat, f));
+        fValueStack.push_back(NumberValue(f));
     }
 
     template <typename T>
-    const T& error(const T& ret_val, const char* p, const char* msg) {
+    T error(T&& ret_val, const char* p, const char* msg) {
 #if defined(SK_JSON_REPORT_ERRORS)
         static constexpr size_t kMaxContext = 128;
         fError = SkStringPrintf("%s: >", msg);
@@ -930,17 +703,25 @@ void Write(const Value& v, SkWStream* stream) {
 
 } // namespace
 
+SkString Value::toString() const {
+    SkDynamicMemoryWStream wstream;
+    Write(*this, &wstream);
+    const auto data = wstream.detachAsData();
+    // TODO: is there a better way to pass data around without copying?
+    return SkString(static_cast<const char*>(data->data()), data->size());
+}
+
 static constexpr size_t kMinChunkSize = 4096;
 
-DOM::DOM(const char* cstr)
+DOM::DOM(const char* c_str)
     : fAlloc(kMinChunkSize) {
     DOMParser parser(fAlloc);
 
-    fRoot = &parser.parse(cstr);
+    fRoot = parser.parse(c_str);
 }
 
 void DOM::write(SkWStream* stream) const {
-    Write(*fRoot, stream);
+    Write(fRoot, stream);
 }
 
 } // namespace skjson
diff --git a/modules/skjson/src/SkJSONTest.cpp b/modules/skjson/src/SkJSONTest.cpp
index 8b876ddf4e..6d4338cbbb 100644
--- a/modules/skjson/src/SkJSONTest.cpp
+++ b/modules/skjson/src/SkJSONTest.cpp
@@ -7,7 +7,9 @@
 
 #include "Test.h"
 
+#include "SkArenaAlloc.h"
 #include "SkJSON.h"
+#include "SkString.h"
 #include "SkStream.h"
 
 using namespace skjson;
@@ -107,18 +109,29 @@ static void check_primitive(skiatest::Reporter* reporter, const Value& v, T pv,
                             bool is_type) {
 
     REPORTER_ASSERT(reporter,  v.is<VT>() == is_type);
-    REPORTER_ASSERT(reporter, *v.as<VT>() == pv);
+    const VT* cast_t = v;
+    REPORTER_ASSERT(reporter, (cast_t != nullptr) == is_type);
+
+    if (is_type) {
+        REPORTER_ASSERT(reporter, &v.as<VT>() == cast_t);
+        REPORTER_ASSERT(reporter, *v.as<VT>() == pv);
+    }
 }
 
 template <typename T>
 static void check_vector(skiatest::Reporter* reporter, const Value& v, size_t expected_size,
                          bool is_vector) {
     REPORTER_ASSERT(reporter, v.is<T>() == is_vector);
-
-    const auto& vec = v.as<T>();
-    REPORTER_ASSERT(reporter, vec.size() == expected_size);
-    REPORTER_ASSERT(reporter, (vec.begin() != nullptr) == is_vector);
-    REPORTER_ASSERT(reporter, vec.end()   == vec.begin() + expected_size);
+    const T* cast_t = v;
+    REPORTER_ASSERT(reporter, (cast_t != nullptr) == is_vector);
+
+    if (is_vector) {
+        const auto& vec = v.as<T>();
+        REPORTER_ASSERT(reporter, &vec == cast_t);
+        REPORTER_ASSERT(reporter, vec.size()  == expected_size);
+        REPORTER_ASSERT(reporter, vec.begin() != nullptr);
+        REPORTER_ASSERT(reporter, vec.end()   == vec.begin() + expected_size);
+    }
 }
 
 static void check_string(skiatest::Reporter* reporter, const Value& v, const char* s) {
@@ -128,7 +141,7 @@ static void check_string(skiatest::Reporter* reporter, const Value& v, const cha
     }
 }
 
-DEF_TEST(SkJSON_DOM, reporter) {
+DEF_TEST(SkJSON_DOM_visit, reporter) {
     static constexpr char json[] = "{ \n\
         \"k1\": null,                \n\
         \"k2\": false,               \n\
@@ -231,7 +244,6 @@ DEF_TEST(SkJSON_DOM, reporter) {
         check_primitive<float, NumberValue>(reporter, v.as<ArrayValue>()[0], 1, true);
         check_primitive<bool, BoolValue>(reporter, v.as<ArrayValue>()[1], true, true);
         check_vector<StringValue>(reporter, v.as<ArrayValue>()[2], 3, true);
-        REPORTER_ASSERT(reporter, v.as<ArrayValue>()[3].is<NullValue>());
     }
 
     {
@@ -263,10 +275,100 @@ DEF_TEST(SkJSON_DOM, reporter) {
         check_string(reporter, v.as<ObjectValue>()["kk1"], "baz");
         check_primitive<bool, BoolValue>(reporter, v.as<ObjectValue>()["kk2"], false, true);
     }
+}
 
-    {
-        const auto& v =
-            jroot["foo"].as<ObjectValue>()["bar"].as<ObjectValue>()["baz"];
-        REPORTER_ASSERT(reporter, v.is<NullValue>());
-    }
+template <typename T>
+void check_value(skiatest::Reporter* reporter, const Value& v, const char* expected_string) {
+    REPORTER_ASSERT(reporter, v.is<T>());
+
+    const T* cast_t = v;
+    REPORTER_ASSERT(reporter, cast_t == &v.as<T>());
+
+    const auto vstr = v.toString();
+    REPORTER_ASSERT(reporter, 0 == strcmp(expected_string, vstr.c_str()));
+}
+
+DEF_TEST(SkJSON_DOM_build, reporter) {
+    SkArenaAlloc alloc(4096);
+
+    const auto v0  = NullValue();
+    check_value<NullValue>(reporter, v0, "null");
+
+    const auto v1  = BoolValue(true);
+    check_value<BoolValue>(reporter, v1, "true");
+
+    const auto v2  = BoolValue(false);
+    check_value<BoolValue>(reporter, v2, "false");
+
+    const auto v3  = NumberValue(0);
+    check_value<NumberValue>(reporter, v3, "0");
+
+    const auto v4  = NumberValue(42);
+    check_value<NumberValue>(reporter, v4, "42");
+
+    const auto v5  = NumberValue(42.75f);
+    check_value<NumberValue>(reporter, v5, "42.75");
+
+    const auto v6  = StringValue(nullptr, 0, alloc);
+    check_value<StringValue>(reporter, v6, "\"\"");
+
+    const auto v7  = StringValue(" foo ", 5, alloc);
+    check_value<StringValue>(reporter, v7, "\" foo \"");
+
+    const auto v8  = StringValue(" foo bar baz ", 13, alloc);
+    check_value<StringValue>(reporter, v8, "\" foo bar baz \"");
+
+    const auto v9  = ArrayValue(nullptr, 0, alloc);
+    check_value<ArrayValue>(reporter, v9, "[]");
+
+    const Value values0[] = { v0, v3, v9 };
+    const auto v10 = ArrayValue(values0, SK_ARRAY_COUNT(values0), alloc);
+    check_value<ArrayValue>(reporter, v10, "[null,0,[]]");
+
+    const auto v11 = ObjectValue(nullptr, 0, alloc);
+    check_value<ObjectValue>(reporter, v11, "{}");
+
+    const Member members0[] = {
+        { StringValue("key_0", 5, alloc), v1  },
+        { StringValue("key_1", 5, alloc), v4  },
+        { StringValue("key_2", 5, alloc), v11 },
+    };
+    const auto v12 = ObjectValue(members0, SK_ARRAY_COUNT(members0), alloc);
+    check_value<ObjectValue>(reporter, v12, "{"
+                                                "\"key_0\":true,"
+                                                "\"key_1\":42,"
+                                                "\"key_2\":{}"
+                                            "}");
+
+    const Value values1[] = { v2, v6, v12 };
+    const auto v13 = ArrayValue(values1, SK_ARRAY_COUNT(values1), alloc);
+    check_value<ArrayValue>(reporter, v13, "["
+                                               "false,"
+                                               "\"\","
+                                               "{"
+                                                   "\"key_0\":true,"
+                                                   "\"key_1\":42,"
+                                                   "\"key_2\":{}"
+                                               "}"
+                                           "]");
+
+    const Member members1[] = {
+        { StringValue("key_00", 6, alloc), v5  },
+        { StringValue("key_01", 6, alloc), v7  },
+        { StringValue("key_02", 6, alloc), v13 },
+    };
+    const auto v14 = ObjectValue(members1, SK_ARRAY_COUNT(members1), alloc);
+    check_value<ObjectValue>(reporter, v14, "{"
+                                                "\"key_00\":42.75,"
+                                                "\"key_01\":\" foo \","
+                                                "\"key_02\":["
+                                                                "false,"
+                                                                "\"\","
+                                                                "{"
+                                                                    "\"key_0\":true,"
+                                                                    "\"key_1\":42,"
+                                                                    "\"key_2\":{}"
+                                                                "}"
+                                                            "]"
+                                            "}");
 }