ValueSerializer: Take advantage of fast elements in dense array serialization.

src/value-serializer.cc

 // Copyright 2016 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/value-serializer.h"
 
 #include <type_traits>
 
 #include "src/base/logging.h"
 #include "src/conversions.h"
@@ skipping 433 matching lines @@
   bool valid_length = array->length()->ToArrayLength(&length);
   DCHECK(valid_length);
   USE(valid_length);
 
   // To keep things simple, for now we decide between dense and sparse
   // serialization based on elements kind. A more principled heuristic could
   // count the elements, but would need to take care to note which indices
   // existed (as only indices which were enumerable own properties at this point
   // should be serialized).
   const bool should_serialize_densely =
       array->HasFastElements() && !array->HasFastHoleyElements();

[Camillo Bruni, 2016/09/07 02:28:07]

This might be a future optimization point by serializing holey elements in a
dense fashion, which would require a special marker object for the_hole value
and some way to detect this case during materialization.

[jbroman, 2016/09/07 14:53:39]

Yeah, it's possible, though it takes a little care, because we have to store a
bitmask or equivalent of where the holes were to begin with, and not slow down
the non-holey path which is probably much more common. (Though it's interesting
to note that json-stringifier still doesn't have a fast path for holey
elements.)

 
   if (should_serialize_densely) {
     // TODO(jbroman): Distinguish between undefined and a hole (this can happen
     // if serializing one of the elements deletes another). This requires wire
     // format changes.
     WriteTag(SerializationTag::kBeginDenseJSArray);
     WriteVarint<uint32_t>(length);
-    for (uint32_t i = 0; i < length; i++) {
+    uint32_t i = 0;
+
+    // Fast paths. Note that FAST_ELEMENTS in particular can bail due to the
+    // structure of the elements changing.
+    switch (array->GetElementsKind()) {

[Camillo Bruni, 2016/09/07 02:28:08]

With only 3 cases present here there is currently no hurry. However, in the case
we need more cases here I'd prefer moving this to the ElementsAccessor which
provides more machinery to deal with the peculiarities of all the elements.

[jbroman, 2016/09/07 14:53:39]

I'm not sure what operation could be added to ElementsAccessor that would work
well here, though. Iteration, maybe, though it would imply some overhead (both
for abstraction and for virtual dispatch blocking inlining and subsequent
optimization).

+      case FAST_SMI_ELEMENTS: {
+        Handle<FixedArray> elements(FixedArray::cast(array->elements()),
+                                    isolate_);
+        for (; i < length; i++) WriteSmi(Smi::cast(elements->get(i)));
+        break;
+      }
+      case FAST_DOUBLE_ELEMENTS: {
+        Handle<FixedDoubleArray> elements(
+            FixedDoubleArray::cast(array->elements()), isolate_);
+        for (; i < length; i++) {
+          WriteTag(SerializationTag::kDouble);
+          WriteDouble(elements->get_scalar(i));
+        }
+        break;
+      }
+      case FAST_ELEMENTS: {
+        Handle<Object> old_length(array->length(), isolate_);
+        for (; i < length; i++) {
+          if (array->length() != *old_length ||
+              array->GetElementsKind() != FAST_ELEMENTS) {
+            // Fall back to slow path.
+            break;
+          }
+          Handle<Object> element(FixedArray::cast(array->elements())->get(i),
+                                 isolate_);
+          if (!WriteObject(element).FromMaybe(false)) return Nothing<bool>();
+        }
+        break;
+      }
+      default:
+        break;
+    }
+
+    // If there are elements remaining, serialize them slowly.
+    for (; i < length; i++) {
       // Serializing the array's elements can have arbitrary side effects, so we
       // cannot rely on still having fast elements, even if it did to begin
       // with.
       Handle<Object> element;
       LookupIterator it(isolate_, array, i, array, LookupIterator::OWN);
       if (!Object::GetProperty(&it).ToHandle(&element) ||
           !WriteObject(element).FromMaybe(false)) {

[Camillo Bruni, 2016/09/07 02:28:07]

edge-case nit: in the case where the array length gets changed to something <
length and > i you will fail with an exception even though you could properly
finish serialization (I'd like to see a test for exactly this case)

[jbroman, 2016/09/07 14:53:39]

I've added such a test case, but it passes already. This works as follows:
- falls out of fast path (array length changed)
- GetProperty(&it) succeeds, and returns undefined (it.WasFound() is false, but
isn't checked)

         return Nothing<bool>();
       }
     }
+
     KeyAccumulator accumulator(isolate_, KeyCollectionMode::kOwnOnly,
                                ENUMERABLE_STRINGS);
     if (!accumulator.CollectOwnPropertyNames(array, array).FromMaybe(false)) {
       return Nothing<bool>();
     }
     Handle<FixedArray> keys =
         accumulator.GetKeys(GetKeysConversion::kConvertToString);
     uint32_t properties_written;
     if (!WriteJSObjectPropertiesSlow(array, keys).To(&properties_written)) {
       return Nothing<bool>();
@@ skipping 956 matching lines @@
   if (stack.size() != 1) {
     isolate_->Throw(*isolate_->factory()->NewError(
         MessageTemplate::kDataCloneDeserializationError));
     return MaybeHandle<Object>();
   }
   return scope.CloseAndEscape(stack[0]);
 }
 
 }  // namespace internal
 }  // namespace v8