Commit 40c68926 authored by Benedikt Meurer's avatar Benedikt Meurer Committed by Commit Bot

[feedback-vector] Don't go MEGAMORPHIC due to dying handlers.

This fixes a problem where ICs for transitioning stores go MEGAMORPHIC
if the transition target map dies in between invocations of the IC,
which is totally possible, since we only hold on weakly to these
transition targets (both from the FeedbackVectors and also from the
TransitonArrays).

The root problem here was an inconsistency in how the maps and handlers
are being reported by the FeedbackVector. On the on hand side the method
FeedbackVector::ExtractMaps() will report all receiver maps that are
still present (i.e. which haven't died themselves), but then the other
method FeedbackVector::FindHandlers() will only report handlers that are
still alive (i.e. which in case of transition target maps being used as
handlers haven't died yet). If the length of these lists don't match the
IC chickens out and goes MEGAMORPHIC. But this is exactly the case with
the transitioning stores, where there's no handler anymore, i.e. as can
be seen in this simple example:

```
// Flags: --expose-gc
function C() { this.x = 1; }
new C();
new C();
gc();     // map with the `C.x` property dies
new C();  // now the STORE_IC in C goes MEGAMORPHIC
```

So the problem is that we have these two methods that don't agree with
each other. Now FeedbackVector::ExtractMaps() is also used by TurboFan
and it even reports receiver maps for PREMONOMORPHIC state, which is
different from the use case that the ICs need. So I replaced the
FeedbackVector::FindHandlers() with a completely new method
FeedbackVector::ExtractMapsAndHandlers(), which returns both the maps
and handlers, exactly as the ICs need it. And only returns pairs for
which both the receiver map and the handler are still alive.

This fixes the odd problem that sometimes STORE_ICs going MEGAMORPHIC
for no apparent reason. Due to the weakness of the transition target
maps, they can still die and cause deoptimizations, but at least
TurboFan will now be able to reoptimize again later with the new maps
and still generate proper code.

Bug: v8:9316
Cq-Include-Trybots: luci.chromium.try:linux-rel,win7-rel
Change-Id: I74c8b60f792f310dc813f997e69efe9ad434296a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1637878
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: 's avatarMichael Stanton <mvstanton@chromium.org>
Reviewed-by: 's avatarJaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61948}
parent b0980748
......@@ -573,11 +573,10 @@ bool IC::UpdatePolymorphicIC(Handle<Name> name,
MapHandles maps;
MaybeObjectHandles handlers;
TargetMaps(&maps);
nexus()->ExtractMapsAndHandlers(&maps, &handlers);
int number_of_maps = static_cast<int>(maps.size());
int deprecated_maps = 0;
int handler_to_overwrite = -1;
if (!nexus()->FindHandlers(&handlers, number_of_maps)) return false;
for (int i = 0; i < number_of_maps; i++) {
Handle<Map> current_map = maps.at(i);
......@@ -642,9 +641,8 @@ void IC::UpdateMonomorphicIC(const MaybeObjectHandle& handler,
void IC::CopyICToMegamorphicCache(Handle<Name> name) {
MapHandles maps;
MaybeObjectHandles handlers;
TargetMaps(&maps);
if (!nexus()->FindHandlers(&handlers, static_cast<int>(maps.size()))) return;
for (int i = 0; i < static_cast<int>(maps.size()); i++) {
nexus()->ExtractMapsAndHandlers(&maps, &handlers);
for (size_t i = 0; i < maps.size(); ++i) {
UpdateMegamorphicCache(maps.at(i), name, handlers.at(i));
}
}
......
......@@ -943,6 +943,7 @@ int FeedbackNexus::ExtractMaps(MapHandles* maps) const {
IsStoreOwnICKind(kind()) || IsStoreDataPropertyInLiteralKind(kind()) ||
IsStoreInArrayLiteralICKind(kind()) || IsKeyedHasICKind(kind()));
DisallowHeapAllocation no_gc;
Isolate* isolate = GetIsolate();
MaybeObject feedback = GetFeedback();
bool is_named_feedback = IsPropertyNameFeedback(feedback);
......@@ -986,19 +987,22 @@ int FeedbackNexus::ExtractMaps(MapHandles* maps) const {
return 0;
}
MaybeObjectHandle FeedbackNexus::FindHandlerForMap(Handle<Map> map) const {
int FeedbackNexus::ExtractMapsAndHandlers(MapHandles* maps,
MaybeObjectHandles* handlers) const {
DCHECK(IsLoadICKind(kind()) || IsStoreICKind(kind()) ||
IsKeyedLoadICKind(kind()) || IsKeyedStoreICKind(kind()) ||
IsStoreOwnICKind(kind()) || IsStoreDataPropertyInLiteralKind(kind()) ||
IsKeyedHasICKind(kind()));
IsStoreInArrayLiteralICKind(kind()) || IsKeyedHasICKind(kind()));
MaybeObject feedback = GetFeedback();
DisallowHeapAllocation no_gc;
Isolate* isolate = GetIsolate();
MaybeObject feedback = GetFeedback();
bool is_named_feedback = IsPropertyNameFeedback(feedback);
HeapObject heap_object;
if ((feedback->GetHeapObjectIfStrong(&heap_object) &&
heap_object.IsWeakFixedArray()) ||
is_named_feedback) {
int found = 0;
WeakFixedArray array;
if (is_named_feedback) {
array =
......@@ -1011,36 +1015,39 @@ MaybeObjectHandle FeedbackNexus::FindHandlerForMap(Handle<Map> map) const {
for (int i = 0; i < array.length(); i += increment) {
DCHECK(array.Get(i)->IsWeakOrCleared());
if (array.Get(i)->GetHeapObjectIfWeak(&heap_object)) {
Map array_map = Map::cast(heap_object);
if (array_map == *map && !array.Get(i + increment - 1)->IsCleared()) {
MaybeObject handler = array.Get(i + increment - 1);
MaybeObject handler = array.Get(i + 1);
if (!handler->IsCleared()) {
DCHECK(IC::IsHandler(handler));
return handle(handler, isolate);
Map map = Map::cast(heap_object);
maps->push_back(handle(map, isolate));
handlers->push_back(handle(handler, isolate));
found++;
}
}
}
return found;
} else if (feedback->GetHeapObjectIfWeak(&heap_object)) {
Map cell_map = Map::cast(heap_object);
if (cell_map == *map && !GetFeedbackExtra()->IsCleared()) {
MaybeObject handler = GetFeedbackExtra();
MaybeObject handler = GetFeedbackExtra();
if (!handler->IsCleared()) {
DCHECK(IC::IsHandler(handler));
return handle(handler, isolate);
Map map = Map::cast(heap_object);
maps->push_back(handle(map, isolate));
handlers->push_back(handle(handler, isolate));
return 1;
}
}
return MaybeObjectHandle();
return 0;
}
bool FeedbackNexus::FindHandlers(MaybeObjectHandles* code_list,
int length) const {
MaybeObjectHandle FeedbackNexus::FindHandlerForMap(Handle<Map> map) const {
DCHECK(IsLoadICKind(kind()) || IsStoreICKind(kind()) ||
IsKeyedLoadICKind(kind()) || IsKeyedStoreICKind(kind()) ||
IsStoreOwnICKind(kind()) || IsStoreDataPropertyInLiteralKind(kind()) ||
IsStoreInArrayLiteralICKind(kind()) || IsKeyedHasICKind(kind()));
IsKeyedHasICKind(kind()));
MaybeObject feedback = GetFeedback();
Isolate* isolate = GetIsolate();
int count = 0;
bool is_named_feedback = IsPropertyNameFeedback(feedback);
HeapObject heap_object;
if ((feedback->GetHeapObjectIfStrong(&heap_object) &&
......@@ -1056,25 +1063,26 @@ bool FeedbackNexus::FindHandlers(MaybeObjectHandles* code_list,
const int increment = 2;
HeapObject heap_object;
for (int i = 0; i < array.length(); i += increment) {
// Be sure to skip handlers whose maps have been cleared.
DCHECK(array.Get(i)->IsWeakOrCleared());
if (array.Get(i)->GetHeapObjectIfWeak(&heap_object) &&
!array.Get(i + increment - 1)->IsCleared()) {
MaybeObject handler = array.Get(i + increment - 1);
DCHECK(IC::IsHandler(handler));
code_list->push_back(handle(handler, isolate));
count++;
if (array.Get(i)->GetHeapObjectIfWeak(&heap_object)) {
Map array_map = Map::cast(heap_object);
if (array_map == *map && !array.Get(i + increment - 1)->IsCleared()) {
MaybeObject handler = array.Get(i + increment - 1);
DCHECK(IC::IsHandler(handler));
return handle(handler, isolate);
}
}
}
} else if (feedback->GetHeapObjectIfWeak(&heap_object)) {
MaybeObject extra = GetFeedbackExtra();
if (!extra->IsCleared()) {
DCHECK(IC::IsHandler(extra));
code_list->push_back(handle(extra, isolate));
count++;
Map cell_map = Map::cast(heap_object);
if (cell_map == *map && !GetFeedbackExtra()->IsCleared()) {
MaybeObject handler = GetFeedbackExtra();
DCHECK(IC::IsHandler(handler));
return handle(handler, isolate);
}
}
return count == length;
return MaybeObjectHandle();
}
Name FeedbackNexus::GetName() const {
......@@ -1095,8 +1103,7 @@ KeyedAccessLoadMode FeedbackNexus::GetKeyedAccessLoadMode() const {
if (GetKeyType() == PROPERTY) return STANDARD_LOAD;
ExtractMaps(&maps);
FindHandlers(&handlers, static_cast<int>(maps.size()));
ExtractMapsAndHandlers(&maps, &handlers);
for (MaybeObjectHandle const& handler : handlers) {
KeyedAccessLoadMode mode = LoadHandler::GetKeyedAccessLoadMode(*handler);
if (mode != STANDARD_LOAD) return mode;
......@@ -1179,8 +1186,7 @@ KeyedAccessStoreMode FeedbackNexus::GetKeyedAccessStoreMode() const {
if (GetKeyType() == PROPERTY) return mode;
ExtractMaps(&maps);
FindHandlers(&handlers, static_cast<int>(maps.size()));
ExtractMapsAndHandlers(&maps, &handlers);
for (const MaybeObjectHandle& maybe_code_handler : handlers) {
// The first handler that isn't the slow handler will have the bits we need.
Handle<Code> handler;
......
......@@ -646,8 +646,9 @@ class V8_EXPORT_PRIVATE FeedbackNexus final {
Map GetFirstMap() const;
int ExtractMaps(MapHandles* maps) const;
int ExtractMapsAndHandlers(MapHandles* maps,
MaybeObjectHandles* handlers) const;
MaybeObjectHandle FindHandlerForMap(Handle<Map> map) const;
bool FindHandlers(MaybeObjectHandles* code_list, int length = -1) const;
bool IsCleared() const {
InlineCacheState state = ic_state();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment