Flatten the Arrays returned and consumed by the v8::Map API

This will significantly simplify the serialization code, as well
as speeding it up (by triggering only a single allocation instead of O(size)
allocations).

BUG=chromium:478263
LOG=y

Review URL: https://codereview.chromium.org/1157843006

Cr-Commit-Position: refs/heads/master@{#28793}

diff --git a/include/v8.h b/include/v8.h
index ab17d12b1c3b0bc49ed2e0675ef3306bef616e06..bcab5bc6fd4bd57b05cf3fe205a0af9ee6bb9ad2 100644 (file)
--- a/include/v8.h
+++ b/include/v8.h
@@ -2977,8 +2977,8 @@ class V8_EXPORT Map : public Object {
   size_t Size() const;
 
   /**
-   * Returns an array of [key, value] arrays representing the contents
-   * of this Map.
+   * Returns an array of length Size() * 2, where index N is the Nth key and
+   * index N + 1 is the Nth value.
    */
   Local<Array> AsArray() const;
 
@@ -2988,8 +2988,8 @@ class V8_EXPORT Map : public Object {
   static Local<Map> New(Isolate* isolate);
 
   /**
-   * Creates a new Map containing the elements of array, which must be comprised
-   * of [key, value] arrays.
+   * Creates a new Map containing the elements of array, which must be formatted
+   * in the same manner as the array returned from AsArray().
    * Guaranteed to be side-effect free if the array contains no holes.
    */
   static V8_WARN_UNUSED_RESULT MaybeLocal<Map> FromArray(Local<Context> context,

diff --git a/src/api.cc b/src/api.cc
index 6f9a745ab44e5fe4d9d92c469066d6c375515e5a..994429a013426463dd82c29a00664612a85ba010 100644 (file)
--- a/src/api.cc
+++ b/src/api.cc
@@ -6295,17 +6295,13 @@ Local<Array> Map::AsArray() const {
   LOG_API(isolate, "Map::AsArray");
   ENTER_V8(isolate);
   i::Handle<i::OrderedHashMap> table(i::OrderedHashMap::cast(obj->table()));
-  int length = table->NumberOfElements();
+  int size = table->NumberOfElements();
+  int length = size * 2;
   i::Handle<i::FixedArray> result = factory->NewFixedArray(length);
-  for (int i = 0; i < length; ++i) {
+  for (int i = 0; i < size; ++i) {
     if (table->KeyAt(i)->IsTheHole()) continue;
-    i::HandleScope handle_scope(isolate);
-    i::Handle<i::FixedArray> entry = factory->NewFixedArray(2);
-    entry->set(0, table->KeyAt(i));
-    entry->set(1, table->ValueAt(i));
-    i::Handle<i::JSArray> entry_array =
-        factory->NewJSArrayWithElements(entry, i::FAST_ELEMENTS, 2);
-    result->set(i, *entry_array);
+    result->set(i * 2, table->KeyAt(i));
+    result->set(i * 2 + 1, table->ValueAt(i));
   }
   i::Handle<i::JSArray> result_array =
       factory->NewJSArrayWithElements(result, i::FAST_ELEMENTS, length);
@@ -6315,6 +6311,9 @@ Local<Array> Map::AsArray() const {
 
 MaybeLocal<Map> Map::FromArray(Local<Context> context, Local<Array> array) {
   PREPARE_FOR_EXECUTION(context, "Map::FromArray", Map);
+  if (array->Length() % 2 != 0) {
+    return MaybeLocal<Map>();
+  }
   i::Handle<i::Object> result;
   i::Handle<i::Object> argv[] = {Utils::OpenHandle(*array)};
   has_pending_exception =

diff --git a/src/collection.js b/src/collection.js
index 1c6b475df6cf4761f4d25738150b1d3bfa3c44d5..86898529c3b7e8289e55b33696307c216eb3dfe5 100644 (file)
--- a/src/collection.js
+++ b/src/collection.js
@@ -485,9 +485,10 @@ $getExistingHash = GetExistingHash;
 $mapFromArray = function(array) {
   var map = new GlobalMap;
   var length = array.length;
-  for (var i = 0; i < length; ++i) {
-    var entry = array[i];
-    %_CallFunction(map, entry[0], entry[1], MapSet);
+  for (var i = 0; i < length; i += 2) {
+    var key = array[i];
+    var value = array[i + 1];
+    %_CallFunction(map, key, value, MapSet);
   }
   return map;
 };

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 9abccae75067cf98a461dc5815ef79c1da73d042..9101257d227e6031b85b83e51ecc6af6e6d3ef1d 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -21384,19 +21384,19 @@ TEST(Map) {
   map = v8::Local<v8::Map>::Cast(val);
   CHECK_EQ(2, map->Size());
 
-  v8::Local<v8::Array> entries = map->AsArray();
-  CHECK_EQ(2, entries->Length());
-  v8::Local<v8::Array> entry = entries->Get(0).As<v8::Array>();
-  CHECK_EQ(2, entry->Length());
-  CHECK_EQ(1, entry->Get(0).As<v8::Int32>()->Value());
-  CHECK_EQ(2, entry->Get(1).As<v8::Int32>()->Value());
-  entry = entries->Get(1).As<v8::Array>();
-  CHECK_EQ(2, entry->Length());
-  CHECK_EQ(3, entry->Get(0).As<v8::Int32>()->Value());
-  CHECK_EQ(4, entry->Get(1).As<v8::Int32>()->Value());
-
-  map = v8::Map::FromArray(env.local(), entries).ToLocalChecked();
+  v8::Local<v8::Array> contents = map->AsArray();
+  CHECK_EQ(4, contents->Length());
+  CHECK_EQ(1, contents->Get(0).As<v8::Int32>()->Value());
+  CHECK_EQ(2, contents->Get(1).As<v8::Int32>()->Value());
+  CHECK_EQ(3, contents->Get(2).As<v8::Int32>()->Value());
+  CHECK_EQ(4, contents->Get(3).As<v8::Int32>()->Value());
+
+  map = v8::Map::FromArray(env.local(), contents).ToLocalChecked();
   CHECK_EQ(2, map->Size());
+
+  // Odd lengths result in a null MaybeLocal.
+  contents = v8::Array::New(isolate, 41);
+  CHECK(v8::Map::FromArray(env.local(), contents).IsEmpty());
 }