[turbofan] Avoid megamorphic loads for zero-map mono/polymorphic sites

Soft-deopt for mono/polymorphic property accesses that don't have any
maps, and only allow zero-map feedback to be monomorphic. This makes
sure we only emit a megamorphic LoadIC builtin call if the IC was
actually megamorphic.

JSGenericLowering assumed that zero maps meant that a load site is
megamorphic. However, it can be the case that the call-site is
monomorphic or polymorphic, and the maps had died. In this case we don't
want to call the megamorphic IC builtin, as on a stub cache miss we
fallback to a normal LoadIC miss, which can record mono/polymorphic
feedback in the IC. After this, we'll enter a miss loop in the
megamorphic load builtin, and worse the LoadIC assumes that there's
something "wrong" with the feedback, so it'll keep trying to reconfigure
the handler (possibly allocating new load handlers if this is a
prototype field access).

As a drive-by, rewrite GetRelevantReceiverMaps to be an in-place
filtering of the maps rather than copying them.

Change-Id: I0c25bfa606367fa81c43223bbd56cdadb5e789ef
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2150586Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67152}

src/compiler/js-heap-broker.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "src/compiler/js-heap-broker.h"
+#include "src/common/globals.h"
 #include "src/compiler/heap-refs.h"
 
 #ifdef ENABLE_SLOW_DCHECKS
@@ -4668,17 +4669,25 @@ bool JSHeapBroker::FeedbackIsInsufficient(FeedbackSource const& source) const {
 }
 
 namespace {
-MapHandles GetRelevantReceiverMaps(Isolate* isolate, MapHandles const& maps) {
-  MapHandles result;
-  for (Handle<Map> map : maps) {
+// Remove unupdatable and abandoned prototype maps in-place.
+void FilterRelevantReceiverMaps(Isolate* isolate, MapHandles* maps) {
+  auto in = maps->begin();
+  auto out = in;
+  auto end = maps->end();
+
+  for (; in != end; ++in) {
+    Handle<Map> map = *in;
     if (Map::TryUpdate(isolate, map).ToHandle(&map) &&
         !map->is_abandoned_prototype_map()) {
       DCHECK(!map->is_deprecated());
-      result.push_back(map);
+      *out = *in;
+      ++out;
     }
   }
-  return result;
-}  // namespace
+
+  // Remove everything between the last valid map and the end of the vector.
+  maps->erase(out, end);
+}
 }  // namespace
 
 ProcessedFeedback const& JSHeapBroker::ReadFeedbackForPropertyAccess(
@@ -4690,14 +4699,19 @@ ProcessedFeedback const& JSHeapBroker::ReadFeedbackForPropertyAccess(
 
   MapHandles maps;
   nexus.ExtractMaps(&maps);
-  if (!maps.empty()) {
-    maps = GetRelevantReceiverMaps(isolate(), maps);
-    if (maps.empty()) return *new (zone()) InsufficientFeedback(kind);
+  FilterRelevantReceiverMaps(isolate(), &maps);
+
+  // If no maps were found for a non-megamorphic access, then our maps died and
+  // we should soft-deopt.
+  if (maps.empty() && nexus.ic_state() != MEGAMORPHIC) {
+    return *new (zone()) InsufficientFeedback(kind);
   }
 
   base::Optional<NameRef> name =
       static_name.has_value() ? static_name : GetNameFeedback(nexus);
   if (name.has_value()) {
+    // We rely on this invariant in JSGenericLowering.
+    DCHECK_IMPLIES(maps.empty(), nexus.ic_state() == MEGAMORPHIC);
     return *new (zone()) NamedAccessFeedback(
         *name, ZoneVector<Handle<Map>>(maps.begin(), maps.end(), zone()), kind);
   } else if (nexus.GetKeyType() == ELEMENT && !maps.empty()) {
@@ -4706,8 +4720,7 @@ ProcessedFeedback const& JSHeapBroker::ReadFeedbackForPropertyAccess(
   } else {
     // No actionable feedback.
     DCHECK(maps.empty());
-    // TODO(neis): Investigate if we really want to treat cleared the same as
-    // megamorphic (also for global accesses).
+    DCHECK_EQ(nexus.ic_state(), MEGAMORPHIC);
     // TODO(neis): Using ElementAccessFeedback here is kind of an abuse.
     return *new (zone())
         ElementAccessFeedback(zone(), KeyedAccessMode::FromNexus(nexus), kind);

src/ic/accessor-assembler.cc

@@ -3654,6 +3654,9 @@ void AccessorAssembler::GenerateLoadIC_Megamorphic() {
   TVARIABLE(MaybeObject, var_handler);
   Label if_handler(this, &var_handler), miss(this, Label::kDeferred);
 
+  CSA_ASSERT(this, TaggedEqual(LoadFeedbackVectorSlot(CAST(vector), slot),
+                               MegamorphicSymbolConstant()));
+
   TryProbeStubCache(isolate()->load_stub_cache(), receiver, CAST(name),
                     &if_handler, &var_handler, &miss);
 

test/mjsunit/compiler/monomorphic-named-load-with-no-map.js

+// Copyright 2020 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 --harmony-weak-refs --expose-gc
+
+// Helper to convert setTimeout into an awaitable promise.
+function asyncTimeout(timeout) {
+  return new Promise((resolve, reject)=>{
+    setTimeout(resolve, timeout);
+  })
+}
+
+function Foo() {}
+
+function getX(o) { return o.x; }
+
+(async function() {
+  let o = new Foo();
+  // Transition o:Foo to o:Foo{x}. This transition is important, as the o:Foo
+  // map is the initial map for the Foo constructor, and so is strongly held by
+  // it. We want o to be the only strong holder of its map.
+  o.x = 42;
+  %CompleteInobjectSlackTracking(new Foo());
+
+  // Warm up 'getX' with 'Foo{x}' feedback for its o.x access.
+  %PrepareFunctionForOptimization(getX);
+  assertEquals(getX(o), 42);
+  assertEquals(getX(o), 42);
+
+  // Clear out 'o', which is the only strong holder of the Foo{x} map.
+  let weak_o = new WeakRef(o);
+  o = null;
+
+  // Tick the message loop so that the weak ref can be collected.
+  await asyncTimeout(0);
+
+  // Collect the old 'o', which will also collect the 'Foo{x}' map.
+  gc();
+
+  // Make sure the old 'o' was collected.
+  assertEquals(undefined, weak_o.deref());
+
+  // Optimize the function with the current monomorphic 'Foo{x}' map o.x access,
+  // where the 'Foo{x}' map is dead and therefore the map set is empty. Then,
+  // create a new 'Foo{x}' object and pass that through. This compilation and
+  // o.x access should still succeed despite the dead map.
+  %OptimizeFunctionOnNextCall(getX);
+  o = new Foo();
+  o.x = 42;
+  assertEquals(getX(o), 42);
+
+})();