Revert "[skjson] Implementation/API tweaks"

This reverts commit 03b68421cafa58428edd8ab12e0d2235c7000c3f.

Reason for revert: Broke Debian9 builds

Original change's description:
> [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.
> 
> Change-Id: I24a7c75db3aa8b12c740c77ac7df4af4e3a1dff8
> Reviewed-on: https://skia-review.googlesource.com/134610
> Commit-Queue: Florin Malita <fmalita@chromium.org>
> Reviewed-by: Mike Klein <mtklein@google.com>

TBR=mtklein@google.com,fmalita@chromium.org

Change-Id: I2c681ef5c8d5fc15508e58b4b0f6ab9491b7d76f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/134880
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@chromium.org>

3 files changed, 392 insertions, 495 deletions

diff --git a/modules/skjson/include/SkJSON.h b/modules/skjson/include/SkJSON.h
index 8e4231e7a7..4cb312c64f 100644
--- a/modules/skjson/include/SkJSON.h
+++ b/modules/skjson/include/SkJSON.h
@@ -11,7 +11,6 @@
 #include "SkArenaAlloc.h"
 #include "SkTypes.h"
 
-class SkString;
 class SkWStream;
 
 namespace skjson {
@@ -28,26 +27,12 @@ namespace skjson {
  *
  *  Values are opaque, fixed-size (64 bits), immutable records.
  *
- *  They can be converted to facade types for type-specific functionality.
+ *  They can be freely converted to any of the facade types for type-specific functionality.
  *
- *  E.g.:
+ *  Note: type checking is lazy/deferred, to facilitate chained property access - e.g.
  *
- *     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");
- *         }
- *     }
+ *      if (!v.as<ObjectValue>()["foo"].as<ObjectValue>()["bar"].is<NullValue>())
+ *          LOG("found v.foo.bar!");
  */
 class alignas(8) Value {
 public:
@@ -61,7 +46,7 @@ public:
     };
 
     /**
-     * @return    The type of this value.
+     * @return    The public type of this record.
      */
     Type getType() const;
 
@@ -69,289 +54,85 @@ public:
      * @return    True if the record matches the facade type T.
      */
     template <typename T>
-    bool is() const { return this->getType() == T::kType; }
+    bool is() const { return T::IsType(this->getType()); }
 
     /**
-     * Unguarded conversion to facade types.
+     * @return    The record cast as facade type T.
      *
-     * @return    The record cast as facade type T&.
+     * Note: this is always safe, as proper typing is enforced in the facade methods.
      */
     template <typename T>
     const T& as() const {
-        SkASSERT(this->is<T>());
-        return *reinterpret_cast<const T*>(this);
+        return *reinterpret_cast<const T*>(this->is<T>() ? this : &Value::Null());
     }
 
     /**
-     * Guarded conversion to facade types.
-     *
-     * @return    The record cast as facade type T*.
+     * @return    Null value singleton.
      */
-    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;
+    static const Value& Null();
 
 protected:
-    /*
-      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
+    uint8_t   fData8[8];
 };
 
 class NullValue final : public Value {
 public:
-    static constexpr Type kType = Type::kNull;
-
-    NullValue();
+    static bool IsType(Value::Type t) { return t == Type::kNull; }
 };
 
-class BoolValue final : public Value {
-public:
-    static constexpr Type kType = Type::kBool;
-
-    explicit BoolValue(bool);
-
-    bool operator *() const {
-        SkASSERT(this->getTag() == Tag::kBool);
-        return *this->cast<bool>();
-    }
-};
-
-class NumberValue final : public Value {
+template <typename T, Value::Type vtype>
+class PrimitiveValue final : public Value {
 public:
-    static constexpr Type kType = Type::kNumber;
+    static bool IsType(Value::Type t) { return t == vtype; }
 
-    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>());
-    }
+    T operator *() const;
 };
 
 template <typename T, Value::Type vtype>
 class VectorValue : public Value {
 public:
-    using ValueT = T;
-    static constexpr Type kType = vtype;
+    static bool IsType(Value::Type t) { return t == vtype; }
 
-    size_t size() const {
-        SkASSERT(this->getType() == kType);
-        return *this->ptr<size_t>();
-    }
+    size_t size() 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* begin() const;
+    const T*   end() const;
 
     const T& operator[](size_t i) const {
-        SkASSERT(this->getType() == kType);
-        SkASSERT(i < this->size());
-
-        return *(this->begin() + i);
+        return (i < this->size()) ? *(this->begin() + i) : T::Null();
     }
 };
 
-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;
-};
+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 >;
 
 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;
-    Value        fRoot;
+    const 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 05bf04d8f0..8af85ba695 100644
--- a/modules/skjson/src/SkJSON.cpp
+++ b/modules/skjson/src/SkJSON.cpp
@@ -18,130 +18,356 @@ 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);
 
-void Value::init_tagged(Tag t) {
-    memset(fData8, 0, sizeof(fData8));
-    fData8[Value::kTagOffset] = SkTo<uint8_t>(t);
-    SkASSERT(this->getTag() == t);
-}
+// 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
+    };
 
-// 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);
+    RecType getRecType() const {
+        return static_cast<RecType>(*this->cast<uint64_t>() & kTypeMask);
+    }
 
-    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);
+    // 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);
     }
 
-    SkASSERT(this->getTag()    == t);
-    SkASSERT(this->ptr<void>() == p);
-}
+    template <typename T>
+    T* cast() { return const_cast<T*>(const_cast<const ValueRec*>(this)->cast<T>()); }
 
-NullValue::NullValue() {
-    this->init_tagged(Tag::kNull);
-    SkASSERT(this->getTag() == Tag::kNull);
-}
+    // 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;
+    }
 
-BoolValue::BoolValue(bool b) {
-    this->init_tagged(Tag::kBool);
-    *this->cast<bool>() = b;
-    SkASSERT(this->getTag() == Tag::kBool);
+    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);
+    }
+
+    const char* stringEnd() const {
+        if (this->getRecType() == ValueRec::kShortString) {
+            const auto* payload = this->cast<char>();
+            return payload + kMaxInlineStringSize - SkToSizeT(payload[kMaxInlineStringSize]);
+        }
+
+        return this->vectorEnd<char>(ValueRec::kString);
+    }
+};
+
+} // namespace
+
+
+// Boring public Value glue.
+
+const Value& Value::Null() {
+    static const Value g_null = ValueRec::MakeTypeBound(ValueRec::kNull);
+    return g_null;
 }
 
-NumberValue::NumberValue(int32_t i) {
-    this->init_tagged(Tag::kInt);
-    *this->cast<int32_t>() = i;
-    SkASSERT(this->getTag() == Tag::kInt);
+const Member& Member::Null() {
+    static const Member g_null = { Value::Null().as<StringValue>(), Value::Null() };
+    return g_null;
 }
 
-NumberValue::NumberValue(float f) {
-    this->init_tagged(Tag::kFloat);
-    *this->cast<float>() = f;
-    SkASSERT(this->getTag() == Tag::kFloat);
+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];
 }
 
-// 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 <>
+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>();
 }
 
-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 <>
+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>());
 }
 
-// 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<Value, Value::Type::kArray>::size() const {
+    return reinterpret_cast<const ValueRec*>(this)->vectorSize(ValueRec::kArray);
+}
 
-        auto* data = this->cast<VectorValue<char, Value::Type::kString>>()->begin();
-        const_cast<char*>(data)[size] = '\0';
-        SkASSERT(this->getTag() == Tag::kString);
-        return;
-    }
+template <>
+const Value* VectorValue<Value, Value::Type::kArray>::begin() const {
+    return reinterpret_cast<const ValueRec*>(this)->vectorBegin<Value>(ValueRec::kArray);
+}
 
-    this->init_tagged(Tag::kShortString);
+template <>
+const Value* VectorValue<Value, Value::Type::kArray>::end() const {
+    return reinterpret_cast<const ValueRec*>(this)->vectorEnd<Value>(ValueRec::kArray);
+}
 
-    auto* payload = this->cast<char>();
-    memcpy(payload, src, size);
-    payload[size] = '\0';
+template <>
+size_t VectorValue<Member, Value::Type::kObject>::size() const {
+    return reinterpret_cast<const ValueRec*>(this)->vectorSize(ValueRec::kObject);
+}
 
-    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 Member* VectorValue<Member, Value::Type::kObject>::begin() const {
+    return reinterpret_cast<const ValueRec*>(this)->vectorBegin<Member>(ValueRec::kObject);
+}
 
-    SkASSERT(this->getTag() == Tag::kShortString);
+template <>
+const Member* VectorValue<Member, Value::Type::kObject>::end() const {
+    return reinterpret_cast<const ValueRec*>(this)->vectorEnd<Member>(ValueRec::kObject);
 }
 
-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);
+template <>
+size_t VectorValue<char, Value::Type::kString>::size() const {
+    return reinterpret_cast<const ValueRec*>(this)->stringSize();
 }
 
+template <>
+const char* VectorValue<char, Value::Type::kString>::begin() const {
+    return reinterpret_cast<const ValueRec*>(this)->stringBegin();
+}
 
-// Boring public Value glue.
+template <>
+const char* VectorValue<char, Value::Type::kString>::end() const {
+    return reinterpret_cast<const ValueRec*>(this)->stringEnd();
+}
 
 const Value& ObjectValue::operator[](const char* key) const {
     // Reverse search for duplicates resolution (policy: return last).
@@ -155,8 +381,7 @@ const Value& ObjectValue::operator[](const char* key) const {
         }
     }
 
-    static const Value g_null = NullValue();
-    return g_null;
+    return Value::Null();
 }
 
 namespace {
@@ -232,7 +457,7 @@ public:
         fScopeStack.reserve(kScopeStackReserve);
     }
 
-    const Value parse(const char* p) {
+    const Value& parse(const char* p) {
         p = skip_ws(p);
 
         switch (*p) {
@@ -241,7 +466,7 @@ public:
         case '[':
             goto match_array;
         default:
-            return this->error(NullValue(), p, "invalid top-level value");
+            return this->error(Value::Null(), p, "invalid top-level value");
         }
 
     match_object:
@@ -255,15 +480,15 @@ public:
         // goto match_object_key;
     match_object_key:
         p = skip_ws(p);
-        if (*p != '"') return this->error(NullValue(), p, "expected object key");
+        if (*p != '"') return this->error(Value::Null(), p, "expected object key");
 
         p = this->matchString(p, [this](const char* key, size_t size) {
             this->pushObjectKey(key, size);
         });
-        if (!p) return NullValue();
+        if (!p) return Value::Null();
 
         p = skip_ws(p);
-        if (*p != ':') return this->error(NullValue(), p, "expected ':' separator");
+        if (*p != ':') return this->error(Value::Null(), p, "expected ':' separator");
 
         ++p;
 
@@ -273,7 +498,7 @@ public:
 
         switch (*p) {
         case '\0':
-            return this->error(NullValue(), p, "unexpected input end");
+            return this->error(Value::Null(), p, "unexpected input end");
         case '"':
             p = this->matchString(p, [this](const char* str, size_t size) {
                 this->pushString(str, size);
@@ -297,7 +522,7 @@ public:
             break;
         }
 
-        if (!p) return NullValue();
+        if (!p) return Value::Null();
 
         // goto match_post_value;
     match_post_value:
@@ -317,7 +542,7 @@ public:
         case '}':
             goto pop_object;
         default:
-            return this->error(NullValue(), p - 1, "unexpected value-trailing token");
+            return this->error(Value::Null(), p - 1, "unexpected value-trailing token");
         }
 
         // unreachable
@@ -327,7 +552,7 @@ public:
         SkASSERT(*p == '}');
 
         if (fScopeStack.back() < 0) {
-            return this->error(NullValue(), p, "unexpected object terminator");
+            return this->error(Value::Null(), p, "unexpected object terminator");
         }
 
         this->popObjectScope();
@@ -340,11 +565,13 @@ 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'
-                ? fValueStack.front()
-                : this->error(NullValue(), p, "trailing root garbage");
+                ? *root
+                : this->error(Value::Null(), p, "trailing root garbage");
         }
 
         goto match_post_value;
@@ -362,7 +589,7 @@ public:
         SkASSERT(*p == ']');
 
         if (fScopeStack.back() >= 0) {
-            return this->error(NullValue(), p, "unexpected array terminator");
+            return this->error(Value::Null(), p, "unexpected array terminator");
         }
 
         this->popArrayScope();
@@ -370,7 +597,7 @@ public:
         goto pop_common;
 
         SkASSERT(false);
-        return NullValue();
+        return Value::Null();
     }
 
     const SkString& getError() const {
@@ -387,12 +614,11 @@ private:
 
     SkString             fError;
 
-    template <typename VectorT>
-    void popScopeAsVec(size_t scope_start) {
+    template <typename T>
+    void popScopeAsVec(ValueRec::RecType type, 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), "");
@@ -404,7 +630,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] = VectorT(begin, count, fAlloc);
+        fValueStack[scope_start - 1] = ValueRec::MakeVector<T>(type, begin, count, fAlloc);
 
         // Drop the current scope.
         fScopeStack.pop_back();
@@ -422,7 +648,7 @@ private:
     void popObjectScope() {
         const auto scope_start = fScopeStack.back();
         SkASSERT(scope_start > 0);
-        this->popScopeAsVec<ObjectValue>(SkTo<size_t>(scope_start));
+        this->popScopeAsVec<Member>(ValueRec::kObject, SkTo<size_t>(scope_start));
 
         SkDEBUGCODE(
             const auto& obj = fValueStack.back().as<ObjectValue>();
@@ -444,7 +670,7 @@ private:
     void popArrayScope() {
         const auto scope_start = -fScopeStack.back();
         SkASSERT(scope_start > 0);
-        this->popScopeAsVec<ArrayValue>(SkTo<size_t>(scope_start));
+        this->popScopeAsVec<Value>(ValueRec::kArray, SkTo<size_t>(scope_start));
 
         SkDEBUGCODE(
             const auto& arr = fValueStack.back().as<ArrayValue>();
@@ -460,31 +686,31 @@ private:
     }
 
     void pushTrue() {
-        fValueStack.push_back(BoolValue(true));
+        fValueStack.push_back(ValueRec::MakePrimitive<bool>(ValueRec::kBool, true));
     }
 
     void pushFalse() {
-        fValueStack.push_back(BoolValue(false));
+        fValueStack.push_back(ValueRec::MakePrimitive<bool>(ValueRec::kBool, false));
     }
 
     void pushNull() {
-        fValueStack.push_back(NullValue());
+        fValueStack.push_back(ValueRec::MakeTypeBound(ValueRec::kNull));
     }
 
     void pushString(const char* s, size_t size) {
-        fValueStack.push_back(StringValue(s, size, fAlloc));
+        fValueStack.push_back(ValueRec::MakeString(s, size, fAlloc));
     }
 
     void pushInt32(int32_t i) {
-        fValueStack.push_back(NumberValue(i));
+        fValueStack.push_back(ValueRec::MakePrimitive<int32_t>(ValueRec::kInt, i));
     }
 
     void pushFloat(float f) {
-        fValueStack.push_back(NumberValue(f));
+        fValueStack.push_back(ValueRec::MakePrimitive<float>(ValueRec::kFloat, f));
     }
 
     template <typename T>
-    T error(T&& ret_val, const char* p, const char* msg) {
+    const T& error(const 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);
@@ -704,25 +930,17 @@ 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* c_str)
+DOM::DOM(const char* cstr)
     : fAlloc(kMinChunkSize) {
     DOMParser parser(fAlloc);
 
-    fRoot = parser.parse(c_str);
+    fRoot = &parser.parse(cstr);
 }
 
 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 6d4338cbbb..8b876ddf4e 100644
--- a/modules/skjson/src/SkJSONTest.cpp
+++ b/modules/skjson/src/SkJSONTest.cpp
@@ -7,9 +7,7 @@
 
 #include "Test.h"
 
-#include "SkArenaAlloc.h"
 #include "SkJSON.h"
-#include "SkString.h"
 #include "SkStream.h"
 
 using namespace skjson;
@@ -109,29 +107,18 @@ static void check_primitive(skiatest::Reporter* reporter, const Value& v, T pv,
                             bool is_type) {
 
     REPORTER_ASSERT(reporter,  v.is<VT>() == is_type);
-    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);
-    }
+    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 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);
-    }
+
+    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);
 }
 
 static void check_string(skiatest::Reporter* reporter, const Value& v, const char* s) {
@@ -141,7 +128,7 @@ static void check_string(skiatest::Reporter* reporter, const Value& v, const cha
     }
 }
 
-DEF_TEST(SkJSON_DOM_visit, reporter) {
+DEF_TEST(SkJSON_DOM, reporter) {
     static constexpr char json[] = "{ \n\
         \"k1\": null,                \n\
         \"k2\": false,               \n\
@@ -244,6 +231,7 @@ DEF_TEST(SkJSON_DOM_visit, 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>());
     }
 
     {
@@ -275,100 +263,10 @@ DEF_TEST(SkJSON_DOM_visit, reporter) {
         check_string(reporter, v.as<ObjectValue>()["kk1"], "baz");
         check_primitive<bool, BoolValue>(reporter, v.as<ObjectValue>()["kk2"], false, true);
     }
-}
-
-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\":{}"
-                                                                "}"
-                                                            "]"
-                                            "}");
+    {
+        const auto& v =
+            jroot["foo"].as<ObjectValue>()["bar"].as<ObjectValue>()["baz"];
+        REPORTER_ASSERT(reporter, v.is<NullValue>());
+    }
 }