Reland^2 "[runtime] Amortize descriptor array growing for fast-mode prototypes"

Fix: check whether the map of the back pointer is the metamap rather than reading the map of the constructor-or-backpointer slot. If the slot contains a constructor, it's possible that the object transitions while the concurrent marker is reading the map (from which it's reading the instance type); and it's possible that the transitioned map isn't set up yet fully when we read the instance type. An acquire load for the constructor-or-backpointer map would also fix it by serializing stores, but is more expensive. Checking the metamap is faster.

In case of false negatives (it is a map but we read the field before it was properly initialized) we'll simply mark too many descriptors in the worst case.

Original change's description:
> Revert "Reland "[runtime] Amortize descriptor array growing for fast-mode prototypes""
> 
> This reverts commit 71f9c117.
> 
> Reason for revert: Seems to cause several TSan flakes, e.g. https://ci.chromium.org/p/v8/builders/ci/V8%20Linux64%20TSAN%20-%20concurrent%20marking/12926
> 
> Original change's description:
> > Reland "[runtime] Amortize descriptor array growing for fast-mode prototypes"
> > 
> > This is a reland of 2de2d3dc
> > 
> > Original change's description:
> > > [runtime] Amortize descriptor array growing for fast-mode prototypes
> > >
> > > This avoids an O(n^2) algorithm that creates an equal amount of garbage.
> > > Even though the actual final descriptor array might be a little bigger,
> > > it reduces peak memory usage by allocating less.
> > >
> > > Bug: b:148346655
> > > Change-Id: I984159d36e9e0b37c19bc81afc90c94c9a9d168a
> > > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2135728
> > > Commit-Queue: Toon Verwaest <verwaest@chromium.org>
> > > Reviewed-by: Igor Sheludko <ishell@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#67031}
> > 
> > Bug: b:148346655, v8:10339
> > Change-Id: I24436d8f49dc1fe527c4f6558db1abcba323b6f8
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2139215
> > Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> > Reviewed-by: Igor Sheludko <ishell@chromium.org>
> > Auto-Submit: Toon Verwaest <verwaest@chromium.org>
> > Commit-Queue: Igor Sheludko <ishell@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#67475}
> 
> TBR=ulan@chromium.org,ishell@chromium.org,verwaest@chromium.org
> 
> Change-Id: I6fa02d0c89557eae33b792c1fe62c9c15eb0f7c7
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: b:148346655, v8:10339
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2172749
> Reviewed-by: Clemens Backes <clemensb@chromium.org>
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#67478}

TBR=ulan@chromium.org,clemensb@chromium.org,ishell@chromium.org,verwaest@chromium.org

Change-Id: Ib86e039374e721919cd5b02495c252ee7af283bd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: b:148346655, v8:10339
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2173359Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67495}

src/diagnostics/objects-debug.cc

@@ -439,8 +439,12 @@ void Map::MapVerify(Isolate* isolate) {
     if (GetBackPointer().IsUndefined(isolate)) {
       // Root maps must not have descriptors in the descriptor array that do not
       // belong to the map.
-      CHECK_EQ(NumberOfOwnDescriptors(),
-               instance_descriptors().number_of_descriptors());
+      int nof = instance_descriptors().number_of_descriptors();
+      if (IsDetached(isolate)) {
+        CHECK(base::IsInRange(NumberOfOwnDescriptors(), 0, nof));
+      } else {
+        CHECK_EQ(NumberOfOwnDescriptors(), nof);
+      }
     } else {
       // If there is a parent map it must be non-stable.
       Map parent = Map::cast(GetBackPointer());

src/heap/marking-visitor-inl.h

@@ -407,7 +407,14 @@ int MarkingVisitorBase<ConcreteVisitor, MarkingState>::VisitMap(Map meta_map,
     DescriptorArray descriptors = map.synchronized_instance_descriptors();
     size += MarkDescriptorArrayBlack(map, descriptors);
     int number_of_own_descriptors = map.NumberOfOwnDescriptors();
-    if (number_of_own_descriptors) {
+    if (map.IsDetached(heap_->isolate())) {
+      // Mark all descriptors in detached maps. Descriptor arrays are reused as
+      // objects transition between maps, and its possible that a newer map dies
+      // before an older map. Marking all descriptors through each map
+      // guarantees that we don't end up with dangling references that we don't
+      // clear since there are no explicit transitions.
+      VisitDescriptors(descriptors, descriptors.number_of_descriptors());
+    } else if (number_of_own_descriptors) {
       // It is possible that the concurrent marker observes the
       // number_of_own_descriptors out of sync with the descriptors. In that
       // case the marking write barrier for the descriptor array will ensure

src/json/json-parser.cc

@@ -838,7 +838,7 @@ MaybeHandle<Object> JsonParser<Char>::ParseJsonValue() {
             Map maybe_feedback = JSObject::cast(*element_stack.back()).map();
             // Don't consume feedback from objects with a map that's detached
             // from the transition tree.
-            if (!maybe_feedback.GetBackPointer().IsUndefined(isolate_)) {
+            if (!maybe_feedback.IsDetached(isolate_)) {
               feedback = handle(maybe_feedback, isolate_);
               if (feedback->is_deprecated()) {
                 feedback = Map::Update(isolate_, feedback);

src/objects/map-inl.h

@@ -123,6 +123,12 @@ bool Map::CanHaveFastTransitionableElementsKind() const {
   return CanHaveFastTransitionableElementsKind(instance_type());
 }
 
+bool Map::IsDetached(Isolate* isolate) const {
+  if (is_prototype_map()) return true;
+  return instance_type() == JS_OBJECT_TYPE && NumberOfOwnDescriptors() > 0 &&
+         GetBackPointer().IsUndefined(isolate);
+}
+
 // static
 void Map::GeneralizeIfCanHaveTransitionableFastElementsKind(
     Isolate* isolate, InstanceType instance_type,

src/objects/map.cc

@@ -658,7 +658,7 @@ Map Map::FindRootMap(Isolate* isolate) const {
     if (back.IsUndefined(isolate)) {
       // Initial map must not contain descriptors in the descriptors array
       // that do not belong to the map.
-      DCHECK_EQ(result.NumberOfOwnDescriptors(),
+      DCHECK_LE(result.NumberOfOwnDescriptors(),
                 result.instance_descriptors().number_of_descriptors());
       return result;
     }
@@ -1221,7 +1221,7 @@ Map Map::FindElementsKindTransitionedMap(Isolate* isolate,
   DisallowHeapAllocation no_allocation;
   DisallowDeoptimization no_deoptimization(isolate);
 
-  if (is_prototype_map()) return Map();
+  if (IsDetached(isolate)) return Map();
 
   ElementsKind kind = elements_kind();
   bool packed = IsFastPackedElementsKind(kind);
@@ -1354,7 +1354,7 @@ static Handle<Map> AddMissingElementsTransitions(Isolate* isolate,
 
   ElementsKind kind = map->elements_kind();
   TransitionFlag flag;
-  if (map->is_prototype_map()) {
+  if (map->IsDetached(isolate)) {
     flag = OMIT_TRANSITION;
   } else {
     flag = INSERT_TRANSITION;
@@ -1721,14 +1721,14 @@ void Map::ConnectTransition(Isolate* isolate, Handle<Map> parent,
                  child->may_have_interesting_symbols());
   if (!parent->GetBackPointer().IsUndefined(isolate)) {
     parent->set_owns_descriptors(false);
-  } else {
+  } else if (!parent->IsDetached(isolate)) {
     // |parent| is initial map and it must not contain descriptors in the
     // descriptors array that do not belong to the map.
     DCHECK_EQ(parent->NumberOfOwnDescriptors(),
               parent->instance_descriptors().number_of_descriptors());
   }
-  if (parent->is_prototype_map()) {
-    DCHECK(child->is_prototype_map());
+  if (parent->IsDetached(isolate)) {
+    DCHECK(child->IsDetached(isolate));
     if (FLAG_trace_maps) {
       LOG(isolate, MapEvent("Transition", parent, child, "prototype", name));
     }
@@ -1755,7 +1755,9 @@ Handle<Map> Map::CopyReplaceDescriptors(
     result->set_may_have_interesting_symbols(true);
   }
 
-  if (!map->is_prototype_map()) {
+  if (map->is_prototype_map()) {
+    result->InitializeDescriptors(isolate, *descriptors, *layout_descriptor);
+  } else {
     if (flag == INSERT_TRANSITION &&
         TransitionsAccessor(isolate, map).CanHaveMoreTransitions()) {
       result->InitializeDescriptors(isolate, *descriptors, *layout_descriptor);
@@ -1766,19 +1768,11 @@ Handle<Map> Map::CopyReplaceDescriptors(
       descriptors->GeneralizeAllFields();
       result->InitializeDescriptors(isolate, *descriptors,
                                     LayoutDescriptor::FastPointerLayout());
-      // If we were trying to insert a transition but failed because there are
-      // too many transitions already, mark the object as a prototype to avoid
-      // tracking transitions from the detached map.
-      if (flag == INSERT_TRANSITION) {
-        result->set_is_prototype_map(true);
-      }
     }
-  } else {
-    result->InitializeDescriptors(isolate, *descriptors, *layout_descriptor);
   }
   if (FLAG_trace_maps &&
       // Mirror conditions above that did not call ConnectTransition().
-      (map->is_prototype_map() ||
+      (map->IsDetached(isolate) ||
        !(flag == INSERT_TRANSITION &&
          TransitionsAccessor(isolate, map).CanHaveMoreTransitions()))) {
     LOG(isolate, MapEvent("ReplaceDescriptors", map, result, reason,
@@ -1960,7 +1954,7 @@ Handle<Map> Map::AsLanguageMode(Isolate* isolate, Handle<Map> initial_map,
 }
 
 Handle<Map> Map::CopyForElementsTransition(Isolate* isolate, Handle<Map> map) {
-  DCHECK(!map->is_prototype_map());
+  DCHECK(!map->IsDetached(isolate));
   Handle<Map> new_map = CopyDropDescriptors(isolate, map);
 
   if (map->owns_descriptors()) {
@@ -2161,7 +2155,7 @@ Handle<Map> Map::TransitionToDataProperty(Isolate* isolate, Handle<Map> map,
                                           StoreOrigin store_origin) {
   RuntimeCallTimerScope stats_scope(
       isolate,
-      map->is_prototype_map()
+      map->IsDetached(isolate)
           ? RuntimeCallCounterId::kPrototypeMap_TransitionToDataProperty
           : RuntimeCallCounterId::kMap_TransitionToDataProperty);
 
@@ -2275,7 +2269,7 @@ Handle<Map> Map::TransitionToAccessorProperty(Isolate* isolate, Handle<Map> map,
                                               PropertyAttributes attributes) {
   RuntimeCallTimerScope stats_scope(
       isolate,
-      map->is_prototype_map()
+      map->IsDetached(isolate)
           ? RuntimeCallCounterId::kPrototypeMap_TransitionToAccessorProperty
           : RuntimeCallCounterId::kMap_TransitionToAccessorProperty);
 
@@ -2390,19 +2384,64 @@ Handle<Map> Map::CopyAddDescriptor(Isolate* isolate, Handle<Map> map,
     return ShareDescriptor(isolate, map, descriptors, descriptor);
   }
 
-  int nof = map->NumberOfOwnDescriptors();
-  Handle<DescriptorArray> new_descriptors =
-      DescriptorArray::CopyUpTo(isolate, descriptors, nof, 1);
-  new_descriptors->Append(descriptor);
+  Handle<DescriptorArray> new_descriptors;
+  Handle<LayoutDescriptor> new_layout_descriptor;
+  if (map->IsDetached(isolate)) {
+    if (descriptors->number_of_slack_descriptors() == 0) {
+      int old_size = descriptors->number_of_descriptors();
+      if (old_size == 0) {
+        new_descriptors = DescriptorArray::Allocate(isolate, 0, 1);
+      } else {
+        int slack = SlackForArraySize(old_size, kMaxNumberOfDescriptors);
+        EnsureDescriptorSlack(isolate, map, slack);
+        new_descriptors = handle(map->instance_descriptors(), isolate);
+      }
+    } else {
+      new_descriptors = descriptors;
+    }
+    new_layout_descriptor =
+        FLAG_unbox_double_fields
+            ? LayoutDescriptor::ShareAppend(isolate, map,
+                                            descriptor->GetDetails())
+            : handle(LayoutDescriptor::FastPointerLayout(), isolate);
+  } else {
+    int nof = map->NumberOfOwnDescriptors();
+    new_descriptors = DescriptorArray::CopyUpTo(isolate, descriptors, nof, 1);
+    new_layout_descriptor =
+        FLAG_unbox_double_fields
+            ? LayoutDescriptor::New(isolate, map, new_descriptors, nof + 1)
+            : handle(LayoutDescriptor::FastPointerLayout(), isolate);
+  }
 
-  Handle<LayoutDescriptor> new_layout_descriptor =
-      FLAG_unbox_double_fields
-          ? LayoutDescriptor::New(isolate, map, new_descriptors, nof + 1)
-          : handle(LayoutDescriptor::FastPointerLayout(), isolate);
+  Handle<Map> result = CopyDropDescriptors(isolate, map);
 
-  return CopyReplaceDescriptors(
-      isolate, map, new_descriptors, new_layout_descriptor, flag,
-      descriptor->GetKey(), "CopyAddDescriptor", SIMPLE_PROPERTY_TRANSITION);
+  if (descriptor->GetKey()->IsInterestingSymbol()) {
+    result->set_may_have_interesting_symbols(true);
+  }
+
+  {
+    DisallowHeapAllocation no_gc;
+    new_descriptors->Append(descriptor);
+    result->InitializeDescriptors(isolate, *new_descriptors,
+                                  *new_layout_descriptor);
+  }
+
+  if (flag == INSERT_TRANSITION &&
+      TransitionsAccessor(isolate, map).CanHaveMoreTransitions()) {
+    ConnectTransition(isolate, map, result, descriptor->GetKey(),
+                      SIMPLE_PROPERTY_TRANSITION);
+  }
+
+  if (FLAG_trace_maps &&
+      // Mirror conditions above that did not call ConnectTransition().
+      (map->IsDetached(isolate) ||
+       !(flag == INSERT_TRANSITION &&
+         TransitionsAccessor(isolate, map).CanHaveMoreTransitions()))) {
+    LOG(isolate, MapEvent("ReplaceDescriptors", map, result,
+                          "CopyAddDescriptor", descriptor->GetKey()));
+  }
+
+  return result;
 }
 
 Handle<Map> Map::CopyInsertDescriptor(Isolate* isolate, Handle<Map> map,

src/objects/map.h

@@ -422,6 +422,11 @@ class Map : public HeapObject {
   inline bool has_sealed_elements() const;
   inline bool has_frozen_elements() const;
 
+  // Weakly checks whether a map is detached from all transition trees. If this
+  // returns true, the map is guaranteed to be detached. If it returns false,
+  // there is no guarantee it is attached.
+  inline bool IsDetached(Isolate* isolate) const;
+
   // Returns true if the current map doesn't have DICTIONARY_ELEMENTS but if a
   // map with DICTIONARY_ELEMENTS was found in the prototype chain.
   bool DictionaryElementsInPrototypeChainOnly(Isolate* isolate);