Skip to content

Commit 800c294

Browse files
mythrialleCommit Bot
mythrialle
authored and
Commit Bot
committed
[ic] Use the existing prototype validity cell when recomputing handlers
For keyed stores we recompute handlers based on the receiver maps we have seen. This is done so that we can transition to the most generic elements kind we have seen so far. When we recompute this handlers we get a new prototype validity cell and ignore the existing cell. This leads to incorrect behaviour if the cell was invalid. Recomputing the handler may be extra work which is not worth doing at this point. So we just reuse the existing validity cell and let the IC recompute the handler if we see the map again. Bug: chromium:1053939 Change-Id: Ifc891d70f5a4b8b774238e12fb40e29b4d174e37 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2122032 Commit-Queue: Mythri Alle <[email protected]> Reviewed-by: Toon Verwaest <[email protected]> Cr-Commit-Position: refs/heads/master@{#66963}
1 parent b8f14fc commit 800c294

7 files changed

Lines changed: 165 additions & 94 deletions

File tree

‎src/ic/handler-configuration.cc‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -200,11 +200,14 @@ KeyedAccessStoreMode StoreHandler::GetKeyedAccessStoreMode(
200200
// static
201201
Handle<Object> StoreHandler::StoreElementTransition(
202202
Isolate* isolate, Handle<Map> receiver_map, Handle<Map> transition,
203-
KeyedAccessStoreMode store_mode) {
203+
KeyedAccessStoreMode store_mode, MaybeHandle<Object> prev_validity_cell) {
204204
Handle<Code> stub =
205205
CodeFactory::ElementsTransitionAndStore(isolate, store_mode).code();
206-
Handle<Object> validity_cell =
207-
Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate);
206+
Handle<Object> validity_cell;
207+
if (!prev_validity_cell.ToHandle(&validity_cell)) {
208+
validity_cell =
209+
Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate);
210+
}
208211
Handle<StoreHandler> handler = isolate->factory()->NewStoreHandler(1);
209212
handler->set_smi_handler(*stub);
210213
handler->set_validity_cell(*validity_cell);

‎src/ic/handler-configuration.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -275,10 +275,10 @@ class StoreHandler final : public DataHandler {
275275
MaybeObjectHandle maybe_data1 = MaybeObjectHandle(),
276276
MaybeObjectHandle maybe_data2 = MaybeObjectHandle());
277277

278-
static Handle<Object> StoreElementTransition(Isolate* isolate,
279-
Handle<Map> receiver_map,
280-
Handle<Map> transition,
281-
KeyedAccessStoreMode store_mode);
278+
static Handle<Object> StoreElementTransition(
279+
Isolate* isolate, Handle<Map> receiver_map, Handle<Map> transition,
280+
KeyedAccessStoreMode store_mode,
281+
MaybeHandle<Object> prev_validity_cell = MaybeHandle<Object>());
282282

283283
static Handle<Object> StoreProxy(Isolate* isolate, Handle<Map> receiver_map,
284284
Handle<JSProxy> proxy,

‎src/ic/ic.cc‎

Lines changed: 95 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -364,9 +364,20 @@ void IC::ConfigureVectorState(Handle<Name> name, Handle<Map> map,
364364
void IC::ConfigureVectorState(Handle<Name> name, MapHandles const& maps,
365365
MaybeObjectHandles* handlers) {
366366
DCHECK(!IsGlobalIC());
367+
std::vector<MapAndHandler> maps_and_handlers;
368+
DCHECK_EQ(maps.size(), handlers->size());
369+
for (size_t i = 0; i < maps.size(); i++) {
370+
maps_and_handlers.push_back(MapAndHandler(maps[i], handlers->at(i)));
371+
}
372+
ConfigureVectorState(name, maps_and_handlers);
373+
}
374+
375+
void IC::ConfigureVectorState(
376+
Handle<Name> name, std::vector<MapAndHandler> const& maps_and_handlers) {
377+
DCHECK(!IsGlobalIC());
367378
// Non-keyed ICs don't track the name explicitly.
368379
if (!is_keyed()) name = Handle<Name>::null();
369-
nexus()->ConfigurePolymorphic(name, maps, handlers);
380+
nexus()->ConfigurePolymorphic(name, maps_and_handlers);
370381

371382
OnFeedbackChanged("Polymorphic");
372383
}
@@ -530,23 +541,39 @@ static bool AddOneReceiverMapIfMissing(MapHandles* receiver_maps,
530541
return true;
531542
}
532543

544+
static bool AddOneReceiverMapIfMissing(
545+
std::vector<MapAndHandler>* receiver_maps_and_handlers,
546+
Handle<Map> new_receiver_map) {
547+
DCHECK(!new_receiver_map.is_null());
548+
if (new_receiver_map->is_deprecated()) return false;
549+
for (MapAndHandler map_and_handler : *receiver_maps_and_handlers) {
550+
Handle<Map> map = map_and_handler.first;
551+
if (!map.is_null() && map.is_identical_to(new_receiver_map)) {
552+
return false;
553+
}
554+
}
555+
receiver_maps_and_handlers->push_back(
556+
MapAndHandler(new_receiver_map, MaybeObjectHandle()));
557+
return true;
558+
}
559+
533560
bool IC::UpdatePolymorphicIC(Handle<Name> name,
534561
const MaybeObjectHandle& handler) {
535562
DCHECK(IsHandler(*handler));
536563
if (is_keyed() && state() != RECOMPUTE_HANDLER) {
537564
if (nexus()->GetName() != *name) return false;
538565
}
539566
Handle<Map> map = receiver_map();
540-
MapHandles maps;
541-
MaybeObjectHandles handlers;
542567

543-
nexus()->ExtractMapsAndHandlers(&maps, &handlers);
544-
int number_of_maps = static_cast<int>(maps.size());
568+
std::vector<MapAndHandler> maps_and_handlers;
569+
nexus()->ExtractMapsAndHandlers(&maps_and_handlers);
570+
int number_of_maps = static_cast<int>(maps_and_handlers.size());
545571
int deprecated_maps = 0;
546572
int handler_to_overwrite = -1;
547573

548574
for (int i = 0; i < number_of_maps; i++) {
549-
Handle<Map> current_map = maps.at(i);
575+
Handle<Map> current_map = maps_and_handlers.at(i).first;
576+
MaybeObjectHandle current_handler = maps_and_handlers.at(i).second;
550577
if (current_map->is_deprecated()) {
551578
// Filter out deprecated maps to ensure their instances get migrated.
552579
++deprecated_maps;
@@ -556,7 +583,7 @@ bool IC::UpdatePolymorphicIC(Handle<Name> name,
556583
// in the lattice and need to go MEGAMORPHIC instead. There's one
557584
// exception to this rule, which is when we're in RECOMPUTE_HANDLER
558585
// state, there we allow to migrate to a new handler.
559-
if (handler.is_identical_to(handlers[i]) &&
586+
if (handler.is_identical_to(current_handler) &&
560587
state() != RECOMPUTE_HANDLER) {
561588
return false;
562589
}
@@ -584,16 +611,16 @@ bool IC::UpdatePolymorphicIC(Handle<Name> name,
584611
} else {
585612
if (is_keyed() && nexus()->GetName() != *name) return false;
586613
if (handler_to_overwrite >= 0) {
587-
handlers[handler_to_overwrite] = handler;
588-
if (!map.is_identical_to(maps.at(handler_to_overwrite))) {
589-
maps[handler_to_overwrite] = map;
614+
maps_and_handlers[handler_to_overwrite].second = handler;
615+
if (!map.is_identical_to(
616+
maps_and_handlers.at(handler_to_overwrite).first)) {
617+
maps_and_handlers[handler_to_overwrite].first = map;
590618
}
591619
} else {
592-
maps.push_back(map);
593-
handlers.push_back(handler);
620+
maps_and_handlers.push_back(MapAndHandler(map, handler));
594621
}
595622

596-
ConfigureVectorState(name, maps, &handlers);
623+
ConfigureVectorState(name, maps_and_handlers);
597624
}
598625

599626
return true;
@@ -606,11 +633,10 @@ void IC::UpdateMonomorphicIC(const MaybeObjectHandle& handler,
606633
}
607634

608635
void IC::CopyICToMegamorphicCache(Handle<Name> name) {
609-
MapHandles maps;
610-
MaybeObjectHandles handlers;
611-
nexus()->ExtractMapsAndHandlers(&maps, &handlers);
612-
for (size_t i = 0; i < maps.size(); ++i) {
613-
UpdateMegamorphicCache(maps.at(i), name, handlers.at(i));
636+
std::vector<MapAndHandler> maps_and_handlers;
637+
nexus()->ExtractMapsAndHandlers(&maps_and_handlers);
638+
for (const MapAndHandler& map_and_handler : maps_and_handlers) {
639+
UpdateMegamorphicCache(map_and_handler.first, name, map_and_handler.second);
614640
}
615641
}
616642

@@ -1768,9 +1794,9 @@ MaybeObjectHandle StoreIC::ComputeHandler(LookupIterator* lookup) {
17681794
void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
17691795
KeyedAccessStoreMode store_mode,
17701796
Handle<Map> new_receiver_map) {
1771-
MapHandles target_receiver_maps;
1772-
TargetMaps(&target_receiver_maps);
1773-
if (target_receiver_maps.empty()) {
1797+
std::vector<MapAndHandler> target_maps_and_handlers;
1798+
nexus()->ExtractMapsAndHandlers(&target_maps_and_handlers, true);
1799+
if (target_maps_and_handlers.empty()) {
17741800
Handle<Map> monomorphic_map = receiver_map;
17751801
// If we transitioned to a map that is a more general map than incoming
17761802
// then use the new map.
@@ -1781,7 +1807,8 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
17811807
return ConfigureVectorState(Handle<Name>(), monomorphic_map, handler);
17821808
}
17831809

1784-
for (Handle<Map> map : target_receiver_maps) {
1810+
for (const MapAndHandler& map_and_handler : target_maps_and_handlers) {
1811+
Handle<Map> map = map_and_handler.first;
17851812
if (!map.is_null() && map->instance_type() == JS_PRIMITIVE_WRAPPER_TYPE) {
17861813
DCHECK(!IsStoreInArrayLiteralICKind(kind()));
17871814
set_slow_stub_reason("JSPrimitiveWrapper");
@@ -1794,7 +1821,7 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
17941821
// Handle those here if the receiver map hasn't changed or it has transitioned
17951822
// to a more general kind.
17961823
KeyedAccessStoreMode old_store_mode = GetKeyedAccessStoreMode();
1797-
Handle<Map> previous_receiver_map = target_receiver_maps.at(0);
1824+
Handle<Map> previous_receiver_map = target_maps_and_handlers.at(0).first;
17981825
if (state() == MONOMORPHIC) {
17991826
Handle<Map> transitioned_receiver_map = new_receiver_map;
18001827
if (IsTransitionOfMonomorphicTarget(*previous_receiver_map,
@@ -1823,11 +1850,11 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
18231850
DCHECK(state() != GENERIC);
18241851

18251852
bool map_added =
1826-
AddOneReceiverMapIfMissing(&target_receiver_maps, receiver_map);
1853+
AddOneReceiverMapIfMissing(&target_maps_and_handlers, receiver_map);
18271854

18281855
if (IsTransitionOfMonomorphicTarget(*receiver_map, *new_receiver_map)) {
18291856
map_added |=
1830-
AddOneReceiverMapIfMissing(&target_receiver_maps, new_receiver_map);
1857+
AddOneReceiverMapIfMissing(&target_maps_and_handlers, new_receiver_map);
18311858
}
18321859

18331860
if (!map_added) {
@@ -1839,7 +1866,7 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
18391866

18401867
// If the maximum number of receiver maps has been exceeded, use the
18411868
// megamorphic version of the IC.
1842-
if (static_cast<int>(target_receiver_maps.size()) >
1869+
if (static_cast<int>(target_maps_and_handlers.size()) >
18431870
FLAG_max_polymorphic_map_count) {
18441871
return;
18451872
}
@@ -1860,36 +1887,37 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
18601887
// use the megamorphic stub.
18611888
if (store_mode != STANDARD_STORE) {
18621889
size_t external_arrays = 0;
1863-
for (Handle<Map> map : target_receiver_maps) {
1890+
for (MapAndHandler map_and_handler : target_maps_and_handlers) {
1891+
Handle<Map> map = map_and_handler.first;
18641892
if (map->has_typed_array_elements()) {
18651893
DCHECK(!IsStoreInArrayLiteralICKind(kind()));
18661894
external_arrays++;
18671895
}
18681896
}
18691897
if (external_arrays != 0 &&
1870-
external_arrays != target_receiver_maps.size()) {
1898+
external_arrays != target_maps_and_handlers.size()) {
18711899
DCHECK(!IsStoreInArrayLiteralICKind(kind()));
18721900
set_slow_stub_reason(
18731901
"unsupported combination of external and normal arrays");
18741902
return;
18751903
}
18761904
}
18771905

1878-
MaybeObjectHandles handlers;
1879-
handlers.reserve(target_receiver_maps.size());
1880-
StoreElementPolymorphicHandlers(&target_receiver_maps, &handlers, store_mode);
1881-
if (target_receiver_maps.size() == 0) {
1906+
StoreElementPolymorphicHandlers(&target_maps_and_handlers, store_mode);
1907+
if (target_maps_and_handlers.size() == 0) {
18821908
Handle<Object> handler = StoreElementHandler(receiver_map, store_mode);
18831909
ConfigureVectorState(Handle<Name>(), receiver_map, handler);
1884-
} else if (target_receiver_maps.size() == 1) {
1885-
ConfigureVectorState(Handle<Name>(), target_receiver_maps[0], handlers[0]);
1910+
} else if (target_maps_and_handlers.size() == 1) {
1911+
ConfigureVectorState(Handle<Name>(), target_maps_and_handlers[0].first,
1912+
target_maps_and_handlers[0].second);
18861913
} else {
1887-
ConfigureVectorState(Handle<Name>(), target_receiver_maps, &handlers);
1914+
ConfigureVectorState(Handle<Name>(), target_maps_and_handlers);
18881915
}
18891916
}
18901917

18911918
Handle<Object> KeyedStoreIC::StoreElementHandler(
1892-
Handle<Map> receiver_map, KeyedAccessStoreMode store_mode) {
1919+
Handle<Map> receiver_map, KeyedAccessStoreMode store_mode,
1920+
MaybeHandle<Object> prev_validity_cell) {
18931921
DCHECK_IMPLIES(
18941922
receiver_map->DictionaryElementsInPrototypeChainOnly(isolate()),
18951923
IsStoreInArrayLiteralICKind(kind()));
@@ -1925,8 +1953,11 @@ Handle<Object> KeyedStoreIC::StoreElementHandler(
19251953
}
19261954

19271955
if (IsStoreInArrayLiteralICKind(kind())) return code;
1928-
Handle<Object> validity_cell =
1929-
Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate());
1956+
Handle<Object> validity_cell;
1957+
if (!prev_validity_cell.ToHandle(&validity_cell)) {
1958+
validity_cell =
1959+
Map::GetOrCreatePrototypeChainValidityCell(receiver_map, isolate());
1960+
}
19301961
if (validity_cell->IsSmi()) {
19311962
// There's no prototype validity cell to check, so we can just use the stub.
19321963
return code;
@@ -1938,16 +1969,17 @@ Handle<Object> KeyedStoreIC::StoreElementHandler(
19381969
}
19391970

19401971
void KeyedStoreIC::StoreElementPolymorphicHandlers(
1941-
MapHandles* receiver_maps, MaybeObjectHandles* handlers,
1972+
std::vector<MapAndHandler>* receiver_maps_and_handlers,
19421973
KeyedAccessStoreMode store_mode) {
1943-
// Filter out deprecated maps to ensure their instances get migrated.
1944-
receiver_maps->erase(
1945-
std::remove_if(
1946-
receiver_maps->begin(), receiver_maps->end(),
1947-
[](const Handle<Map>& map) { return map->is_deprecated(); }),
1948-
receiver_maps->end());
1974+
std::vector<Handle<Map>> receiver_maps;
1975+
for (size_t i = 0; i < receiver_maps_and_handlers->size(); i++) {
1976+
receiver_maps.push_back(receiver_maps_and_handlers->at(i).first);
1977+
}
19491978

1950-
for (Handle<Map> receiver_map : *receiver_maps) {
1979+
for (size_t i = 0; i < receiver_maps_and_handlers->size(); i++) {
1980+
Handle<Map> receiver_map = receiver_maps_and_handlers->at(i).first;
1981+
DCHECK(!receiver_map->is_deprecated());
1982+
MaybeObjectHandle old_handler = receiver_maps_and_handlers->at(i).second;
19511983
Handle<Object> handler;
19521984
Handle<Map> transition;
19531985

@@ -1960,8 +1992,8 @@ void KeyedStoreIC::StoreElementPolymorphicHandlers(
19601992

19611993
} else {
19621994
{
1963-
Map tmap = receiver_map->FindElementsKindTransitionedMap(
1964-
isolate(), *receiver_maps);
1995+
Map tmap = receiver_map->FindElementsKindTransitionedMap(isolate(),
1996+
receiver_maps);
19651997
if (!tmap.is_null()) {
19661998
if (receiver_map->is_stable()) {
19671999
receiver_map->NotifyLeafMapLayoutChange(isolate());
@@ -1970,6 +2002,16 @@ void KeyedStoreIC::StoreElementPolymorphicHandlers(
19702002
}
19712003
}
19722004

2005+
MaybeHandle<Object> validity_cell;
2006+
HeapObject old_handler_obj;
2007+
if (!old_handler.is_null() &&
2008+
old_handler->GetHeapObject(&old_handler_obj) &&
2009+
old_handler_obj.IsDataHandler()) {
2010+
validity_cell = MaybeHandle<Object>(
2011+
DataHandler::cast(old_handler_obj).validity_cell(), isolate());
2012+
}
2013+
// TODO(mythria): Do not recompute the handler if we know there is no
2014+
// change in the handler.
19732015
// TODO(mvstanton): The code below is doing pessimistic elements
19742016
// transitions. I would like to stop doing that and rely on Allocation
19752017
// Site Tracking to do a better job of ensuring the data types are what
@@ -1978,14 +2020,15 @@ void KeyedStoreIC::StoreElementPolymorphicHandlers(
19782020
if (!transition.is_null()) {
19792021
TRACE_HANDLER_STATS(isolate(),
19802022
KeyedStoreIC_ElementsTransitionAndStoreStub);
1981-
handler = StoreHandler::StoreElementTransition(isolate(), receiver_map,
1982-
transition, store_mode);
2023+
handler = StoreHandler::StoreElementTransition(
2024+
isolate(), receiver_map, transition, store_mode, validity_cell);
19832025
} else {
1984-
handler = StoreElementHandler(receiver_map, store_mode);
2026+
handler = StoreElementHandler(receiver_map, store_mode, validity_cell);
19852027
}
19862028
}
19872029
DCHECK(!handler.is_null());
1988-
handlers->push_back(MaybeObjectHandle(handler));
2030+
receiver_maps_and_handlers->at(i) =
2031+
MapAndHandler(receiver_map, MaybeObjectHandle(handler));
19892032
}
19902033
}
19912034

‎src/ic/ic.h‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,8 @@ class IC {
8282
// Configure the vector for POLYMORPHIC.
8383
void ConfigureVectorState(Handle<Name> name, MapHandles const& maps,
8484
MaybeObjectHandles* handlers);
85+
void ConfigureVectorState(
86+
Handle<Name> name, std::vector<MapAndHandler> const& maps_and_handlers);
8587

8688
char TransitionMarkFromState(IC::State state);
8789
void TraceIC(const char* type, Handle<Object> name);
@@ -312,12 +314,13 @@ class KeyedStoreIC : public StoreIC {
312314
Handle<Map> ComputeTransitionedMap(Handle<Map> map,
313315
TransitionMode transition_mode);
314316

315-
Handle<Object> StoreElementHandler(Handle<Map> receiver_map,
316-
KeyedAccessStoreMode store_mode);
317+
Handle<Object> StoreElementHandler(
318+
Handle<Map> receiver_map, KeyedAccessStoreMode store_mode,
319+
MaybeHandle<Object> prev_validity_cell = MaybeHandle<Object>());
317320

318-
void StoreElementPolymorphicHandlers(MapHandles* receiver_maps,
319-
MaybeObjectHandles* handlers,
320-
KeyedAccessStoreMode store_mode);
321+
void StoreElementPolymorphicHandlers(
322+
std::vector<MapAndHandler>* receiver_maps_and_handlers,
323+
KeyedAccessStoreMode store_mode);
321324

322325
friend class IC;
323326
};

0 commit comments

Comments
 (0)