[map] Properly share the map for builtin iterator result objects.

Previously we had a special, unshared map on the native context that was
used for results of builtin iterators, which was different from the map
that is created from an object literal like `{value, done}`. This not
only leads to unnecessary polymorphism, but also makes it impossible
for user defined iterators to take the fast-paths that we have in
various places (i.e. in collections or promises).

With this change we now properly share the map for `{value, done}` and
use that for the builtin iterator result objects, as well as the
fast-paths.

Drive-by-fix: Remove the restrictions on map caching and transition
caching during bootstrapping. This no longer makes sense.

Bug: v8:9114, v8:9243
Change-Id: I19eb9071f7ec0ed58f8a6f87eed781bc790174b7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1609794
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61488}

src/bootstrapper.cc

@@ -3510,26 +3510,32 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
   }
 
   {  // -- I t e r a t o r R e s u l t
-    Handle<Map> map = factory->NewMap(JS_OBJECT_TYPE, JSIteratorResult::kSize,
-                                      TERMINAL_FAST_ELEMENTS_KIND, 2);
-    Map::SetPrototype(isolate(), map, isolate_->initial_object_prototype());
-    Map::EnsureDescriptorSlack(isolate_, map, 2);
-
-    {  // value
-      Descriptor d = Descriptor::DataField(isolate(), factory->value_string(),
-                                           JSIteratorResult::kValueIndex, NONE,
-                                           Representation::Tagged());
-      map->AppendDescriptor(isolate(), &d);
-    }
+    // Setup the map for IterResultObjects created from builtins in such a
+    // way that it's exactly the same map as the one produced by object
+    // literals in the form `{value, done}`. This way we have better sharing
+    // of maps (i.e. less polymorphism) and also make it possible to hit the
+    // fast-paths in various builtins (i.e. promises and collections) with
+    // user defined iterators.
+    Handle<Map> map = factory->ObjectLiteralMapFromCache(native_context(), 2);
+
+    // value
+    map = Map::CopyWithField(isolate(), map, factory->value_string(),
+                             FieldType::Any(isolate()), NONE,
+                             PropertyConstness::kConst,
+                             Representation::Tagged(), INSERT_TRANSITION)
+              .ToHandleChecked();
 
-    {  // done
-      Descriptor d = Descriptor::DataField(isolate(), factory->done_string(),
-                                           JSIteratorResult::kDoneIndex, NONE,
-                                           Representation::Tagged());
-      map->AppendDescriptor(isolate(), &d);
-    }
+    // done
+    // TODO(bmeurer): Once FLAG_modify_field_representation_inplace is always
+    // on, we can say Representation::HeapObject() here and have the inplace
+    // update logic take care of the case where someone ever stores a Smi into
+    // the done field.
+    map = Map::CopyWithField(isolate(), map, factory->done_string(),
+                             FieldType::Any(isolate()), NONE,
+                             PropertyConstness::kConst,
+                             Representation::Tagged(), INSERT_TRANSITION)
+              .ToHandleChecked();
 
-    map->SetConstructor(native_context()->object_function());
     native_context()->set_iterator_result_map(*map);
   }
 

src/heap/factory.cc

@@ -3901,11 +3901,6 @@ Handle<Map> Factory::ObjectLiteralMapFromCache(Handle<NativeContext> context,
     return handle(context->object_function()->initial_map(), isolate());
   }
 
-  // We do not cache maps for too many properties or when running builtin code.
-  if (isolate()->bootstrapper()->IsActive()) {
-    return Map::Create(isolate(), number_of_properties);
-  }
-
   // Use initial slow object proto map for too many properties.
   const int kMapCacheSize = 128;
   if (number_of_properties > kMapCacheSize) {

src/objects/map.cc

@@ -1700,16 +1700,6 @@ void Map::ConnectTransition(Isolate* isolate, Handle<Map> parent,
                  child->may_have_interesting_symbols());
   DCHECK_IMPLIES(parent->may_have_interesting_symbols(),
                  child->may_have_interesting_symbols());
-  // Do not track transitions during bootstrap except for element transitions.
-  if (isolate->bootstrapper()->IsActive() &&
-      !name.is_identical_to(isolate->factory()->elements_transition_symbol())) {
-    if (FLAG_trace_maps) {
-      LOG(isolate,
-          MapEvent("Transition", *parent, *child,
-                   child->is_prototype_map() ? "prototype" : "", *name));
-    }
-    return;
-  }
   if (!parent->GetBackPointer()->IsUndefined(isolate)) {
     parent->set_owns_descriptors(false);
   } else {
@@ -2015,9 +2005,12 @@ Handle<Map> Map::CopyForPreventExtensions(
       attrs_to_add);
   Handle<LayoutDescriptor> new_layout_descriptor(map->GetLayoutDescriptor(),
                                                  isolate);
+  // Do not track transitions during bootstrapping.
+  TransitionFlag flag =
+      isolate->bootstrapper()->IsActive() ? OMIT_TRANSITION : INSERT_TRANSITION;
   Handle<Map> new_map = CopyReplaceDescriptors(
-      isolate, map, new_desc, new_layout_descriptor, INSERT_TRANSITION,
-      transition_marker, reason, SPECIAL_TRANSITION);
+      isolate, map, new_desc, new_layout_descriptor, flag, transition_marker,
+      reason, SPECIAL_TRANSITION);
   new_map->set_is_extensible(false);
   if (!IsFixedTypedArrayElementsKind(map->elements_kind())) {
     ElementsKind new_kind = IsStringWrapperElementsKind(map->elements_kind())
@@ -2153,7 +2146,9 @@ Handle<Map> Map::TransitionToDataProperty(Isolate* isolate, Handle<Map> map,
                                     value);
   }
 
-  TransitionFlag flag = INSERT_TRANSITION;
+  // Do not track transitions during bootstrapping.
+  TransitionFlag flag =
+      isolate->bootstrapper()->IsActive() ? OMIT_TRANSITION : INSERT_TRANSITION;
   MaybeHandle<Map> maybe_map;
   if (!map->TooManyFastProperties(store_origin)) {
     Representation representation = value->OptimalRepresentation();
@@ -2335,7 +2330,9 @@ Handle<Map> Map::TransitionToAccessorProperty(Isolate* isolate, Handle<Map> map,
 
   pair->SetComponents(*getter, *setter);
 
-  TransitionFlag flag = INSERT_TRANSITION;
+  // Do not track transitions during bootstrapping.
+  TransitionFlag flag =
+      isolate->bootstrapper()->IsActive() ? OMIT_TRANSITION : INSERT_TRANSITION;
   Descriptor d = Descriptor::AccessorConstant(name, pair, attributes);
   return Map::CopyInsertDescriptor(isolate, map, &d, flag);
 }

test/mjsunit/regress/regress-v8-9243.js

+// Copyright 2019 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.
+
+// Flags: --allow-natives-syntax
+
+// The special IterResultObject map that builtins use should be the same
+// as the one produced by the `{value, done}` object literal.
+const user = {value:undefined, done:true};
+
+// Array iterator.
+const arrayResult = (new Array())[Symbol.iterator]().next();
+assertTrue(%HaveSameMap(user, arrayResult));
+
+// Map iterator.
+const mapResult = (new Map())[Symbol.iterator]().next();
+assertTrue(%HaveSameMap(user, mapResult));
+
+// Set iterator.
+const setResult = (new Set())[Symbol.iterator]().next();
+assertTrue(%HaveSameMap(user, setResult));
+
+// Generator.
+function* generator() {}
+const generatorResult = generator().next();
+assertTrue(%HaveSameMap(user, setResult));