Blink-compatible deserialization of old object format.

src/value-serializer.cc

Index: src/value-serializer.cc
diff --git a/src/value-serializer.cc b/src/value-serializer.cc
index 8e74e91f9652100bc36b196ef9ea2c1c31114384..a6d0c3c2bac7ed620e7de1fd86a61dc05bd944b8 100644
--- a/src/value-serializer.cc
+++ b/src/value-serializer.cc
@@ -327,6 +327,12 @@ Maybe<SerializationTag> ValueDeserializer::PeekTag() const {
   return Just(tag);
 }
 
+void ValueDeserializer::ConsumeTag(SerializationTag peeked_tag) {
+  SerializationTag actual_tag = ReadTag().ToChecked();
+  DCHECK(actual_tag == peeked_tag);
+  (void)actual_tag;

[Camillo Bruni, 2016/08/16 09:04:07]

nit: you can use our very pretty USE macro here :)

[jbroman, 2016/08/16 21:34:14]

I was looking for one, but didn't find it. Thanks. :)

+}
+
 Maybe<SerializationTag> ValueDeserializer::ReadTag() {
   SerializationTag tag;
   do {
@@ -512,9 +518,7 @@ Maybe<uint32_t> ValueDeserializer::ReadJSObjectProperties(
     SerializationTag tag;
     if (!PeekTag().To(&tag)) return Nothing<uint32_t>();
     if (tag == end_tag) {
-      SerializationTag consumed_tag = ReadTag().ToChecked();
-      (void)consumed_tag;
-      DCHECK(tag == consumed_tag);
+      ConsumeTag(end_tag);
       return Just(num_properties);
     }
 
@@ -561,5 +565,72 @@ void ValueDeserializer::AddObjectWithID(uint32_t id,
   }
 }
 
+static MaybeHandle<JSObject> CreateJSObjectFromKeyValuePairs(
+    Isolate* isolate, Handle<Object>* data, uint32_t num_properties) {
+  Handle<JSObject> object =
+      isolate->factory()->NewJSObject(isolate->object_function());

[Camillo Bruni, 2016/08/16 09:04:07]

Performance Hint: This is a rather slow (but obviously correct) way to create a
new object. Depending on how far down the optimization road you will go for
future versions of the serialization format, this function is going to be
crucial :).

Not sure how familiar you are with all the internals of V8, so I apologize
upfront for the detailed comment here. 

Objects have several backing stores for properties, the main distinction lies
between array-indices which are stored in elements() and all the other
properties which we usually just call properties. Inside there we have several
distinctions.


For elements we optimize depending on the keys we use and on the objects stored
(you'll find all the implementation details in elements.cc). First of all, if we
have Smi-ranged keys we're using fast-elements, if we have large keys and/or
accessors for indices we resort to a dictionary elements kind (= slow elements).
In the fast case we distinguish between packed and non packed depending on
whether they contain a hole value or not. A hole appears when you use `delete
arrayish[i]`.

For properties (aka. non-indices keys) we have first have fast-in-object
properties, which are, very much according to their name, directly in the object
and typically used to store properties that need fast access (currently used on
a first-come basis). Then we have the properties store which can either be fast
(just a list) or slow (a dictionary). We resort to slow-properties (not related
to slow-elements) when we have too many properties, if we have accessors and if
you delete a property (based on the assumption that the object will be used as a
dictionary henceforth).

For fast properties we keep track of the object structure by means of hidden
classes which are called (confusingly) Maps. This means whenever you add a new
property to an object we transition the object internally to a new Map/hidden
class (see Object::SetDataProperty). However, we only do this for fast
properties (so not for element indices and not for slow/dictionary properties)

In the context of serialization, you may want to keep track of the original
property representation, especially for the elements (aka. it's ELEMENTS_KIND).
This will be important to make sure you can quickly materialize objects without
transitioning the backing stores from the uninitialized state via some
intermediate version to the final version. For instance, each property added
might require the properties store (list for fast-properties, dictionary for
slow-properties) to be copied over. So simply making sure we reserve enough
space upfront for properties and elements can save quite some work here (of
course ideally you want to directly start off with the right elements-kind and
right property type).

Getting the right elements kind and size should be rather easy to achieve as you
could simply encode this in the serialization format, or even deduce it from the
keys you've seen. In this case you would simply transition to the new elements
kind from the pristine object (note this will do a Map transition under the
hood).

Getting the properties right might be trickier, since for every fast property
added we need the matching Map. So as a first step I simply suggest making sure
that the object is set to dictionary mode (aka. slow properties) so we do not
have to generate and check the maps along-side the property addition. This comes
at the cost of optimizations being disabled where the new object is being used.
Many optimizations require a stable object structure which can only be ensured
with a precise Map attached to it, which is not the case for dict-mode objects.

SUMMARY:
- for now-ish (optimiztion phase) use JSObject::NormalizeProperties to reserve
enough space for new properties and resort to cheap to add dictionary properties
if there are too many properties (see Map::TooManyFastProperties)
- check how JsonParser::ParseJsonObject shortcuts some of the transition logic
(which would work if you had an JSObjectStart tag or so)

[jbroman, 2016/08/16 21:34:14]

No problem. I've learned a lot the last couple weeks, but there's a lot that I
haven't yet learned.
I saw that. AFAICT holes look the same as undefined to script, but I gather
there's some subtlety here I'm missing.
This CL (as opposed to the other one) deals with "version 0" objects, which
Chrome has not written for over four years (but which Blink must still support
because someone may have one stored in IndexedDB). I'd rather bias this code
toward simplicity and focus optimization work on the path that's likely to be
used much more in practice (especially for postMessage, which will always use
the latest version).

From what I understand, the JSON parser code does do map transitions the same
way that DefineOwnPropertyIgnoreAttributes does, except that it has a fast path
for the case where a simple transition is available.

Versions afterwards do have a "begin object" tag (this is needed so that
references to the object in its properties can be correctly resolved), though
they don't currently write down any more information. I could imagine storing
some sort of hint about whether fast or dictionary objects were likely to be
useful (to avoid doing the transitions if we'll fall back to dictionary
properties anyhow), but that seems a way down the optimization road to me (and
obviously doesn't exist in currently stored IndexedDB data).

+  for (unsigned i = 0; i < 2 * num_properties; i += 2) {
+    Handle<Object> key = data[i];
+    Handle<Object> value = data[i + 1];
+    bool success;
+    LookupIterator it = LookupIterator::PropertyOrElement(
+        isolate, object, key, &success, LookupIterator::OWN);
+    if (!success ||
+        JSObject::DefineOwnPropertyIgnoreAttributes(&it, value, NONE).is_null())
+      return MaybeHandle<JSObject>();

[Camillo Bruni, 2016/08/16 09:04:07]

nit: please add braces here, we don't allow trailing statement after an if
statement (unless on the same line).
Could you check the other places in this file as well? They slipped through my
last reviews...

[jbroman, 2016/08/16 21:34:14]

OK. As I mentioned in the other review, I couldn't find a reference to this in a
style guide, so I'm not sure what other rules I could be missing. I don't want
to waste reviewers' time. :)

+  }
+  return object;
+}
+
+MaybeHandle<Object>
+ValueDeserializer::ReadObjectUsingEntireBufferForLegacyFormat() {
+  if (version_ > 0) return MaybeHandle<Object>();
+
+  HandleScope scope(isolate_);
+  std::vector<Handle<Object>> stack;
+  while (position_ < end_) {
+    SerializationTag tag;
+    if (!PeekTag().To(&tag)) break;
+
+    Handle<Object> new_object;
+    switch (tag) {
+      case SerializationTag::kEndJSObject: {
+        ConsumeTag(SerializationTag::kEndJSObject);
+
+        // JS Object: Read the last 2*n values from the stack and use them as
+        // key-value pairs.
+        uint32_t num_properties;
+        if (!ReadVarint<uint32_t>().To(&num_properties) ||
+            stack.size() / 2 < num_properties)
+          return MaybeHandle<Object>();

[Camillo Bruni, 2016/08/16 09:04:07]

nit: missing braces

[jbroman, 2016/08/16 21:34:14]

Done.

+
+        size_t begin_properties = stack.size() - 2 * num_properties;
+        Handle<Object>* data =
+            num_properties ? &stack[begin_properties] : nullptr;
+        if (!CreateJSObjectFromKeyValuePairs(isolate_, data, num_properties)
+                 .ToHandle(&new_object))
+          return MaybeHandle<Object>();

[Camillo Bruni, 2016/08/16 09:04:07]

nit: braces

[jbroman, 2016/08/16 21:34:14]

Done.

+
+        stack.resize(begin_properties);
+        break;
+      }
+      default:
+        if (!ReadObject().ToHandle(&new_object)) return MaybeHandle<Object>();
+        break;
+    }
+    stack.push_back(new_object);
+  }
+
+// Nothing remains but padding.
+#ifdef DEBUG
+  while (position_ < end_)
+    DCHECK(*position_++ == static_cast<uint8_t>(SerializationTag::kPadding));

[Camillo Bruni, 2016/08/16 09:04:07]

nit: braces

[jbroman, 2016/08/16 21:34:14]

Done.

+#endif
+  position_ = end_;
+
+  if (stack.size() != 1) return MaybeHandle<Object>();
+  return scope.CloseAndEscape(stack[0]);
+}
+
 }  // namespace internal
 }  // namespace v8